-
Notifications
You must be signed in to change notification settings - Fork 284
chore(CODEOWNERS): Remove unused teams and add cloud-samples-infra #742
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
chore(CODEOWNERS): Remove unused teams and add cloud-samples-infra #742
Conversation
/media_cdn/ @terraform-google-modules/dee-infra @terraform-google-modules/terraform-samples-reviewers | ||
/network_connectivity/ @terraform-google-modules/dee-infra @terraform-google-modules/terraform-samples-reviewers | ||
/privateca/ @terraform-google-modules/dee-infra @terraform-google-modules/terraform-samples-reviewers | ||
/run/ @terraform-google-modules/torus-dpe @terraform-google-modules/terraform-samples-reviewers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these being removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
torus-dpe team is no longer in use and being removed from repositories. Once torus-dpe is removed from this row, the value of keeping this rule as an override seems missing. Is there a need to keep something in place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see; I believe the intent here is that samples-reviews from the top level will catch this.
I think this isn't entirely best practice, and not something we should be suggesting others do, but given this isn't one of our Big Four, I think it's fine?
(Strictly speaking samples should never be added without expert reviewers, but the fact that in this specific case the expert reviewers moved teams to the superteam, I think it's fine)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarifying, the big four have this, and the intent is that cloud-samples-infra is able to approve things like changes to README, Makefile, and CONTRIBUTING without having authority over the samples themselves. This PR should only have removed unused CODEOWNERS of samples, not moved authority around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added cloud-samples-infra to write permissions, some pending comments
Co-authored-by: Katie McLaughlin <[email protected]>
Assigning to @glasnt, to check and merge this PR. |
Description
Fixes b/368475360 and cleans up unused teams/redundant entries.
As part of this, there's a new row for
/*
, this is a way to give CODEOWNERS authority to root files without the entire repo: the cloud-samples-infra team is not scoped for reviewing terraform sample changes.GitHub is now marking this change as invalid because the new team needs to be added as a repo admin. Requesting a reviewer from terraform-samples-git-admins take care of that.
Note: If you are not associated with Google, open an issue for discussion before submitting a pull request.
Checklist
Readiness
Style
guide
Testing
I have performed tests described in the Contributing guide:
terraform apply
terraform fmt
checkIntended location
Yes, this sample will be (or already is) included on cloud.google.com
Location(s):
No, this sample won't be included on cloud.google.com
Reason:
API enablement
Review