Skip to content

Conversation

@vigaur
Copy link
Contributor

@vigaur vigaur commented Dec 16, 2019

Latest improvements:

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

  • I have reviewed the documentation for the workflow.
  • Validation tools were run on swagger spec(s) and have all been fixed in this PR.
  • The OpenAPI Hub was used for checking validation status and next steps.

ARM API Review Checklist

  • Service team MUST add the "WaitForARMFeedback" label if the management plane API changes fall into one of the below categories.
  • adding/removing APIs.
  • adding/removing properties.
  • adding/removing API-version.
  • adding a new service in Azure.

Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.

  • If you are blocked on ARM review and want to get the PR merged urgently, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
    Please follow the link to find more details on API review process.

@AutorestCI
Copy link

You don't have permission to trigger SDK Automation.
Please add yourself to Azure group from opensource portal if you are MSFT employee,
or please ask reviewer to add comment *** /openapibot sdkautomation ***.
Please ask [email protected] (or NullMDR in github) for additional help.

@msftclas
Copy link

msftclas commented Dec 16, 2019

CLA assistant check
All CLA requirements met.

@vigaur vigaur changed the title swagger changes for zone to zone recovery plan ASR : swagger changes for zone to zone recovery plan Dec 16, 2019
@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@vigaur
Copy link
Contributor Author

vigaur commented Dec 16, 2019

Model validation and semantic validations are not due to this PR and are being fixed as a separate PR : #7863

@vigaur
Copy link
Contributor Author

vigaur commented Dec 17, 2019

@myronfanqiu please take a look at this PR. Also please note that the changes are not breaking changes. thanks

@mmyyrroonn mmyyrroonn added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Dec 17, 2019
@mmyyrroonn
Copy link
Contributor

@vigaur Could you solve the CI errors? You need to change the swagger spec and change the example files.

@vigaur
Copy link
Contributor Author

vigaur commented Dec 17, 2019

@myronfanqiu Please note that the Model validation and semantic validation errors are not result of current changes, Another PR is being raised for it along with #7863

Also please let me know who should be added to get the ARM feedback for the PR, thanks

@mmyyrroonn
Copy link
Contributor

@vigaur Can you fix another CI errors in this PR too? I know it's not related with the changes happened in this PR. But it's better to improve the quality of examples.

@vigaur
Copy link
Contributor Author

vigaur commented Dec 20, 2019

@myronfanqiu I understand the concern, though since these examples involve multiple stakeholders, we are taking this as back log item and will raise a separate PR for this, thanks

@mmyyrroonn
Copy link
Contributor

@vigaur Hello. We still need ARM's review. BTW, I don't have the right to merge it without CI pass. I will invite the correct person to help you.

@mmyyrroonn
Copy link
Contributor

@NullMDR Could you involve the right person to help on this PR? @vigaur want to merge this PR without CI pass.

@PhoenixHe-NV
Copy link

/azp run automation - sdk

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@PhoenixHe-NV
Copy link

@vigaur Please supress model validation error in readme.md. You can reference https://github.com/Azure/azure-rest-api-specs/blob/master/specification/network/resource-manager/readme.md#suppression as example

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Dec 23, 2019

azure-sdk-for-java - Release

️✔️ succeeded [Logs] [Expand Details]

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Dec 23, 2019

azure-sdk-for-go - Release

️✔️ succeeded [Logs] [Expand Details]

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Dec 23, 2019

azure-sdk-for-python - Release

⚠️ warning [Logs] [Expand Details]
  • ⚠️ Generate from 8803e2b with merge commit d1441c8. SDK Automation 13.0.17.20191226.1
    Failed to find any diff after autorest so no changed packages was found.

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Dec 23, 2019

azure-sdk-for-net - Release

failed [Logs] [Expand Details]
  • Generate from 8803e2b with merge commit d1441c8. SDK Automation 13.0.17.20191226.1
    [AutoRest] realpath(): Permission denied
    [AutoRest] realpath(): Permission denied
    [AutoRest] realpath(): Permission denied
    [AutoRest] realpath(): Permission denied
    [AutoRest] realpath(): Permission denied
    [AutoRest] realpath(): Permission denied
  • Microsoft.Azure.Management.RecoveryServices.SiteRecovery [Logs]  [Release SDK Changes]
      Failed to create the package Microsoft.Azure.Management.RecoveryServices.SiteRecovery.
      Error: dotnet msbuild build.proj /t:CreateNugetPackage /p:Scope=recoveryservices-siterecovery /v:n /p:SkipTests=true , {} 

    @openapi-sdkautomation
    Copy link

    openapi-sdkautomation bot commented Dec 23, 2019

    azure-sdk-for-js - Release

    ️✔️ succeeded [Logs] [Expand Details]
    • ️✔️ Generate from 8803e2b with merge commit d1441c8. SDK Automation 13.0.17.20191226.1
    • ️✔️@azure/arm-recoveryservices-siterecovery [Logs]  [Release SDK Changes]
      [npmPack] npm WARN deprecated [email protected]: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-node-resolve.
      [npmPack] loaded rollup.config.js with warnings
      [npmPack] (!) Unused external imports
      [npmPack] default imported from external module 'rollup' but never used
      [npmPack] 
      [npmPack] ./esm/siteRecoveryManagementClient.js → ./dist/arm-recoveryservices-siterecovery.js...
      [npmPack] created ./dist/arm-recoveryservices-siterecovery.js in 1s

    @mmyyrroonn
    Copy link
    Contributor

    @vigaur Hello. Could you solve the ARM's review comments?

    @vigaur
    Copy link
    Contributor Author

    vigaur commented Jan 14, 2020

    @ryansbenson I have resolved all the comments. We are using different objects for input (RecoveryPlanProviderSpecificInput here) and output (RecoveryPlanProviderSpecificDetails here) for many other entities in our code. This is the case since, sometime we may send additional properties in output which may not be present in input (we may add extra properties for recovery plan output which may not be present in input). Also, sometimes for the same property output format may be different than input format.

    Copy link
    Contributor

    @ryansbenson ryansbenson left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I understand what you're doing, its just going to generate an unusable sdk. The type model for get and put are completely different so there is no way to get, update a single field, put. In your case there would be an entire deep details -> input mapping step needed. I highly suggest you try using your own SDK and decided if you think its acceptable. As this is a common pattern across this your swagger I'll approve it, though please own customer satisfaction for the SDK.

    @ryansbenson ryansbenson added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed ARMChangesRequested WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Jan 14, 2020
    @mmyyrroonn mmyyrroonn closed this Jan 16, 2020
    @mmyyrroonn mmyyrroonn reopened this Jan 16, 2020
    @mmyyrroonn
    Copy link
    Contributor

    re-trigger the CI

    @azure-pipelines
    Copy link

    Azure Pipelines successfully started running 1 pipeline(s).

    @PhoenixHe-NV
    Copy link

    /azp run automation - sdk

    @azure-pipelines
    Copy link

    Azure Pipelines successfully started running 1 pipeline(s).

    @mmyyrroonn
    Copy link
    Contributor

    /azp run automation - sdk

    @azure-pipelines
    Copy link

    Azure Pipelines successfully started running 1 pipeline(s).

    @mmyyrroonn
    Copy link
    Contributor

    /azp run PrettierCheck

    @azure-pipelines
    Copy link

    No pipelines are associated with this pull request.

    @PhoenixHe-NV
    Copy link

    /azp run public.rest-api-specs

    @azure-pipelines
    Copy link

    Azure Pipelines successfully started running 1 pipeline(s).

    @mmyyrroonn
    Copy link
    Contributor

    @vigaur Hello. Could you fix errors in PrettierCheck?

    @azure-pipelines
    Copy link

    Azure Pipelines successfully started running 1 pipeline(s).

    @azure-pipelines
    Copy link

    Azure Pipelines successfully started running 1 pipeline(s).

    @PhoenixHe-NV
    Copy link

    /azp run automation - sdk

    @azure-pipelines
    Copy link

    Azure Pipelines successfully started running 1 pipeline(s).

    @PhoenixHe-NV PhoenixHe-NV merged commit d1441c8 into Azure:master Jan 29, 2020
    ssripadham pushed a commit to ssripadham/azure-rest-api-specs that referenced this pull request Feb 21, 2020
    * swagger changes for zone to zone recovery plan
    
    * zone to zone recovery plan provider specific read only property update
    
    * prettier format
    
    * prettier formatting
    00Kai0 pushed a commit to 00Kai0/azure-rest-api-specs that referenced this pull request Oct 12, 2020
    * swagger changes for zone to zone recovery plan
    
    * zone to zone recovery plan provider specific read only property update
    
    * prettier format
    
    * prettier formatting
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    7 participants