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

Update CODEOWNERS file #347

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Update CODEOWNERS file #347

wants to merge 2 commits into from

Conversation

sosey
Copy link
Member

@sosey sosey commented Jan 27, 2025

This updates the CODEOWNERS file to specify two global owners for review of any notebook file and the repo itself, along with folks in INS who are designated as triage/curator for each instrument.

This updates the CODEOWNERS file to specify two global owners for review of any notebook file and the repo itself, along with folks in INS who are designated as triage/curator for each instrument.
@sosey
Copy link
Member Author

sosey commented Jan 27, 2025

There is also reference in this repo to an "hst_notebooks science curators team", which may have been used in the past, but looks possibly old.

I think it's reasonable to include the right people for each instrument in the codeowners file. If all the tests are passing, and someone other than the submitter has reviewed and approved, then the PR could merge. Do you all feel this is sufficient? The number of required reviews can also be specified in the repo settings. The current PR allows for the case of someone other than an instrument team curator submitting a PR, and requiring someone from the relevent team to review and approve.

@gibsongreen
Copy link
Collaborator

There is also reference in this repo to an "hst_notebooks science curators team", which may have been used in the past, but looks possibly old.

I think it's reasonable to include the right people for each instrument in the codeowners file. If all the tests are passing, and someone other than the submitter has reviewed and approved, then the PR could merge. Do you all feel this is sufficient? The number of required reviews can also be specified in the repo settings. The current PR allows for the case of someone other than an instrument team curator submitting a PR, and requiring someone from the relevent team to review and approve.

Hello @sosey! I have been think this through and do have a few comments that I've been thinking through. The first that I believe predicates the rest of what I was considering is, is this something we would want to standardize across all the notebook repositories?

@sosey
Copy link
Member Author

sosey commented Jan 28, 2025

Yes, we need to standardize this, even across all the spacetelescope repos. I've brought it up in several of the notebook meetings but will raise it again the next time we meet. I thought this was a good repo to start with for the notebooks, given the changes INS and DMD has for triage and review. We can flush out what makes sense with the functionality we have and then use it as an example for the other notebook repos. Please comment your other suggestions :)

@gibsongreen
Copy link
Collaborator

Wonderful, I think it is a great idea to start here and to have a repository that shows somewhat of a proof of concept for standardizing this process! I'll continue to let this simmer, here are a couple of things that I have begun to consider:

  1. For jdaviz, we do require 2 technical reviews unless a ‘trivial’ label is added to the PR. I don’t see that label currently in hst_notebooks but that should be straightforward to implement and could be a good way to distinguish PRs that are already passing tests versus those that need a full technical review (ie. 1 vs. 2 reviewers)
  2. A decent fraction of notebook PRs can appear to be trivial if the diff is small or code changes are inconsequential to the functionality of the notebook/directory of said notebook. In some cases though (off the top of my head an example is html deployment), unless a curator has merged PR(s), there can be cases where a PR is merged and we will need to open a second PR if something like the html isn’t rendered because the notebook file itself wasn’t updated (just one example). That being said, if we do require two approvals/reviewers it would be valuable to ensure that one of the reviews is technically/CI based.

Just to make a note of this, updating the template notebook structure and any 'How To' instructions for curators explaining new updates to the review process will also need to be updated so everyone is on the same page of the new standard review practices

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.

2 participants