Skip to content

Add initial code owners #272

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

Merged
merged 5 commits into from
Jan 10, 2025
Merged

Add initial code owners #272

merged 5 commits into from
Jan 10, 2025

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented Jan 9, 2025

This PR adds an initial set of code owners, as discussed at today's MESSAGE team meeting.

Some notes:

  • @measrainsey and @GamzeUnlu95 are added so we can soon try out the process of identifying (a) person(s) to take over these codes and making a follow-up PR that modifies the CODEOWNERS file.
  • We could also add a line .github/CODEOWNERS @khaeru @glatterf42 or similar to ensure people are requested to review changes to the owners file itself. I would need to look around for other uses of this feature to see if that's common.

How to review

I've requested review from everyone who'll be affected by this change.

  • Check whether the paths in the added file include all the files you see yourself as responsible for.
    • None missing.
    • None included which are actually owned by someone else.
  • Approve.

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅
  • Add, expand, or update documentation.
  • Update doc/whatsnew.

@khaeru khaeru added doc Improvements or additions to documentation enh New features or functionality labels Jan 9, 2025
@khaeru khaeru self-assigned this Jan 9, 2025
Copy link

codecov bot commented Jan 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.7%. Comparing base (d55fbf9) to head (d36243a).
Report is 310 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #272     +/-   ##
=======================================
- Coverage   77.6%   76.7%   -1.0%     
=======================================
  Files        211     211             
  Lines      16079   16079             
=======================================
- Hits       12481   12333    -148     
- Misses      3598    3746    +148     

see 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@glatterf42 glatterf42 left a comment

Choose a reason for hiding this comment

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

Great, thanks for setting this up! I assume we're not setting an owner for message-ix-models/report because no one is clearly responsible for this part? If we find someone to work on replicating all legacy reporting features here, we can add them :)

As for one of your questions, it is recommended as the most secure practice to define the repository owners as the owners of .github/ or at least .github/CODEOWNERS.

Comment on lines 27 to 31
# message_ix_models/project/advance
# message_ix_models/project/circeular
# message_ix_models/project/edits
# message_ix_models/project/engage
# message_ix_models/project/gea
Copy link
Member

Choose a reason for hiding this comment

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

I guess these should eventually also include the relevant doc pages, i.e. doc/project/advance, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. There is a distinction between completed projects and current/future projects. I have not yet added the docs but will put some indicative language about how those should be handled.

@khaeru
Copy link
Member Author

khaeru commented Jan 10, 2025

As for one of your questions, it is recommended as the most secure practice to define the repository owners as the owners of .github/ or at least .github/CODEOWNERS.

Thanks for spotting that. It seems we don't have an explictly-defined owner for the repo, only:

  • You and I (and also many others) who are Owners of the overall @iiasa org, thus have 'owner' role priviliges on the repo.
  • @iiasa/message-ix-devs have the 'maintain' role.

I guess for now I will add the two of us.

I assume we're not setting an owner for message-ix-models/report because no one is clearly responsible for this part?

That's one of many areas where I wrote some initial code but it would be good to have joint contributions (possibly many people) to (a) expanding and (b) maintaining it. I'm not sure yet how that maps to the idea of "owner" and review duties, so I've left it out for now.

Someday we could aim for having an owner for most files, but use non-ownership to designate code as unmaintained, as an indication that people should not rely on it or that we intend to migrate to better solutions and deprecate it.

@khaeru khaeru merged commit dbd783e into main Jan 10, 2025
30 checks passed
@khaeru khaeru deleted the enh/codeowners branch January 10, 2025 12:35
measrainsey pushed a commit that referenced this pull request Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Improvements or additions to documentation enh New features or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants