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

add GitHub CODEOWNERS using the existing PR review teams #1515

Merged
merged 1 commit into from
May 6, 2019

Conversation

das-g
Copy link
Member

@das-g das-g commented Apr 30, 2019

Add GitHub CODEOWNERS using the existing PR review teams from https://github.com/orgs/DjangoGirls/teams (an idea brought up by #1271 (comment))

This will automatically request reviews of the respective teams for pull requests touching "their" files, but won't require these reviews for merging.

@das-g das-g requested review from a team April 30, 2019 23:25
@magul
Copy link
Member

magul commented May 6, 2019

May I suggest here to use the same language code for a resource prefixes and translators group on GitHub? We right now have /zh/ as a prefix and cn as group distinction.

What do You think about that @aniav, @RachellCalhoun?

@ekohl
Copy link
Collaborator

ekohl commented May 6, 2019

One downside of this is that you can't merge anything unless there's an ack from the translation team. Sometimes there are trivial changes and it would be blocked. There is https://probot.github.io/apps/auto-assign/ but I have no experience with it.

@das-g
Copy link
Member Author

das-g commented May 6, 2019

May I suggest here to use the same language code for a resource prefixes and translators group on GitHub? We right now have /zh/ as a prefix and cn as group distinction.

The group name is probably due to a confusion, as CN isn't an ISO 639 language code at all. (It is the 3166-2 country code for the People's Republic of China.)

The ISO 639-1 (=two-letter) language code for the group of languages known as "Chinese" is indeed zh. So I guess we should indeed rename that GitHub team.

I've filed a separate issue to discuss this further: #1517

@das-g
Copy link
Member Author

das-g commented May 6, 2019

One downside of this is that you can't merge anything unless there's an ack from the translation team. Sometimes there are trivial changes and it would be blocked.

If I understood the documentation correctly, particularly

When someone with admin or owner permissions has enabled required reviews, they also can optionally require approval from a code owner before the author can merge a pull request in the repository.

I'd say it isn't the case that by default, merges are blocked as long as no "code owner" has approved the PR. So I think users with write permission to the target repo would still be able to merge, even without an approval.

If I understand the quoted documentation part correctly, there would be a two-step opt-in for having a blocking code owner approval requirement:

  • Option "Require pull request reviews before merging" would have to be enabled (it doesn't seem to currently be enabled for DjangoGirls/tutorial)
  • Option "Require review from Code Owners" would have to be enabled (only available when the first option is enabled)

(See also steps 5 and 8 of https://help.github.com/en/articles/enabling-required-reviews-for-pull-requests)

Thus, adding the CODEOWNERS file should automatically request reviews by the respective code owners, but not require approval by them for merging.

Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

In that case let's try this and see if it helps. We can always revert.

@das-g das-g merged commit 743646e into DjangoGirls:master May 6, 2019
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