Block ARM review approval (label ARMSignedOff) if suppressions have not been addressed (related to "suppression review queue")

[konrad-jamrozik]

Example affected PR:

• Azure VMware Solution 2023-09-01 in TypeSpec azure-rest-api-specs#28023

Relevant email thread:

From: Cameron Taggart
Sent: Wednesday, March 13, 2024 8:46 AM
To: Azure SDK Engineering System Team
Subject: How long to wait for suppression review?

The core issue is that the PR was waiting for a long time for suppression review and it was unclear to the PR author when it will happen. The PR author proposed "suppression review queue" similar to ARM review queue. However, the relevant suppressions should have been reviewed as part of ARM review. Hence there should be no need need for a separate suppression review queue.

Per Roopesh Manda:

Ideally all suppressions [related to ARM review] must be reviewed before the PR is signed off by the ARM reviewer. The SuppressionReviewRequired label does not impact the visibility of the PR in the review queue. The workflow diagram does not call out the suppression review requirement explicitly for this reason.

We could also explore making a change to not allow an ARMSignedOff label to be placed if the SuppressionReviewRequired label is present but the Approved-Suppression label is not present to streamline this.

Unfortunately, there is no easy way to prevent people from adding specific label. Our collaboration model assumes folks have permissions to modify labels, meaning they can add or remove any label. We could make the automation immediately remove the ARMSignedOff label after it has been added, but I believe it would be confusing to the person who added the label plus we would risk them not even noticing the label was removed.

This issue depends on:

• [PR Workflow] The SuppressionReviewRequired label should be split into subcategories #6622

This is because we currently SuppressionReviewRequired is overloaded: it means multiple different kinds of suppressions, including SDK generation suppressions, i.e. step 3 in the PR diagram. We want to add special handling only for the suppressions that interfere with the ARM review, which means suppressions of LintDiff.

[konrad-jamrozik]

@rkmanda as you can see from the description, this is not an easy problem to fix by automation. We are trying to provide some kind of safety in case the human ARM reviewer does incomplete review i.e. they forget to review suppressions.

[cataggar]

For me, documentation that stated that you need to request ARM Review for suppressions would be sufficient. That just isn’t clear from the diagram and other docs.