fix: Exclude content from codeowner #108
Conversation
Only have codeownership on all the other parts but allow any dev to approve content changes
There was a problem hiding this comment.
Do we have separate branch protection rules set up on this repo? If not, wouldn't this change enable any external user to push content without our approval, including spam?
Also, I thought that the formation of a contributor-docs admin group was being discussed to be assigned as codeowner for this repo? I may not be aware of recent updates on this matter if they have taken place since the last engineering technical sync.
| !docs/** | ||
| !examples/** |
There was a problem hiding this comment.
The natural minimum protection measure would be to add @MetaMask/engineering as a codeowner so that only internal engineers can approve a PR, but I understand that this is not desirable due to the excessive notifications it would send out.
| !docs/** | |
| !examples/** | |
| docs/** @MetaMask/engineering | |
| examples/** @MetaMask/engineering |
|
Hi @NicolasMassart We did have a conversation about this during the Technical Engineering Sync on Sept 10, 2024 and I spoke with @Gudahtt before he left. We'd like to in the very least bump the number of required approvers up to 2 and we'd like to put something in place that posts when updates happen to this repo such that everyone is notified of a potential change. These documents are meant to be guidelines for the whole org so having clear visibility and broader discussion on changes is ideal. I am not saying this PR can't go in but I am asking that we address the need for broader visibility into changes on this repo. Maybe for now we can update the PR template checklist to include an item to post the PR in the dev channel for broader visibility? |
|
@desi and @MajorLift, many discussions in different places led to this proposal not being up to date with the latest status on this matter. Thanks for the reminder. The initial idea (discussed among mobile team during our retro if I remember correctly), was to have framework team be responsible for the repo and all the CI and non content parts, but allow engineers to agree on content change without the need of the framework team approval. I will close this as it misses the point, sorry for that. |
It would not; the "required reviews" branch protection setting is separate from the code owner branch protection setting. A review would still be required from someone with write access, which would essentially be the engineering team. We won't lose branch protections for files uncovered by code owners. |
| @@ -2,3 +2,5 @@ | |||
| # Each line is a file pattern followed by one or more owners. | |||
|
|
|||
| * @MetaMask/wallet-framework-engineers | |||
| !docs/** | |||
There was a problem hiding this comment.
Negation does not work in this file: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners#codeowners-syntax
Curious that GitHub still shows the syntax as valid 🤔 . That's a bit misleading. Ah well.
Only have codeownership on all the other parts but allow any dev to approve content changes