Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[suppressions.yaml] Add PR label when edited #28756

Open
mikeharder opened this issue Apr 19, 2024 · 2 comments
Open

[suppressions.yaml] Add PR label when edited #28756

mikeharder opened this issue Apr 19, 2024 · 2 comments
Assignees
Labels
Central-EngSys This issue is owned by the Engineering System team. Spec PR Tools Tooling that runs in azure-rest-api-specs repo.

Comments

@mikeharder
Copy link
Member

When a PR includes edits to any file named suppressions.yaml, add a label that blocks merging (and an associated label to unblock merging). Options:

  • SuppressionReviewRequired / Approved-Suppression
    • Existing labels shared by all suppressions
  • SuppressionsYamlReviewRequired
    • New label for all changes to suppressions.yaml
  • TypesSpecReviewRequired
    • New label specific to TypeSpecRequirement
@konrad-jamrozik
Copy link

@konrad-jamrozik
Copy link

konrad-jamrozik commented Apr 19, 2024

@mikeharder

I think we should make the label as specific as possible, and also at least try to align with existing naming conventions. I just wrote down all the complexities of suppressions we have here:

I think we could adopt a convention similar to the sdk-suppresions.yaml labels

For example, they have BreakingChange-Go-Sdk-Suppression and BreakingChange-Go-Sdk-Suppression-Approved.

Given we have:

- tool: TypeSpecRequirement

We could do:

  • Tool-TypeSpecRequirement-Suppression
  • Tool-TypeSpecRequirement-Suppression-Approved.

Later on, if we do e.g. - tool: BreakingChangeCrossVersion we would do:

  • Tool-BreakingChangeCrossVersion-Suppression
  • Tool-BreakingChangeCrossVersion-Suppression-Approved

I am suggesting to prefix this with Tool- to distinguish it from the two different kinds of suppressions: the AutoRest README.md suppressions and sdk-suppressions.yaml. See Azure/azure-sdk-tools#8133 for details.

We probably also should rename SuppresionReviewRequired to match the new convention. I filed an issue for this:


Note we also need to integrate any newly added suppression labels into the Next Steps to Merge comment. This can be done in a similar way SuppressionReviewRequired is currently handled. I provided source pointers to it in this issue.

This relates to:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Central-EngSys This issue is owned by the Engineering System team. Spec PR Tools Tooling that runs in azure-rest-api-specs repo.
Projects
Status: 📋 Backlog
Status: 📋 Backlog
Development

No branches or pull requests

2 participants