Skip to content
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

[correctness] updated operation type in Storage container to correct payload #31653

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

hiaga
Copy link
Member

@hiaga hiaga commented Nov 25, 2024

Choose a PR Template

Switch to "Preview" on this description then select one of the choices below.

Click here to open a PR for a Data Plane API.

Click here to open a PR for a Control Plane (ARM) API.

Copy link

openapi-pipeline-app bot commented Nov 25, 2024

Next Steps to Merge

✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge.

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

Microsoft.RecoveryServices

@AzureRestAPISpecReview AzureRestAPISpecReview added ARMReview BreakingChangeReviewRequired <valid label in PR review process>add this label when breaking change review is required NotReadyForARMReview ReadyForApiTest <valid label in PR review process>add this label when swagger and service APIs are ready for test resource-manager labels Nov 25, 2024
@hiaga hiaga added the PublishToCustomers Acknowledgement the changes will be published to Azure customers. label Nov 25, 2024
@hiaga hiaga added the Versioning-Approved-BugFix https://github.com/Azure/azure-sdk-tools/issues/6374 label Nov 25, 2024
@hiaga hiaga added the AzCoreIDC label Dec 3, 2024
@viveksheelsingh viveksheelsingh added the ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review label Dec 3, 2024
Copy link

@viveksheelsingh viveksheelsingh left a comment

Choose a reason for hiding this comment

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

Since there is no svc behaviour chnage, and breaking chnage board has approved, approving.

@mikekistler
Copy link
Member

@mikeharder Pls have a look at this PR. I think it should have been labeled VersioningReviewRequired since only compatible changes were made and with no API version change, but the bot labelled it BreakingChangeReviewRequired. There were no issues flagged by the BreakingChanges (Cross Version) PR check.

@mikeharder
Copy link
Member

mikeharder commented Dec 3, 2024

@mikeharder Pls have a look at this PR. I think it should have been labeled VersioningReviewRequired since only compatible changes were made and with no API version change, but the bot labelled it BreakingChangeReviewRequired. There were no issues flagged by the BreakingChanges (Cross Version) PR check.

Check BreakingChanges (Cross Version) passed, but check Swagger BreakingChanges failed. Given this, do you still think this PR should not have been lableled BreakingChangeReviewRequired?

I believe the current behavior matches the documentation. See the second bullet point here:

https://eng.ms/docs/products/azure-developer-experience/design/specs-pr-guides/pr-brch-deep#breakingchangereviewrequired-label

@mikekistler
Copy link
Member

In the doc you linked it says:

The label VersioningReviewRequired denotes that your PR has a versioning issue possibly violating the Azure Versioning and Breaking Changes Policy. This happens when you made a non-breaking change, aka compatible change to an API version that was already published (merged to a production branch).

The issues flagged in the Swagger BreakingChanges check -- added enum values and added optional property -- would be perfectly fine if done in a new api-version. They are flagged only because they were made in an existing api-version. So I do think that VersioningReviewRequired is the correct label.

@hiaga
Copy link
Member Author

hiaga commented Dec 8, 2024

Hi @mikeharder, @mikekistler could you please help find conclusion to above discussion. We're currently blocked on this PR for some PowerShell changes

@mikekistler mikekistler added the BreakingChange-Approved-BugFix Changes are to correct the REST API definition to correctly describe service behavior label Dec 8, 2024
@hiaga hiaga merged commit 552b4dd into Azure:main Dec 9, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARMReview ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review AzCoreIDC BreakingChange-Approved-BugFix Changes are to correct the REST API definition to correctly describe service behavior BreakingChangeReviewRequired <valid label in PR review process>add this label when breaking change review is required NotReadyForARMReview PublishToCustomers Acknowledgement the changes will be published to Azure customers. ReadyForApiTest <valid label in PR review process>add this label when swagger and service APIs are ready for test Recovery Services Backup resource-manager Versioning-Approved-BugFix https://github.com/Azure/azure-sdk-tools/issues/6374
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants