[Breaking Change] Breaking change presence should not always result in a required review

[konrad-jamrozik]

As currently written here:
https://dev.azure.com/azure-sdk/internal/_wiki/wikis/internal.wiki/1009/Breaking-Change-Detection-Rules

This document will describe which conditions detected by the breaking change tooling should be flagged for breaking change review.

The following SHOULD be labeled for review:

• Any "error" condition relative to a prior GA version.

The following SHOULD NOT be labeled for review:

• Any condition that is not relative to a prior GA version.
• Any PR that is not to a "production" branch -- "main" in the public repo or "RPSaaSMaster" in the private repo.

Specifically the problem is that right now, if breaking change check detects an issue, it adds a label BreakingChangeReviewRequiredor NewApiVersionRequired. In addition, the check is required in GitHub.

But going forward we don't want that: sometimes there is a breaking change, but it is OK, hence no need for official breaking change review. We need to figure out how to surface / communicate it.

Note that sometimes the check fails to add the labels. If so, this is most likely #6169.

There is also ongoing discussion about "self-approval" in the email thread Subject: FW: Need help in a false positive breaking change check in a API Spec PR.

Related email threads:

• Subject: Re: Adding new POST action to swagger is causing BreakingChange check to fail.

  This check does not apply to private previews. But our tooling flags it because it has no way to
  know when a preview API version is in private preview or public preview.
  We are planning an improvement to the process that will allow a service team to “self certify”
  that the API version is in private preview and thus “auto approve” the change.

• Subject: RE: breaking changes detected in PR

• Subject: FW: Need help in a false positive breaking change check in a API Spec PR

• Subject: RE: Please approve the breaking changes of HDInsight private preview service

Related work:

• [Breaking Change] Bypass review of BreakingChange/NewApiVersionRequired for PRs to dev branches #5838
• [Breaking Change][PR Workflow] Use more granular labels for Breaking Changes approvals #6374

[konrad-jamrozik]

openapi-alps PR 513409: Do not add BreakingChangeReviewRequired or NewApiVersionRequired labels if the target branch is not a production branch.

The PR has been deployed to production today at 10:25 AM PST.

Closing this issue as completed.

[konrad-jamrozik]

Reopening the issue as I realized there is one complexity to consider.

While the BreakingChangeReviewRequired and NewApiVersionRequired labels will no longer be added where they shouldn't, the "breaking change" checks are still required on the repos GitHub branch policies, so they will block PRs from being merged per getRequiredAndAutomatedMergingRequirementsMetCheckRuns, and hence will require Approved-BreakingChange label as explained in https://aka.ms/azsdk/pr-suppressions. Not every spec author has permissions to add such labels. Albeit perhaps we may want to give them such option, as we plan to give PR authors more leeway anyway, per #7354. They could possibly do it the way they can add MergeRequested label, as implemented in this PR.

Alternatively, I could modify the implementation so that if the relevant labels are not added, relevant required checks do not fail in the first place, but e.g. end up neutral. But I don't like it, as I believe this would be a confusing messaging.

I think I like the solution in which folks can add a comment that will trigger adding the Approved-BreakingChange label. However, there are two things to consider here:

• This could lead to misuse, where PR authors add this label even on PRs that require breaking change board review. To avoid this we could make the automation run some additional verification after the PR author adds a comment requesting the label but before actually adding the label. However, if we do it like so, we won't be able to use the GitOps policies. We will have to e.g. hook into the openapi-alps logic.
• We need to somehow surface this information to the PR authors, explaining how and when they can add a comment requesting adding the Approved-BreakingChange label.

@weshaggard @mikeharder @mikekistler @JeffreyRichter @heaths thoughts?

[konrad-jamrozik]

I had a conversation with @weshaggard. The way we want to approach it is to make the breaking change checks be no longer required on GitHub on all the branches except these two that require breaking change board review. As a result, there won't be any need to add Approved-BreakingChange label anywhere. As Wes said, having people add Approved-BreakingChange would teach the PR authors bad habits.

We are also considering stopping the breaking change checks from even running on all the branches except these two that require breaking change board review. This means any PRs to dev branches will no longer provide any information about potential breaking changes, but perhaps this information is not that useful anyway.

[heaths]

If GitHub's required checks feature isn't expressive enough, we do have that "Required validation" bot that could treat it as required - the one that lists checks / other issues that need to be addressed. That bot - as a check? - is required itself, so if it's not happy the PR can't be merged. Might be easier to keep all the "what's required" stuff centralized via that bot; though, GitHub's required checks icon is pretty nice. That said, in my experience, not a lot of spec authors seem to pay attention to it anyway. I often, as a review, have to point out the failures before it can be approved/merged.

[heaths]

It might also be a good time to check out the new ruleset feature I mentioned to @weshaggard. These are a superset of branch rules, and you can define multiple matching branch names / patterns. It doesn't solve the problem as much as makes it easier to apply the same branch rules to numerous branches, so you could easily have a "production" ruleset (or whatever you want to call it) and a "development" ruleset that doesn't require the breaking change build.

I've used it in one or two repos if I want me to give a quick tour / comparison.

[konrad-jamrozik]

Per another chat I had with @weshaggard I executed on this plan. Audit log of my changes to GitHub branch protection rules:

https://dev.azure.com/azure-sdk/internal/_wiki/wikis/internal.wiki/1029/Changelog-GitHub-branch-protection-rules

This closes this work item.