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

Workflow: Add repository CODEOWNERS file #13604

Merged
merged 7 commits into from Feb 4, 2019

Conversation

Projects
None yet
10 participants
@aduth
Copy link
Member

aduth commented Jan 30, 2019

Related: #13441

This pull request seeks to add an initial implementation of a CODEOWNERS file to help notify interested parties of changes to specific areas of the codebase, and in an effort toward alleviating pressure for reviews to a concentrated few.

Implementation notes:

The formatting options for this file are rather limiting, inheriting the .gitignore pattern format. I experimented with a few options here, either inlining several paths in the same line, or including wildcards for things like Babel plugins (always babel-*), Webpack plugins (always *-webpack-plugin), etc. As proposed here I find to be a nice compromise, though is open to future iterations.

The column alignment was hand-crafted, so could become a maintenance burden, but the expectation is that new path entries to this file may not be quite as frequent as adding/removing individuals from existing entries. The hope here is that it improves readability by providing a visual distinction between paths and owners. I can imagine some automation here in the future (see below, e.g. text-table).

In implementing this, I found some interesting usage from other projects that could prove useful for future iterations, such as DefinitelyTyped's auto-generation of the file from package definition manifest headers.

The syntax documentation notes that owners are chosen by last-match, not all-matches. Thus, @WordPress/gutenberg-core is included for each additional entry, in addition to the wildcard fallback.

To that point, it should be noted that this will definitely increase notification noise for those on the Gutenberg Core team, as they will now receive notifications for every single pull request which is opened. The alternative is to change the implementation here to strictly an opt-in on an individual basis (or creation of new sub-teams).

Coverage for all packages was verified using a small script I put together. We may want to reconsider organization of the specific packages, or abandon organization altogether in favor of a simple listing of packages (which could be more suitable to individuals hand-picking individual packages, rather than lumped into a suite, e.g. "UI Components").

@nerrad
Copy link
Contributor

nerrad left a comment

Personally, I think it'd be be better to add teams here as opposed to individuals. I think teams will be easier to maintain by anyone with admin access in the org and there will be a clear record for admins of the org regarding who's on a team.

Using teams leads to less churn in this file.

Plus organizing people on teams could have potential future benefits for organizational work in the org (potential automations etc using the github api).

@nerrad

This comment has been minimized.

Copy link
Contributor

nerrad commented Jan 30, 2019

Another advantage to using teams as opposed to individuals in this file is when the reviews are automatically requested/suggested in github pulls, the team is listed instead of the individual, thus giving some visible insight into what "component" the pull is relative to as well.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Jan 30, 2019

Personally, I think it'd be be better to add teams here as opposed to individuals. I think teams will be easier to maintain by anyone with admin access in the org and there will be a clear record for admins of the org regarding who's on a team.

In compiling this file, I found myself to be not as confident that teams make more sense than listing individuals. From #13441, there's a number of people who have expressed interest in very specific areas of code, and I think this is something we should want to anticipate and embrace. It's easy for a person to commit to supporting the i18n package, and much less appealing if they'd consider themselves opting in to receiving notifications for every package falling under "Utilities". Even those who expressed interest in a thing like "UI Components" may not have anticipated that they'd volunteered to support changes to the notices or element modules.

Teams do have some advantage. From a maintainer's perspective, it's much less work to add someone to a team than it is to propose a change to this file. On the other hand, anyone can propose to add themselves to this list; through teams, an individual would need to find and solicit the effort of someone who has access to manage a team.

Ultimately, I think there might be some combination of both. A "Docs" team, for example, seems rather non-controversial.

In any case, I'd worry to avoid prematurely optimizing and at least getting something in place, so that we can identify where these groupings emerge. Even in getting this far, I've found it rather actionable in reaching out to people directly along the lines of "Hey, I've seen you've reviewed a few pull requests for X, would you be okay to receive notifications as a designated code owner?".

@nerrad

This comment has been minimized.

Copy link
Contributor

nerrad commented Jan 30, 2019

In any case, I'd worry to avoid prematurely optimizing and at least getting something in place, so that we can identify where these groupings emerge. Even in getting this far, I've found it rather actionable in reaching out to people directly along the lines of "Hey, I've seen you've reviewed a few pull requests for X, would you be okay to receive notifications as a designated code owner?".

Very good point!

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Jan 30, 2019

Reflecting on the inclusion of @WordPress/gutenberg-core for each entry, I'm not too pleased with the prospect of receiving an influx of notifications, and in truth I'd likely scramble to find some way to automate filtering thereof.

I think there's a point to be made that what's proposed here would be additive to whatever workflows people have already incorporated. Personally, I leverage IFTTT to aggregate pull requests into a daily digest, rather than receive a notification for each. Others, I expect, have their own processes.

I think it also risks losing some advantage we could have even within the "Core" team to be granular in our focuses. @iseulde mentioned an explicit interest on RichText-related modules, but I didn't include it here, since it'd be redundant with the fact that she'd receive notifications for all pull requests anyways as a member of the Core team.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 31, 2019

Thanks for putting this together @aduth I've been thinking about @WordPress/gutenberg-core and yeah, maybe we should just use individual usernames as expressed on the issue for now.

@ntwb

This comment has been minimized.

Copy link
Member

ntwb commented Jan 31, 2019

I think it also risks losing some advantage we could have even within the "Core" team to be granular in our focuses. @iseulde mentioned an explicit interest on RichText-related modules, but I didn't include it here, since it'd be redundant with the fact that she'd receive notifications for all pull requests anyways as a member of the Core team.

I'm in that same boat, being a member of the @WordPress/gutenberg-core team makes adding, me, ntwb redundant to the tooling section.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 31, 2019

/packages/rich-text

It was listed twice - fixed with 6474a4f.

I'm in that same boat, being a member of the @WordPress/gutenberg-core team makes adding, me, ntwb redundant to the tooling section.

I think the same issue applies to @chrisvanpatten. I would vote for listing everyone who opted for some areas explicitly. I can also add commit with my preferences. We can remove later gutenberg-core team from groups who have enough reviewers listed.

/lib @WordPress/gutenberg-core

# Documentation
/docs @WordPress/gutenberg-core @chrisvanpatten @mkaz @ajitbohra @nosolosw

This comment has been minimized.

@gziolo

gziolo Jan 31, 2019

Member

I would also add *.md in the Documentation section.

This comment has been minimized.

@aduth

aduth Jan 31, 2019

Author Member

I would also add *.md in the Documentation section.

This would trigger any CHANGELOG.md edits, which any pull request should include.

This comment has been minimized.

@gziolo

gziolo Jan 31, 2019

Member

Good point, how about */README.md then?

This comment has been minimized.

@aduth

aduth Jan 31, 2019

Author Member

Good point, how about */README.md then?

I think that could work, sure 👍

This comment has been minimized.

@aduth

aduth Jan 31, 2019

Author Member

Good point, how about */README.md then?

I think that could work, sure 👍

On second thought, I'm wondering if it might be better to leave out initially, as I worry it could become quite noisy. Like CHANGELOG, I think it could be that a large number of pull requests would include README changes to document their impact to public-facing APIs, in a way which perhaps loses its effectiveness to notifying the "most correct" parties.

Put another way, we can have much more confidence that a change to a file within /docs is part of a pull request oriented toward documentation than one which includes a change to a README file.

/packages/rich-text @WordPress/gutenberg-core

# PHP
/lib @WordPress/gutenberg-core

This comment has been minimized.

@gziolo

gziolo Jan 31, 2019

Member

I would also add *.php in the PHP section.

# Rich Text
/packages/format-library @WordPress/gutenberg-core
/packages/rich-text @WordPress/gutenberg-core

This comment has been minimized.

@gziolo

gziolo Jan 31, 2019

Member

How about CSS group and *.scss pattern match?

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 31, 2019

To clarify, even though I raised a few questions. I don't think they are essential to be answered before the first iteration. I'm fine with merging this document as is and we can continue discussion based on the feedback people share after a few days of getting notifications.

@gziolo

gziolo approved these changes Jan 31, 2019

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Jan 31, 2019

I would also add *.php in the PHP section.

How about CSS group and *.scss pattern match?

Ties a bit to my previous comment, but also: We don't have people to include for these items. We can add them if it's clear there's someone who's interested in being notified for any change to any SCSS or PHP file.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 31, 2019

Yes, I agree with the comments. I didn’t consider what volume of notifications it might create 👍

@ntwb

ntwb approved these changes Feb 1, 2019

Copy link
Member

ntwb left a comment

I agree with @gziolo, happy to see this merged as is and to experiment and iterate

@gziolo gziolo referenced this pull request Feb 4, 2019

Open

Add context argument to registry.select() #11460

4 of 4 tasks complete
Adding @coderkevin
See related discussion #11460 (comment)
@@ -0,0 +1,76 @@
# Data
/packages/api-fetch @youknowriad @gziolo @aduth @nerrad

This comment has been minimized.

@gziolo

gziolo Feb 4, 2019

Member

@kadamwhite @mmtr - I know that you both had very meaningful contributions in this package. Would you mind being listed as reviewers for any future code changes in this folder (or any other listed in this config file)?

This comment has been minimized.

@mmtr

mmtr Feb 5, 2019

Contributor

That sounds good to me, thanks @gziolo!

This comment has been minimized.

@gziolo

gziolo Feb 5, 2019

Member

Awesome, I will open PR :)

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Feb 4, 2019

Let's plan to land this and see how it works out. We should evaluate its effectiveness, maybe as soon as this Wednesday's editor chat, or next week's. We should acknowledge that changes to the document -- both additions and removals -- are expected and encouraged. It should be seen less as obligation and more as an attempt to improve review workflows by surfacing pull requests to interested parties.

Pings to those will start to see notifications as a result of these changes:

@youknowriad @gziolo @aduth @nerrad @Soean @ajitbohra @jorgefilipecosta @nosolosw @ntwb @chrisvanpatten @jaymanpandya @swissspidy @iseulde @mkaz @daniloercoli @diegoreymendez @etoledom @hypest @koke @marecar3 @mzorz @pinarol @SergioEstevao @Tug

The full list can be found here:

https://github.com/WordPress/gutenberg/blob/master/.github/CODEOWNERS

@aduth aduth merged commit 101853a into master Feb 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the add/code-owners branch Feb 4, 2019

@youknowriad youknowriad added this to the 5.1 (Gutenberg) milestone Feb 5, 2019

@nosolosw

This comment has been minimized.

Copy link
Member

nosolosw commented Feb 5, 2019

In lieu of a better place, I'll document my thoughts here: one gotcha that I've noticed is that you're pinged when the PR is created, not when is ready to review - ie: it doesn't take into account the In Progress label, PRs that are created as a personal playground/experiments, etc.

@koke

This comment has been minimized.

Copy link
Contributor

koke commented Feb 5, 2019

The workflow we use on mobile is that every PR has an assigned reviewer, will this change start pinging everyone for every mobile PR? It's not clear to me from the documentation.

I did a quick test and it seems like even if you explicitly request review from a code owner, it will still add everyone else to the PR. Unless there's a way to override that, I think I'd prefer to disable the ping for .native.js files, I imagine it'll be noisy and disruptive. @hypest what do you think?

@hypest

This comment has been minimized.

Copy link
Contributor

hypest commented Feb 5, 2019

👋 @koke , the idea here was to start with everyone (from the mobile squad) be on the list and we fine tune this by opening a new PR against the CODEOWNERS file. It's true that it's going to be super noisy if we leave it with the sweeping *.native.js.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 5, 2019

In lieu of a better place, I'll document my thoughts here: one gotcha that I've noticed is that you're pinged when the PR is created, not when is ready to review - ie: it doesn't take into account the In Progress label, PRs that are created as a personal playground/experiments, etc.

I think it's fine, it will allow sharing early feedback. As someone who authors PR one can set proper label and make it clear in the description what's the current status of PR.

@gziolo gziolo referenced this pull request Feb 5, 2019

Merged

Update CODEOWNERS.md file #13667

@nerrad

This comment has been minimized.

Copy link
Contributor

nerrad commented Feb 5, 2019

On problem I think still exists is that there is no easy way to distinguish between "reviewers" and "approvers". Of course its only really a problem for knowing when a pull can be merged and that would only be encountered by people who have the ability to merge to master.

Not sure what the resolution for this would be yet, but just noting it.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Feb 5, 2019

On problem I think still exists is that there is no easy way to distinguish between "reviewers" and "approvers". Of course its only really a problem for knowing when a pull can be merged and that would only be encountered by people who have the ability to merge to master.

Related conversation during today's #core-js meeting (link requires registration):

https://wordpress.slack.com/archives/C5UNMSU4R/p1549376782547200

daniloercoli added a commit that referenced this pull request Feb 5, 2019

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…rnmobile/372-use-RichText-on-Title-block

* 'master' of https://github.com/WordPress/gutenberg: (22 commits)
  Make the modal title styling consistent (#13669)
  Disable navigation block for text mode. (#12185)
  Fix: Linting problem in modal example code (#13671)
  Add myself as a code owner to the annotations (#13672)
  Add more reviewers to CODEOWNERS.md file (#13667)
  Plugin: Remove jQuery heartbeat-to-hooks proxying (#13576)
  Workflow: Add repository CODEOWNERS file (#13604)
  Add a mobile minimum size for form fields (#13639)
  Update edit-save documentation  (#13578)
  Alt image setting (#13631)
  Fix: Allow years lower than 1970 in DateTime component. (#13602)
  Using addQueryArgs to generate Manage All Reusable Blocks link (#13653)
  Bump plugin version to 5.0.0-rc.1 (#13652)
  Update lodash to 4.17.10 (#13651)
  Refreshed PR (#9469)
  Set default values of the width and height input fields according to the actual image dimensions (#7687)
  12647 fix css color picker (#12747)
  Remove "we" from messages (#13644)
  Fix: Font size picker max width on mobile (#13264)
  Fix/issue 12501 menu item aria label
  ...
@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Feb 5, 2019

I overlooked native stylesheets (*.native.scss) in the initial implementation here, so some folks might have been seeing notifications for mobile pull requests.

Improvements to this are proposed at #13675.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment