Same-version breaking change check added BreakingChangeReviewRequired instead of NewApiVersionRequired

[konrad-jamrozik]

• https://github.com/Azure/azure-rest-api-specs-pr/pull/16984
• https://dev.azure.com/azure-sdk/internal/_build/results?buildId=3477365&view=results
• Teams post

I suspect it may be because of the BreakingChangeRules.yml entry that instructed the automation to add BreakingChangeReviewRequired.

This bug likely was introduced while working on:

• [Breaking Change][PR Workflow] Use more granular labels for Breaking Changes approvals #6374

Specifically:

• Pull Request 524748

[konrad-jamrozik]

Looks like in this specific case the label BreakingChangeReviewRequired was added manually by @mentat9:

• https://github.com/Azure/azure-rest-api-specs-pr/pull/16984#event-11751767391

Unclear yet why the automation didn't do it. I will investigate.

This bug report let me discover a case that I didn't consider before: if the "same version breaking changes" check (Swagger BreakingChange) reports a failure, it may report a BreakingChangeReviewRequired instead of VersioningReviewRequired. Example rule definition showing this: SameVersion / RemovePath.

With the new labels human reviewers need to remember to apply appropriate "review required" label and then, use appropriate, matching "approved" label, if applicable. Specifically:

BreakingChangeReviewRequired -> BreakingChange-Approved-*
VersioningChangeReviewRequired -> Versioning-Approved-*

Details at:

• [Breaking Change][PR Workflow] Use more granular labels for Breaking Changes approvals #6374
• https://gist.github.com/mikekistler/5a35e1ca7d69a52b4e55c25d805fe5d1

In addition, perhaps we should denote any kind of breaking changes in same-version breaking changes as VersioningReviewRequired. This nuance escaped me because previous the label was named NewApiVersionRequied. With the new name the meaning has changed.

[konrad-jamrozik]

We decided to go with proposal B from this gist

This is captured by this PR:
Pull Request 528285: Make automation add VersioningReviewRequired instead of NewApiVersionRequired. Assorted refactoring and new breaking changes types for labels.

I have a "confusion matrix" diagram elaborating on the new behavior here:

• Same-version API check (Swagger BreakingChange) should add VersioningReviewRequired instead of BreakingChangeReviewRequired when detecting issues on preview versions (as opposed to GA) (breaking changes confusion matrix) #7724

Related work:

• Pull Request 529441: Bugfix: the new BreakingChange-Approval-* labels now will be recognized by automation managing ARM review
• Pull Request 528285: Make automation add VersioningReviewRequired instead of NewApiVersionRequired. Assorted refactoring and new breaking changes types for labels.

[konrad-jamrozik]

This will be fixed when this is fixed:

• Same-version API check (Swagger BreakingChange) should add VersioningReviewRequired instead of BreakingChangeReviewRequired when detecting issues on preview versions (as opposed to GA) (breaking changes confusion matrix) #7724