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

Implement CODEOWNERS #384

Merged
merged 10 commits into from
Jan 11, 2024
Merged

Implement CODEOWNERS #384

merged 10 commits into from
Jan 11, 2024

Conversation

sserita
Copy link
Contributor

@sserita sserita commented Dec 11, 2023

Implement a CODEOWNERS file for pyGSTi to modernize our PR pipeline.

This branch/PR will stay open as we get volunteers in. Edit: We now have all the volunteers and subteams set up.

Add Piper as code owner for instruments, workspace plots/tables, and associated tutorials
Add Tim and Jordan as drift/RB and RB code owners, respectively.

Also create a pygsti-maintainers team instead of calling out specific maintainers,
as a future-proofing mechanism.
Using the new pygsti-rb team to head off email spam

One annoyance of Code Owners is that PRs auto add all code owners,
UNLESS they are part of team that has auto assign enabled.
In this case, I've made a pygsti-rb team that auto-assigns
between Jordan and Tim in a round robin way.
Hopefully this will keep emails down.

One note: This is not really needed for one user code owners
since they will just get the notification and
the pygsti-maintainers team also has round-robin applied.
@sserita
Copy link
Contributor Author

sserita commented Dec 12, 2023

So one annoyance I've been reading about with code owners is email spam because every PR auto-assigns a review to all code owners. As far as I can tell, there is only ONE exception to this: When a code owner is a team, that team can be set to auto-assign to a member within the team and NOT email everyone else.

With this in mind, I have settled on the following approach:

  • When the code owner group is a single person, they are just added to the code owners as normal and will get notified/assigned to every PR that touches that part of the code. I believe this is probably intended behavior.
  • When the code owner group is multiple people, we create a subteam of the QPL team which will actually be the code owner. Then we set that group to auto-assign round robin and not email everyone else.

We currently have this happening twice: once for the maintainers and once for RB.
subteams

Here are the relevant settings for pygsti-maintainers, with the important boxes surrounded in red:
pygsti-maintainers

And here we have pygsti-rb, which is similar. However, in this case, I have to be part of the group to maintain it but I don't want to count towards the RB reviewer quota, so we tell GitHub to skip me:
pygsti-rb

@sserita
Copy link
Contributor Author

sserita commented Dec 12, 2023

I'll note that my previous solution has the added benefit that code owners for subteams can be changed out without requiring a modification of CODEOWNERS in the future.

@rileyjmurray
Copy link
Collaborator

@sserita I like the idea of creating subteams

Add pygsti-gatekeepers as owner everywhere

The intention will be that develop will require 2 approvals.
One will be the "subject matter" owner, and the other
will by a pyGSTi gatekeeper (Sandia staff member).
In cases with only one gatekeeper and the gatekeeper
is submitting a PR, this may require a branch protection bypass,
but that might be OK. TBD on how it all works out.
Added Kenny as code owner for RPE, tutorials, instruments
Moved Riley to core functionality from maintainers
@sserita sserita marked this pull request as ready for review January 10, 2024 23:05
@sserita sserita added this to the 0.9.12.1 milestone Jan 10, 2024
@sserita sserita self-assigned this Jan 10, 2024
Copy link
Contributor

@coreyostrove coreyostrove left a comment

Choose a reason for hiding this comment

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

This all looks good to me. Merge at will.

Copy link
Contributor

@coreyostrove coreyostrove left a comment

Choose a reason for hiding this comment

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

Try that again, but hit the approve button this time...

@sserita sserita merged commit ee6e3b3 into develop Jan 11, 2024
13 checks passed
@sserita sserita deleted the feature-code-owners branch January 11, 2024 19:49
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.

None yet

3 participants