From 2eb9a4dad980b8a1d33416a4dcbb8f86e8955a78 Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Wed, 2 Aug 2023 13:15:24 -0700 Subject: [PATCH] Improve ARM PR template and guidance around breaking changes (#25086) --- .../control_plane_template.md | 27 +++++++------ .github/comment.yml | 40 +++++++++---------- 2 files changed, 35 insertions(+), 32 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE/control_plane_template.md b/.github/PULL_REQUEST_TEMPLATE/control_plane_template.md index b95dc392dafb..465280ea095c 100644 --- a/.github/PULL_REQUEST_TEMPLATE/control_plane_template.md +++ b/.github/PULL_REQUEST_TEMPLATE/control_plane_template.md @@ -31,25 +31,27 @@ What's the purpose of this PR? Check all that apply. This is **mandatory**! To merge this PR, you **must** go through the following checklist and confirm you understood and followed the instructions by checking all the boxes: -- [ ] I have reviewed the general guidance on the spec PR review process: https://aka.ms/specprreview. -- [ ] I confirm this PR is modifying Azure Resource Manager (ARM) related specifications, and not data-plane related specifications. -- [ ] I commit to follow the [Breaking Change Policy](https://aka.ms/AzBreakingChangesPolicy). +- [ ] I confirm this PR is modifying Azure Resource Manager (ARM) related specifications, and not data plane related specifications. - [ ] I have reviewed following [Resource Provider guidelines](https://aka.ms/rpguidelines), including [ARM resource provider contract](https://github.com/Azure/azure-resource-manager-rpc) and [REST guidelines](https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md) (estimated time: 4 hours). - I understand this is required before I can request review from an ARM API Review board. + I understand this is required before I can proceed to Step 2, "ARM Review", for this PR. -### ARM API changes review +### Breaking changes review (Step 1) -- If you want for the ARM team to review this PR, you must add the `ARMReview` label. -- The automation may automatically add the `ARMReview` label, if appropriate. - If this happens, proceed according to guidance given in GitHub comments also added by the automation. +- If the automation determines you have breaking changes, i.e. Step 1 from the diagram applies to you, + you must follow the [breaking changes process]. + **IMPORTANT** This applies even if: + - The tool fails while it shouldn't, e.g. due to runtime exception, or incorrect detection of breaking changes. + - You believe there is no need for you to request breaking change approval, for any reason. + Such claims must be reviewed, and the process is the same. -### Breaking change review +### ARM API changes review (Step 2) + +- If this PR is in purview of ARM review then automation will add the `ARMReview` label. +- If you want to force ARM review, add the label yourself. +- Proceed according to the diagram at the top of this comment. -If you have any breaking changes as defined in the [Breaking Change Policy](https://aka.ms/AzBreakingChangesPolicy/), -follow the process outlined in the [High-level Breaking Change Process doc](https://aka.ms/breakingchangesprocess#high-level-breaking-change-process). - ## Getting help - For general PR approval workflow, see the diagram at the top of this comment. @@ -62,3 +64,4 @@ follow the process outlined in the [High-level Breaking Change Process doc](http [List of SDK breaking changes approvers]: https://teams.microsoft.com/l/message/19:0351f5f9404446e4b4fd4eaf2c27448d@thread.skype/1689115217750?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=3e17dcb0-4257-4a30-b843-77f47f1d4121&parentMessageId=1689115217750&teamName=Azure%20SDK&channelName=API%20Spec%20Review&createdTime=1689115217750 [public repo merge queue]: https://github.com/Azure/azure-rest-api-specs/pulls?q=is%3Aopen+is%3Apr+label%3AMergeRequested+-label%3AIDCDevDiv+draft%3Afalse+sort%3Acreated-asc [private repo merge queue]: https://github.com/Azure/azure-rest-api-specs-pr/pulls?q=is%3Aopen+is%3Apr+label%3AMergeRequested+-label%3AIDCDevDiv+draft%3Afalse+sort%3Acreated-asc +[breaking changes process]: https://eng.ms/docs/cloud-ai-platform/azure-core/azure-core-pm-and-design/trusted-platform-pm-karimb/service-lifecycle-and-actions-team/service-lifecycle-actions-team/apex/media/launchingproductbreakingchanges#breaking-change-process-1 diff --git a/.github/comment.yml b/.github/comment.yml index 69a048770786..b9e4f97e1359 100644 --- a/.github/comment.yml +++ b/.github/comment.yml @@ -14,15 +14,16 @@

**ACTION ITEM ALTERNATIVE B**: Request approval.
Alternatively, if you cannot fix the breaking changes, then you can request an approval for them. - Please follow the process described in the - [High-level Breaking Change Process doc](https://aka.ms/breakingchangesprocess#high-level-breaking-change-process). -

- **ACTION ITEM ALTERNATIVE C**: Report false positive.
- If you think there are no breaking changes, - i.e. the validation should pass yet it fails, - then proceed as explained in **ACTION ITEM ALTERNATIVE B**.
- This applies even if the breaking change tool fails with internal runtime error. - In such case a manual breaking change review is necessary. + Please follow the [breaking changes process](https://eng.ms/docs/cloud-ai-platform/azure-core/azure-core-pm-and-design/trusted-platform-pm-karimb/service-lifecycle-and-actions-team/service-lifecycle-actions-team/apex/media/launchingproductbreakingchanges#breaking-change-process-1). +
+ This case applies even if: + - rule: type: label @@ -43,17 +44,16 @@ **ACTION ITEM ALTERNATIVE B**: Request approval.
Alternatively, if you cannot avoid modifying existing API versions, then you can request an approval for them. - Please follow the process described in the - [High-level Breaking Change Process doc](https://aka.ms/breakingchangesprocess#high-level-breaking-change-process). -

- **ACTION ITEM ALTERNATIVE C**: Report false positive.
- If you think there are no changes in existing API version, - i.e. there should be no `NewApiVersionRequired` label, - then proceed as explained in **ACTION ITEM ALTERNATIVE B**.
- This applies even if the breaking change tool fails with internal runtime error. - In such case a manual breaking change review is necessary. -

- For additional guidance, please see https://aka.ms/NewApiVersionRequired + Please follow the [breaking changes process](https://eng.ms/docs/cloud-ai-platform/azure-core/azure-core-pm-and-design/trusted-platform-pm-karimb/service-lifecycle-and-actions-team/service-lifecycle-actions-team/apex/media/launchingproductbreakingchanges#breaking-change-process-1). +
+ This case applies even if: + - rule: type: label