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

chore: Add entire team as code owners for the changelog and Cargo.lock #2655

Merged
merged 2 commits into from
Jan 31, 2025

Conversation

netrome
Copy link
Contributor

@netrome netrome commented Jan 31, 2025

Because virtually every PR may change these, and anyone in the team should be able to vet changes to these files.

@netrome netrome added the no changelog Skip the CI check of the changelog modification label Jan 31, 2025
@netrome netrome self-assigned this Jan 31, 2025
@netrome netrome requested a review from a team January 31, 2025 14:07
acerone85
acerone85 previously approved these changes Jan 31, 2025
Copy link
Contributor

@AurelienFT AurelienFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of we do that I believe we will all be codeowners on all PRs and our approval will count as a code owner approval on the whole code just because the Changelog has been touched. Is it something we want ?

@netrome
Copy link
Contributor Author

netrome commented Jan 31, 2025

Of we do that I believe we will all be codeowners on all PRs and our approval will count as a code owner approval on the whole code just because the Changelog has been touched. Is it something we want ?

Nope that's not how code owners work. For each file with a code owner touched by a PR, you need an approval from a code owner of that file. This is how it's said in the docs:

Optionally, you can choose to require reviews from code owners. If you do, any pull request that affects code with a code owner must be approved by that code owner before the pull request can be merged into the protected branch.

See: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-pull-request-reviews-before-merging

Also see this PR: #2598
I have codeowner approval for all fraud proof stuff, but it still requires approval from one of the oldies because of the changelog and Cargo.lock.

@netrome netrome dismissed AurelienFT’s stale review January 31, 2025 15:09

Appreciate the pushback, but the request is based on incorrect assumptions.

@netrome netrome requested a review from AurelienFT January 31, 2025 15:26
Copy link
Contributor

@AurelienFT AurelienFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I believe you then, my bad. I just saw that when you look at files changed it says that "@acerone" doesn't exist I believe it should be "@acerone85"

@netrome
Copy link
Contributor Author

netrome commented Jan 31, 2025

Ok I believe you then, my bad. I just saw that when you look at files changed it says that "@acerone" doesn't exist I believe it should be "@acerone85"

Oh thanks for spotting, fixed now 🙏

@netrome netrome requested a review from AurelienFT January 31, 2025 15:54
@netrome netrome dismissed AurelienFT’s stale review January 31, 2025 16:23

Addressed the requested change.

@netrome netrome enabled auto-merge (squash) January 31, 2025 16:23
@netrome netrome force-pushed the chore/codeowner-add-entire-team-for-trivial-parts branch from 5e5b214 to e5d764c Compare January 31, 2025 16:36
@netrome netrome merged commit c9ff37f into master Jan 31, 2025
31 of 34 checks passed
@netrome netrome deleted the chore/codeowner-add-entire-team-for-trivial-parts branch January 31, 2025 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Skip the CI check of the changelog modification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants