[Breaking Change][PR Workflow] REST API Breaking Changes PR check should check who added the suppression label

[mikekistler]

PRs in the azure-rest-api-specs repo are blocked from merging if the Breaking Changes PR check reports a failure. But the result of the Breaking Changes check is squashed if the "Approved-BreakingChange" label has been added to the PR. That label is supposed to signify that the PR was reviewed by the Breaking Change review board and determined to be an allowable change.

Unfortunately, there is no means to prevent any user with labelling permissions to add the "Approved-BreakingChange" label, so changes that should not be permitted are allowed to merge, resulting in customer disruption.

An example is PR 23647.

Since we cannot restrict who can add the label, I think the PR check must only be squashed if the label was added by a member of the breaking change review board or otherwise trained and responsible user.

[live1206]

@mikekistler Sorry about the misuse of Approved-BreakingChange label for the reverting public case. I am wondering if Approved-BreakingChange should only be added by the breaking change review board? So that PR assignee will not touch this. There is another case that, the breaking change has been approved in private repo. And the exact same change got pushed to public main, theoretically the breaking change doesn't need to be reviewed again. Should Approved-BreakingChange be added by the breaking change review board not the PR assignee?

[mikekistler]

You are right that some cases can reasonably be handled outside the breaking change review. I think we need to clearly document the process and ensure all those with labelling permissions understand it.

[live1206]

You are right that some cases can reasonably be handled outside the breaking change review. I think we need to clearly document the process and ensure all those with labelling permissions understand it.

That would be very helpful! And it would clarify the potential misusages.

[konrad-jamrozik]

Per the triage meeting, we need to figure out a general solution how to restrict which "approve" and "override" labels can be added by whom. This is going to be a bit more work.

[mikekistler]

Just to be clear, there is no mechanism in GitHub (as best I know) that allows permissions on individual labels. Anyone with labeling permissions -- which is many people -- can add any label. So what we need to do is condition our pipeline behavior not just on the presence of a label but also who added it.

[live1206]

You are right that some cases can reasonably be handled outside the breaking change review. I think we need to clearly document the process and ensure all those with labelling permissions understand it.

@mikekistler Just to be clear, do you mean we will not add label BreakingChange-Review label to the PR for cases to be handled outside the breaking change review? For instance, the tooling will compare the approved PR in private repo and the change in public repo, if they are the same breaking change review will not be flagged.

Because this request is about Approved-BreakingChange label should take effect only if it is added by the breaking change review board member.

[mikekistler]

There are many many changes, improvements, and clarifications needed in our breaking change review process. This issue is only intended to address the very narrow issue that we want to limit who can control the PR pipeline by adding labels. We can't actually control who can add the labels -- GitHub does not give us that capability. So instead we should consider who added the label when determining whether it should affect the pipeline behavior.

[live1206]

So instead we should consider who added the label when determining whether it should affect the pipeline behavior.

Understood, that's exactly what I meant if Approved-BreakingChange only take effect for the pipeline behavior added by breaking change review board member, the PR assignee should not add it no matter case it is. Then, the case we talked about above, exact change approved in private repo pushed to public repo should be approved by the breaking change review board as well.

[mikekistler]

There may be valid cases for the PR assignee to add the label. We need to define the process. Once the process is defined, we'll ensure that it is communicated and understood by anyone that authorized to add the label. This issue is just to create the mechanism to limit the effect of adding the label to those identified by the process as authorized to do so.