-
Notifications
You must be signed in to change notification settings - Fork 5.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Hub Generated] Review request for Microsoft.Devices to add version stable/2020-03-01 #10114
[Hub Generated] Review request for Microsoft.Devices to add version stable/2020-03-01 #10114
Conversation
…01 to version 2020-03-01
…3-01' of https://github.com/rajeevmv/azure-rest-api-specs into dev-deviceprovisioningservices-Microsoft.Devices-2020-03-01
Azure Pipelines successfully started running 1 pipeline(s). |
Azure CLI Extension Generation - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
Trenton Generation - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-python - Release
|
azure-sdk-for-js - Release
|
azure-sdk-for-go - Release
|
azure-sdk-for-net - Release
|
azure-sdk-for-java - Release
|
Can one of the admins verify this patch? |
"$ref": "#/definitions/PrivateEndpointConnection" | ||
} | ||
}, | ||
"PrivateEndpointConnection": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There're common models defined for PrivateLinks. Suggest to re-use them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required? or optional? The API has been integrated with Networking and UX and I would like to avoid changing them if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that only storage and storagesync team are referring to this common model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required for all the new references. Otherwise it's very hard to keep the consistency. Please use the common model as suggested above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feature has been shipped and is GA. We cannot make this change without breaking this API.
} | ||
} | ||
}, | ||
"ErrorDetails": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you reference the ErrorResponse defined in common type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The definition of ErrorResponse and ErrorDetails are different. I cannot change this without breaking the current API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the lintDiff check failures which is related to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too, I cannot make this change without breaking the API of this feature which is GA
Thanks for using the copy the API in first commit and separating the changes. It helps immensely while reviewing. |
@akning-ms , can you help force merge this PR since the change is to align with service in production? Thanks. |
|
||
``` yaml | ||
directive: | ||
- suppress: PageableOperation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these three rules are warnings and doesn't need to be suppressed. It would not fail the lintDiff check. Can you remove this suppression section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the suppression part.
@rajeevmv , can you remove the suppression part? |
Pull request contains merge conflicts. |
Pull request contains merge conflicts. |
Merge from main fork
Removed suppressions |
No commit pushedDate could be found for PR 10114 in repo Azure/azure-rest-api-specs |
/azp run automation - sdk |
Azure Pipelines successfully started running 1 pipeline(s). |
azure-resource-manager-schemas - Release
|
azure-sdk-for-python-track2 - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
Hi @akning-ms , could you help force merge this PR since the change is to align with service in production? Thanks. |
This is a PR generated at OpenAPI Hub. You can view your work branch via this link.
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.
Please follow the link to find more details on PR review process.