Add the ARM review labels only after NewAPIVersionRequired is addressed

[bdefoy]

As a part of the automation, it should be made clear that the author needs to follow up with the NewAPIVersionRequired action label to create a new API version before moving on to the ARM API review. Only after the NewAPIVersionRequired label is addressed should the WaitForARMfeedback label be added, making the PR actionable for the ARM reviewers.

Additionally, when the NewApiVersionRequired label is added by automation, it should remove the WaitForARMFeedback label, add the ARMChangesRequested label, and add a comment for the PR Author to remove the ARMChangesRequested label once they have created a new API version or obtained an exception from the Breaking Changes Review Board.

[konrad-jamrozik]

@bdefoy I have some clarifying questions.

Re:

As a part of the automation, it should be made clear that the author needs to follow up with the NewAPIVersionRequired action label to create a new API version before moving on to the ARM API review. Only after the NewAPIVersionRequired label is addressed should the WaitForARMfeedback label be added, making the PR actionable for the ARM reviewers.

If I understand correctly, you are asking for it to be made clearer to the PR submitter what is the meaning of the NewAPIVersionRequired label, and relevant action items? I understand that label should only ever by added by our automation, not by hand. By the breaking change detectors we have, to be exact (I grokked the code to confirm this). I guess the ask here is to add some GitHub comment to the PR saying what are the steps to address this label, correct? Like, how to create the new API version and then to request the ARM feedback by adding WaitForARMfeedback label?

Additionally, when the NewApiVersionRequired label is added by automation, it should remove the WaitForARMFeedback label, add the ARMChangesRequested label,

If I understand correctly, the idea here is that as long as the NewApiVersionRequired label is in effect, there also has to be ARMChangesRequested label in effect. Basically the NewApiVersionRequired is a sub-type of all possible changes requested by ARM. Correct?

And you also don't want to see the WaitForARMFeedback label because it tells your folks to do manual review, but you don't want to waste reviewing the code until a new API version has been properly defined (or the PR submitter got an exemption, so that your reviewers know it is OK to review the existing API version).

and add a comment for the PR Author to remove the ARMChangesRequested label once they have created a new API version or obtained an exception from the Breaking Changes Review Board.

OK, so this is basically part of the instructions the PR submitter should follow when addressing the NewAPIVersionRequired. The instructions that should be made clear, as described above, in the first quote?

[bdefoy]

As a part of the automation, it should be made clear that the author needs to follow up with the NewAPIVersionRequired action label to create a new API version before moving on to the ARM API review. Only after the NewAPIVersionRequired label is addressed should the WaitForARMfeedback label be added, making the PR actionable for the ARM reviewers.

If I understand correctly, you are asking for it to be made clearer to the PR submitter what is the meaning of the NewAPIVersionRequired label, and relevant action items? I understand that label should only ever by added by our automation, not by hand. By the breaking change detectors we have, to be exact (I grokked the code to confirm this). I guess the ask here is to add some GitHub comment to the PR saying what are the steps to address this label, correct? Like, how to create the new API version and then to request the ARM feedback by adding WaitForARMfeedback label?

It looks like we already have a bot comment for this label. E.g., Azure/azure-rest-api-specs#24010 (comment). The comment mentions that a new API version should be added and why, but it should also tell the PR author that the ARM review will not continue until that is addressed. Also, yes, they should need to add the WaitForARMFeedback label remove the ARMChangesRequested label to continue the review. Adding this label will remove the ARMChangesRequested label. This requirement is already mentioned in a comment when the ARMChangesRequested label is added though. E.g., https://github.com/Azure/azure-rest-api-specs-pr/pull/12565#issuecomment-1550381726.

Additionally, when the NewApiVersionRequired label is added by automation, it should remove the WaitForARMFeedback label, add the ARMChangesRequested label,

If I understand correctly, the idea here is that as long as the NewApiVersionRequired label is in effect, there also has to be ARMChangesRequested label in effect. Basically the NewApiVersionRequired is a sub-type of all possible changes requested by ARM. Correct?

Yes, NewApiVersionRequired is a change requested by ARM requirement from the Breaking Changes Review Board, and when it is active on a PR we want the ARMChangesRequested label to be active as well.

And you also don't want to see the WaitForARMFeedback label because it tells your folks to do manual review, but you don't want to waste reviewing the code until a new API version has been properly defined (or the PR submitter got an exemption, so that your reviewers know it is OK to review the existing API version).

Exactly.

and add a comment for the PR Author to remove the ARMChangesRequested label once they have created a new API version or obtained an exception from the Breaking Changes Review Board.

OK, so this is basically part of the instructions the PR submitter should follow when addressing the NewAPIVersionRequired. The instructions that should be made clear, as described above, in the first quote?

Yes 👍

cc @rkmanda

[bdefoy]

@konrad-jamrozik I made some edits to my previous comment based on feedback from Roopesh. In summary, the bot should:

• Add the ARMChangesRequested label when it sees a NewApiVersionRequired label
• Once the required changes have been made (NewApiVersionRequired label gets removed) or an exception has been granted (I believe there is another label added for the exception), add the WaitForARMFeedback label

[konrad-jamrozik]

@bdefoy can you check my understanding of how things work below? It goes a bit out of scope of this issue, but I am trying to wrap around my head around the broader workflow.

Here are some scenarios, based on your guidance and on this diagram:

Scenario A, "ARM changes required, no new API version required", as currently implemented

Note the current implementation of scenario A is also how things should work.

1. A new PR is submitted.
2. Automation determines this is an ARM change, thus adding ARMReview label.
3. Automation determines there is no need for any new API version, so it does not add any NewApiVersionRequired label.
4. Automation adds the WaitForARMFeedback label.
5. ARM reviewer sees the WaitForARMFeedback label and thus starts a review. They leave comments and when done, manually add the label ARMChangesRequested.
6. Automation, upon seeing ARMChangesRequested, removes WaitForARMFeedback.
7. PR author sees the ARMChangesRequested label and thus starts addressing them. When done, they manually remove the label ARMChangesRequested.
8. Automation, upon seeing removal of ARMChangesRequested, adds the label WaitForARMFeedback.
9. ARM reviewer sees the WaitForARMFeedback label and thus starts a review. Here we either go back to step 5, or, if there are no further remarks, the ARM reviewer manually adds the label ARMSignedOff and manually removes the label WaitForARMFeedback.
   • Question: should we automate removal of the WaitForARMFeedback label here? I see merged PRs that have both ARMSignedOff and WaitForARMFeedback, see this query.
   • In fact, I also see merged PRs that have ARMSignedOff and ARMChangesRequested, see this query, which I think is bug in our bots.

Scenario B, "New ARM API version required", as it should work

1. A new PR is submitted.
2. Automation determines this is an ARM change, thus adding ARMReview label.
3. Automation determines there is a need for a new API version, so it adds a NewApiVersionRequired label.
4. Automation sees the NewApiVersionRequired label and thus adds ARMChangesRequested label.
   • As a result, we end up in a situation like in Scenario A step 7, but now in addition we also have NewApiVersionRequired label.
   • This is a new behavior, not currently implemented. This issue is about implementing it.
5. The PR author sees the NewApiVersionRequired and ARMChangesRequested labels and starts addressing them.
6. As a result of PR author pushing commits with changes in new API instead of existing API, the automation removes the NewApiVersionRequired label.
   • Important observation: even though NewApiVersionRequired label was removed, the label ApiChangesRequested is still present.
   • Alternatively to triggering the removal of NewApiVersionRequired label, the PR author may request a breaking-change-approval review. If approved by the breaking change reviewer, the reviewer manually adds the Approved-BreakingChange label.
7. PR author addresses the remaining changes requested by ARM, if any. When done, they manually remove the ApiChangesRequested label.
   • At this point the PR might either have both NewApiVersionRequired and Approved-BreakingChange labels on it, or none of them.
8. Automation, upon seeing removal of ARMChangesRequested, adds the label WaitForARMFeedback.
9. Here we effectively end up in Scenario A, step 5, but with the possibility of the extra labels being present as elaborated in step 7.

Scenario B, "New ARM API version required", as currently implemented

1. A new PR is submitted.
2. Automation determines this is an ARM change, thus adding ARMReview label.
3. Automation determines there is a need for a new API version, so it adds a NewApiVersionRequired label.
4. Automation adds the WaitForARMFeedback label.
   • This is the problem described by this issue. This label should not be added because NewApiVersionRequired label is present. Because of this, the steps 6-8 are necessary, but could be avoided.
5. The PR author sees the NewApiVersionRequired label and starts addressing it.
6. At the same time an ARM reviewer sees the WaitForARMFeedback label and starts the review, just to realize they should hold off until NewApiVersionRequired is removed.
7. The ARM reviewer manually adds the ARMChangesRequested Label.
8. Automation, upon seeing ARMChangesRequested, removes WaitForARMFeedback.
9. As a result of PR author pushing commits with changes in new API instead of existing API (because of step 5), the automation removes the NewApiVersionRequired label.
   • Alternatively to triggering the removal of NewApiVersionRequired label, the PR author may request a breaking-change-approval review. If approved by the breaking change reviewer, the reviewer manually adds the Approved-BreakingChange label.
10. Here we effectively end up in Scenario A, step 5, but with the possibility of the extra labels being present as elaborated in step 9.

Invariants (sanity checks)

There are some invariants that should always hold true. I am considering adding some of invariant checks in the source code, and emitting a warning if they are violated, as an additional method of verifying if the logic is correct.

These invariants are to hold after the automation processed all labelling updates after given set of commits has been pushed to a PR.

• NewApiVersionRequired label should be added only by automation.
• NewApiVersionRequired label should be removed only by automation, triggered upon new commit push to the PR that fixes the issue.
• WaitForARMFeedback label should be added only by automation.
  • Question: Are there legitimate cases where a human should do it?
• WaitForARMFeedback label should be removed only by automation.
• ARMChangesRequested label should be added only by an ARM reviewer.
• ARMChangesRequested label should be removed only by the PR author.
• If NewApiVersionRequired label is present, the label ARMChangesRequested must be also present.
• WaitForARMFeedback label cannot be present at the same time as NewApiVersionRequired label.
• WaitForARMFeedback label cannot be present at the same time as ARMChangesRequested label.
• If ARMSignedOff label is present, ARMChangesRequested and WaitForARMFeedback labels cannot be present.
• If ARMSignedOff label is present, then labels NewApiVersionRequired and Approved-BreakingChange are either present both at the same time, or not at all.
• If the PR pertains to ARM, the ARMReview label must be always present.
• If the ARMReview label is present, at any point of time, exactly one of these labels must be present: WaitForARMFeedback, ARMChangesRequested, ARMSignedOff.

[bdefoy]

@bdefoy can you check my understanding of how things work below? It goes a bit out of scope of this issue, but I am trying to wrap around my head around the broader workflow.

Here are some scenarios, based on your guidance and on this diagram:

Scenario A, "ARM changes required, no new API version required", as currently implemented

Note the current implementation of scenario A is also how things should work.

Yes, these steps are correct and working as intended in the workflow.

• Question: should we automate removal of the WaitForARMFeedback label here? I see merged PRs that have both ARMSignedOff and WaitForARMFeedback, see this query.

I believe it already is automated. The PRs you see in that query are exceptions, so there might be a bug in this part of the workflow. However, as I'm writing this, I only see 16 PRs with both ARMSignedOff and WaitForARMFeedback out of the 2,800+ PRs with ARMSignedOff in the public repo.

• In fact, I also see merged PRs that have ARMSignedOff and ARMChangesRequested, see this query, which I think is bug in our bots.

Agreed, but once again 14 of 2,800+, so a small number.

Scenario B, "New ARM API version required", as it should work

• As a result, we end up in a situation like in Scenario A step 7, but now in addition we also have NewApiVersionRequired label.
• This is a new behavior, not currently implemented. This issue is about implementing it.

👍

7. PR author addresses the remaining changes requested by ARM, if any. When done, they manually remove the ApiChangesRequested label.

   • At this point the PR might either have both NewApiVersionRequired and Approved-BreakingChange labels on it, or none of them.

All the steps here look right to me, but I am not sure about this one. Searching through the PRs, it does look like it is the case where both labels stay on the PR when the breaking change is approved. However, I'm not sure if that's intentional or not. I think it is fine if that is the way it works.

This also reminds me that there is another label, BreakingChangeReviewRequired. I'm not sure for which scenarios that label is added versus the NewApiVersionRequired label, but the bot's reply message is different. Might not be relevant here, but thought I'd mention it.

Scenario B, "New ARM API version required", as currently implemented

Yes, this seems accurate to what is currently happening. Also to note here, I believe it is more common the case that breaking changes are not approved. So, it's common that the PR author is publishing more commits to address the breaking changes while the WaitForARMFeedback label is active.

Invariants (sanity checks)

These all look accurate to me. The only exceptions to these I can think of are when there are either mistakes by people manually editing labels or bugs in the automation.

• NewApiVersionRequired label should be added only by automation.
• NewApiVersionRequired label should be removed only by automation, triggered upon new commit push to the PR that fixes the issue.
• WaitForARMFeedback label should be added only by automation.
  • Question: Are there legitimate cases where a human should do it?
• WaitForARMFeedback label should be removed only by automation.

The only exception for the above is if for some reason the automation fails to add/remove these labels, then some reviewer may want to manually add/remove them.

• ARMChangesRequested label should be added only by an ARM reviewer.
• ARMChangesRequested label should be removed only by the PR author.

The only exception for the above is if the ARM reviewer or the PR author make a mistake. So, both author and reviewer should be able to add or remove. E.g., the PR author might remove the label without first addressing the reviewer's comments.

• If NewApiVersionRequired label is present, the label ARMChangesRequested must be also present.
• WaitForARMFeedback label cannot be present at the same time as NewApiVersionRequired label.
• WaitForARMFeedback label cannot be present at the same time as ARMChangesRequested label.
• If ARMSignedOff label is present, ARMChangesRequested and WaitForARMFeedback labels cannot be present.

Above looks good.

• If ARMSignedOff label is present, then labels NewApiVersionRequired and Approved-BreakingChange are either present both at the same time, or not at all.

Yes to above, except for what I wrote earlier (not sure if both NewApiVersionRequired and Approved-BreakingChange are supposed to be present at the same time).

• If the PR pertains to ARM, the ARMReview label must be always present.

👍

• If the ARMReview label is present, at any point of time, exactly one of these labels must be present: WaitForARMFeedback, ARMChangesRequested, ARMSignedOff.

Above seems correct, but this is commonly not the case for PRs at the moment. Also, I think the check that adds ARMReview is different from the one that adds WaitForARMFeedback, because I have seen some PRs with only ARMReview.

[konrad-jamrozik]

@bdefoy @weshaggard

• Improve message for the NewApiVersionRequired label azure-rest-api-specs#24058

[konrad-jamrozik]

@bdefoy re BreakingChangeReviewRequired, I found this code snippet:

    // If exists both  "BreakingChangeReviewRequired" and "NewApiVersionRequired",remove the "NewApiVersionRequired"
    if (BreakingChangeLabels.every((l) => labelToEmit.includes(l))) {
      labelToEmit = labelToEmit.filter((l) => l !== "NewApiVersionRequired");
    }

The diagram says:

Update 5/23/2023

@bdefoy I believe we most likely want to treat BreakingChangeReviewRequired the same way as NewApiVersionRequired, i.e.: if it is present, it should remove WaitForARMFeedback, prevent it from re-appearing, and add ARMChangesRequested.

This is because:

• both labels NewApiVersionRequired and BreakingChangeReviewRequired are "staged to be added" by our breaking change detector tool based on BreakingChangeRules.yml
• if both these labels are "staged to be added", only BreakingChangeReviewRequired will be added, per the code snippet I linked to above.

[konrad-jamrozik]

@konrad-jamrozik Konrad Jamrozik FTE I made some edits to my previous comment based on feedback from Roopesh. In summary, the bot should:

• Add the ARMChangesRequested label when it sees a NewApiVersionRequired label
• Once the required changes have been made (NewApiVersionRequired label gets removed) or an exception has been granted (I believe there is another label added for the exception), add the WaitForARMFeedback label

@bdefoy I think we should add the following:

• These proposed rules apply only if the PR has ARMReview. I.e. they do not apply to data-plane PRs.
• BreakingChangeReviewRequired label should have the same treatment as NewApiVersionRequired label, per my 5/23/2023 update to this comment.
• If NewApiVersionRequired or BreakingChangeReviewRequired is present, in a PR with ARMReview, then we should ensure WaitForARMFeedback is not, unless Approved-BreakingChange is present.
  • By "ensure" I mean: don't add it if unnecessary, and remove it if somebody adds the label (we cannot prevent addition of a label, but we can remove it). I elaborated a bit on the challenge of doing the latter here.
• When it comes to adding WaitForARMFeedback label, it should happen through the existing rule which says the label gets added when the PR author manually removes the ARMChangesRequested label.

Do you agree with the above?

[konrad-jamrozik]

I grokked the code a bit more and I have a few interesting insights.

How adding WaitForARMFeedback label is currently implemented

Our code handling labelling logic after the PR CI checks run (i.e. the Summary task) says that the WaitForARMFeedback label will be added when all of the following conditions hold:

From processARMReview:

• the PR introduces a new API
• the PR is labeled resource-manager
  • this is determined by getPRType based on file paths edited by the PR
• the PR is not a draft
  • This requirement is causing problems. See:
    • [PR Workflow] Labels data-plane, new-api-version and typespec missing from a draft PR in azure-rest-api-specs-pr #6173
    • [PR Workflow] ARMReview label was not added in spite of the relevant checkbox being checked #6219

From isOkToAddWaitForARMFeedback:

• The PR has none of the following labels already: WaitForARMFeedback, ARMSignedOff, ARMChangesRequested
• The PR is not pending breaking change review. Where we say a PR is pending breaking change review when it has label BreakingChangeReviewRequired and does not have label Approved-BreakingChange

Given that the first condition says that the PR needs to introduce a new API for the WaitForARMFeedback label to be added, one may ask: then how is it possible to have both NewApiVersionRequired and WaitForARMFeedback at the same time? Currently, there are 7 PRs with both these labels, all closed

The answer is twofold:

• A PR might both have NewApiVersionRequired and be adding new APIs. Presumably this is when the PR is extensive, both making changes adding new APIs, as well as making breaking changes to existing API causing the label NewApiVersionRequired to be added. We can see an example of this happening in this build log for PR:
  • User/gpapiashvili/cluster migrate azure-rest-api-specs#23844
• And there is another way to add WaitForARMFeedback, which is by workflow-bot, when relevant checkmark is checked in the PR comment. The workflow-bot rule for this is in comment.yml, here. You can see it in action e.g. here, resulting in the PR having both NewApiVersionRequired and WaitForARMFeedback.

Proposed changes to address the problem described in this issue

• Eliminate the workflow-bot rule that adds ARMReview and WaitForARMFeedback labels when relevant checkmark is checked. Also eliminate the checkmark and its description altogether from the Pull Request template comment.
  • Instead, always add ARMReview label when a non-draft PR ends up getting the resource-manager label, as explained above.
    • Currently ARMReview is added only when the checkmark is checked OR if:
      • the PR is adding a new API AND is not a draft
• Adjust the logic mentioned above, in isOkToAddWaitForARMFeedback, to not add WaitForARMFeedback when NewApiVersionRequired is labels pending to be added.
  • Instead, make it add ARMChangesRequested

Important caveat to the above is that if the logic sees both NewApiVersionRequired and BreakingChangeReviewRequired labels pending addition, then it will skip adding the NewApiVersionRequired label, per this logic in processBreakingChangeLabels. This needs to be taken into account when implementing the behavior change.

[konrad-jamrozik]

openapi-alps PR 474083: Prevent isOkToAddWaitForARMFeedback from adding WaitForARMFeedback label if NewApiVersionRequired label is present

@weshaggard @rkmanda @bdefoy please have a look at the PR, thx! :)

[konrad-jamrozik]

The change has been deployed to production. Details here.