Add CODEOWNERS

[kratman]

Description

Change

Code owners are now specified in .github/CODEOWNERS and can be configured as required reviewers. This can help make it easier for maintainer-trainees to help with the project maintenance. The code owners can be required to approve pull requests, but the trainees can be granted write permission (merge approved PRs, trigger workflow runs, etc). This will make the trainees more involved in the process.

Side effects

All members of the pybamm-team/maintainers group will be requested to review on new PRs. This can be avoided by making PRs drafts. The review requests happen only when a PR is marked as ready for review.

Next steps

In order for this to work properly some settings need to be adjusted by a maintainer.

1. The maintainer-trainees group should have permissions upgraded to write access. This should let us merge approved PRs and trigger workflows.

2. The following four options should be enabled in the repo after this PR is merged. These settings will prevent non-code owners from merging code that is not approved by a code owner. Dismissing stale reviews prevents changes being pushed post-review.
   

Key checklist:

• No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
• All tests pass: $ python run-tests.py --all (or $ nox -s tests)
• The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[kratman]

@brosaplanella, @Saransh-cpp, @agriyakhetarpal This is from our discussion in Slack. I set up the simplest code owners file. Making it works requires settings, so we would have to merge this PR, then do some additional testing when the settings are changed. Not sure how the maintainers feel about being pinged for each PR.

[agriyakhetarpal]

Looks good to me, thanks @kratman! I would like to note that I am not sure how I feel about an entire team getting tagged on new PRs. In my experience, there is a lot of noise in my GitHub notifications despite the fact that I watch repositories selectively... is there a workaround? I have seen other repositories and teams work with CODEOWNERS without automatic review requests: they show up in a tooltip instead with the words "@user requested a review from a team as a code owner" – does that require GitHub Enterprise?

[kratman]

@agriyakhetarpal I have only worked on a single project that used CODEOWNERS. I was not admin for that repo, so I do not know how configuring notifications works with it. I know a bunch of people who setup custom email filters to handle github notifications.

I put my notification/watch settings pretty high for projects I work on, but I know it annoys some people.

[Saransh-cpp]

Thanks for working on this, @kratman! I'll streamline this in the upcoming days to avoid sending notifications to everyone on every PR. I'll probably push to this branch itself.

[kratman]

Seems like that is a lot of fine grained control that needs to be edited and maintained over time. I did the one line version to keep it simple.

Do we really need to restrict certain modules to specific users rather than just the team as a whole? I would hope all maintainers can be trusted with the entire codebase.

[Saransh-cpp]

Ended up working on this, haha!

@pybamm-team/maintainers please go through the file and make suggestions / changes.

@rtimms @priyanshuone6 I could not find your response in the "maintainer areas" form floated a while back. Please let me know if there are any folders that you are comfortable maintaining, or feel free to add yourself to the file.

Edit: I cannot add maintainer trainees to the file as they do not have write access to pybamm. I'll look into the workflow permissions.

[kratman]

Edit: I cannot add maintainer trainees to the file as they do not have write access to pybamm. I'll look into the workflow permissions.

I don't think they should be in there. The original purpose was to let the trainees get more access while still restricting what they can do with the code. Once a basic code owners file is in place the trainees can safely have their rights elevated.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (40be4bc) 99.59% compared to head (1f4385f) 99.59%.
Report is 2 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3591   +/-   ##
========================================
  Coverage    99.59%   99.59%           
========================================
  Files          258      258           
  Lines        20811    20823   +12     
========================================
+ Hits         20726    20738   +12     
  Misses          85       85           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Saransh-cpp]

• The listed maintainers will receive a notification whenever a file owned by them is edited, but everyone can review and approve those PRs.
• This will auto-request reviews.
• For pybamm-team/maintainers, the whole team would be requested for review. For instance, this PR has also requested a review from pybamm-team/maintainers.
• I see, this PR's focus shifted from giving trainees more permissions to just adding CODEOWNERS -
  • We had discussed adding CODEOWNERS in pybamm in the last SC meeting, so these are 2 separate issues.
  • The notifications sent to the maintainers would be a problem, given that not every maintainer is watching every activity on this repository.
  • Personal issue: I use https://github.com/pulls/review-requested and https://github.com/pybamm-team/PyBaMM/pulls?q=is%3Apr+is%3Aopen+user-review-requested%3A%40me to track the PRs I have to review, and it will become difficult for me to use these page if they are filled with PRs that I cannot/should not review.

The best option would be to give write permission to trainees who are about to transition to the maintainer status, as suggested by @brosaplanella.

[kratman]

• For pybamm-team/maintainers, the whole team would be requested for review. For instance, this PR has also requested a review from pybamm-team/maintainers.

It seems like half the maintainers would get notified with this option as well. That is sort of why I was pushing for the simpler solution.

[valentinsulzer]

Unrelated to the general discussion here, but should we move meshes, geometry, spatial_methods, and discretization under one folder?

In #3596 I moved step into the experiment folder

[Saransh-cpp]

Thanks for working on this @kratman! I think we have the final structure ready now. There will be a few changes once discretisation-related files are clubbed, so maybe we can merge this after #3641.

[kratman]

@Saransh-cpp Since #3641 is stalled, can we merge this so it can be tested and update it later?

[Saransh-cpp]

can we merge this so it can be tested and update it later?

Let's wait for a couple of days for the remaining approvals from SC.

[kratman]

can we merge this so it can be tested and update it later?

Let's wait for a couple of days for the remaining approvals from SC.

@tinosulzer @brosaplanella @martinjrobins