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 CODEOWNERS for i18n reviews #2710

Merged
merged 2 commits into from
Nov 8, 2019
Merged

Conversation

nschonni
Copy link
Member

@nschonni nschonni commented Oct 22, 2019

This was dictated from the docs https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners but I haven't really seen the @repo/team format work very well in other places, so maybe someone that's used this properly should take a look

@nschonni
Copy link
Member Author

PS: noticed that we have Catalan and Galician translations in here, but there aren't a corrisponding i18n team in the org

@nschonni
Copy link
Member Author

Oh, I'm wondering if the teams used by the file might need to be given permission to the repo (even if it's reader), since in the reviewer dropdown you can find the website team, but not any other teams

@richardlau
Copy link
Member

Oh, I'm wondering if the teams used by the file might need to be given permission to the repo (even if it's reader), since in the reviewer dropdown you can find the website team, but not any other teams

That was certainly true when we experimented in the core repo: nodejs/node#20554 (comment) (we ultimately removed it in nodejs/node#21161)

@nschonni
Copy link
Member Author

Hmm, if protected branches is enabled here, giving write access shouldn't really matter (in this repo). Unless it was a different level of "write" access required

@nschonni
Copy link
Member Author

@Trott would adding the i18n teams as writers to this repo require a TSC approval?

@XhmikosR
Copy link
Contributor

XhmikosR commented Oct 25, 2019

Protected branches seems enabled but there's not a restriction in approvals AFAICT. It only requires CI to pass and branches to be up to date.

@Trott
Copy link
Member

Trott commented Oct 25, 2019

@Trott would adding the i18n teams as writers to this repo require a TSC approval?

I think if the website team broadly thinks it's a good idea and the i18n folks don't mind, then that should be OK. I'd notify TSC and CommComm for information purposes but I don't think there'd be any pushback. The number of people who have write permissions here is already pretty large.

@Trott
Copy link
Member

Trott commented Nov 8, 2019

Maybe we'll land this and wait a few days or whatever to see if it works. If it doesn't quite work (which I expect), then we can move forward with plan "let's see if we can add the individual i18n groups to the repo".

@Trott Trott merged commit 839f64f into nodejs:master Nov 8, 2019
@nschonni nschonni deleted the add-codeowners branch November 8, 2019 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants