search

NCAR / ccpp-physics

Collection of physics parameterizations compliant with the Common Community Physics Package (CCPP) framework
Other
60 stars 146 forks source link

Update CODEOWNERS: add NRL representatives #1086

Open climbfuji opened 2 months ago

climbfuji commented 2 months ago

As previously discussed, add representatives from NRL as codeowners for ccpp-physics. I am going to create a similar PR for the ufs/dev branch in the ufs-community fork.

I will also note that there are still errors in this CODEOWNERS file (before me making any changes), as indicated by Github. I noted that the overall codeowners are the same for both forks, so therefore I only create the PR here and wait for it to make it into the ufs/dev branch (Fanglin approved this by email a couple of months ago)?

climbfuji commented 2 months ago

image

dustinswales commented 2 months ago

image

@climbfuji I believe(?) we decided sometime ago that we weren't going to give CODEOWNERS from the ufs/dev fork write access to the NCAR:ccpp-physics repo. Hence these errors.

climbfuji commented 2 months ago

image

@climbfuji I believe(?) we decided sometime ago that we weren't going to give CODEOWNERS from the ufs/dev fork write access to the NCAR:ccpp-physics repo. Hence these errors.

Ok, but then should they be removed? Or will they still be assigned to review (I believe not)? Or do you just want to keep the file in sync and live with the fact that they won't be assigned as codeowners?

dustinswales commented 2 months ago

Ok, but then should they be removed? Or will they still be assigned to review (I believe not)? Or do you just want to keep the file in sync and live with the fact that they won't be assigned as codeowners?

@climbfuji You got it. We don't want CODEOWNERS to get notified a second time for changes that go from UFS:ufs/dev -> NCAR:main, and we don't want the CODEOWNERS file to have a delta between the two repositories. So essentially we limit the "collaborators" on the NCAR side to avoid the excessive notifications, and keep the files synced. Not ideal, but we didn't have any better solutions at the time.

climbfuji commented 2 months ago

Ok, but then should they be removed? Or will they still be assigned to review (I believe not)? Or do you just want to keep the file in sync and live with the fact that they won't be assigned as codeowners?

@climbfuji You got it. We don't want CODEOWNERS to get notified a second time for changes that go from UFS:ufs/dev -> NCAR:main, and we don't want the CODEOWNERS file to have a delta between the two repositories. So essentially we limit the "collaborators" on the NCAR side to avoid the excessive notifications, and keep the files synced. Not ideal, but we didn't have any better solutions at the time.

Thanks, this makes sense.

climbfuji commented 1 month ago

@grantfirl @dustinswales Is there a way we can merge this PR for NCAR main? There won't be a PR for ufs-community as discussed previously.

grantfirl commented 1 month ago

@climbfuji @dustinswales We're not using the CODEOWNERS file at all for the NCAR/main branch because we don't want to notify developers twice, once for ufs/dev and once for main for every PR. So, by adding @climbfuji to CODEOWNERS here, does it really accomplish anything?

climbfuji commented 1 month ago

@climbfuji @dustinswales We're not using the CODEOWNERS file at all for the NCAR/main branch because we don't want to notify developers twice, once for ufs/dev and once for main for every PR. So, by adding @climbfuji to CODEOWNERS here, does it really accomplish anything?

I am confused. I see the four current "overall" codeowners from DTC and NOAA notified and their reviews requested for this PR. So the file gets used. How are you otherwise making sure that PRs into NCAR ccpp-physics main are reviewed and approved (a) by the maintainers/codeowners of the schemes that changed, and (b) by each organization represented in the CCPP physics effort (DTC, NOAA, NRL so far)?

grantfirl commented 1 month ago

@climbfuji @dustinswales We're not using the CODEOWNERS file at all for the NCAR/main branch because we don't want to notify developers twice, once for ufs/dev and once for main for every PR. So, by adding @climbfuji to CODEOWNERS here, does it really accomplish anything?

I am confused. I see the four current "overall" codeowners from DTC and NOAA notified and their reviews requested for this PR. So the file gets used. How are you otherwise making sure that PRs into NCAR ccpp-physics main are reviewed and approved (a) by the maintainers/codeowners of the schemes that changed, and (b) by each organization represented in the CCPP physics effort (DTC, NOAA, NRL so far)?

Me too, apparently. I don't know how the current reviewers are being generated by GitHub since the CODEOWNERS has been turned off in the settings. When we had CODEOWNERS turned on for this branch, GitHub would automatically send a notification to review and we would manually have to remove reviewers every time. At some point, the CODEOWNERS file became out-of-sync with collaborator permissions (either someone didn't accept the invitation or doesn't have permissions or both). Since we didn't want developers to have to review twice anyway, we didn't worry about fixing the issues and just turned it off. I think that is what happened. Then, we have manually been adding reviewers if they need to be added.

I still don't see a good technical solution here if we don't want to spam developers with multiple code reviews for each fork. We could maintain different files in the two forks and include you in this one so that you always get a review request and nuke all of the usernames for individual files, taking care to manually add reviewers as necessary based on the the ufs/dev file. Maybe that is the best solution.

dustinswales commented 1 month ago

I still don't see a good technical solution here if we don't want to spam developers with multiple code reviews for each fork. We could maintain different files in the two forks and include you in this one so that you always get a review request and nuke all of the usernames for individual files, taking care to manually add reviewers as necessary based on the the ufs/dev file. Maybe that is the best solution.

^I think this is the only solution. Different CODEOWNER files across repos.

climbfuji commented 1 month ago

Thank you both. I agree there is no ideal solution. If you don't want to bother a scheme maintainer with multiple reviews, but alert them if changes are coming in from somewhere else than a fork where they are already listed as reviewer, then this would get very complicated to automate.

Maybe we define a list of ALL-REPO codeowners for NCAR ccpp-physics main with two representatives from each organization involved in the ccpp-physics effort. It would then be the responsibility of these representatives to add reviewers from their organization as needed for PRs into NCAR main?