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

initial CODEOWNERS file #68

Merged
merged 2 commits into from
Nov 9, 2020
Merged

initial CODEOWNERS file #68

merged 2 commits into from
Nov 9, 2020

Conversation

escattone
Copy link
Contributor

@escattone escattone commented Oct 30, 2020

This is an initial step towards mdn/yari#1098. It shows what we can do with a CODEOWNERS file. We can build on this going forward.

By the way, for this repo I've enabled the Require review from Code Owners feature, which will kick-in once we merge this PR.

@escattone escattone requested a review from ddbeck October 30, 2020 22:31
Copy link
Contributor

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

My first concern is that I don't know what it means when it says @peterbe @escattone. Does it mean, it triggers 2 review requests?
Whenever that happens, I'm always confused but I assume it means "This PR needs a review from me AND the other names listed"
The remedy to that is that the review request goes to a GitHub team instead. E.g. core-yari-dev
which means that the review request only needs a review from 1 person in the team.

(by the way, I sent an invite to @fiji-flo to that team)

What I'd prefer is that we set up 1 new team for all the content. It'll just have Chris a first and single member today, but it feels more right.

I'm not sure how to review this PR itself. Part of me wants to take the idea of teams seriously rather than a list of usernames. But we also have to "just do it" and learn as we go instead of waiting for perfection.
But I really do think we should help readers of this file to explain what it does. At least an executive summary and/or a link to the documentation.

.github/CODEOWNERS Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
.github/CODEOWNERS Show resolved Hide resolved
Copy link
Contributor

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

Peter's suggestion of using GitHub teams is probably a good one. We use individual users on BCD and it's a little fussy when someone can't or doesn't want to review something (they have to individually opt out).

One concrete suggestion in a line comment. Thanks!

.github/CODEOWNERS Outdated Show resolved Hide resolved
@escattone
Copy link
Contributor Author

escattone commented Nov 4, 2020

Thanks for the review @peterbe and @ddbeck! Here's what I did based on your feedback:

  • We have two new teams: @mdn/core-yari-dev and @mdn/core-yari-content. Both of these teams have been given write access to this repository (the team itself has to be given at least write access) and also had GitHub's code-review assignment feature enabled, so only 1 member of the team will be selected/notified each time for a review (when the team is requested), and the selection process will be "load-balanced" so no one member is overwhelmed with review requests.
  • I used each of these two teams within the CODEOWNERS file instead of individuals.
  • I changed @TBD to simply TBD for the commented-out example matches.

Copy link
Contributor

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you, @escattone!

@escattone
Copy link
Contributor Author

Just FYI, I spoke with @chrisdavidmills about this during our meeting this morning, and he's OK to start with this as well.

Copy link
Contributor

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

I must admit, I struggle to fully form an opinion on this.
If all it does is create some default review requests, then the only sane thing might be to just get started.

Also, it seems we don't need to nail this perfectly as we'll still be able to use our existing access rights to merge whatever needs to be merged anyway.

@escattone
Copy link
Contributor Author

Let's get this started, so I'm going to merge this. We can iterate from here.

@escattone escattone merged commit 323fd96 into mdn:main Nov 9, 2020
@escattone escattone deleted the initial-codeowners-1098 branch November 9, 2020 21:12
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants