NCAR / container-dtc-nwp

End-to-end NWP systems in containers.
https://dtcenter.org/community-code/numerical-weather-prediction-nwp-containers
28 stars 13 forks source link

Feature/issue 63 pr template #64

Closed JohnHalleyGotway closed 2 years ago

JohnHalleyGotway commented 2 years ago

Pull Request Testing

Pull Request Checklist

fossell commented 2 years ago

@JohnHalleyGotway - I think this is a good start and don't have much to add for these testing checklists. I'd need to use it for the project a bit to see if there are gaps or changes to help improve it, so my vote would be to merge and enhance later.

More generally, should we expand this template to also include things like description of changes, issues this fixes, etc.? We could just start with this to streamline our testing/review requests and add to the template as time allows too.

JohnHalleyGotway commented 2 years ago

Kate,

Thanks for the feedback. Personally no, I do not think the nitty gritty details about description of changes belong in the pull request. Instead, all of those details should live in the issue... either in the body of the issue or in comments to it. And we can certainly create issue templates to cover those details, if warranted. The PR should remain focused on testing those changes and making note of their expected impacts on existing systems.

I've gotten used to the following naming conventions in METplus and find them to be very helpful:

This process keeps the issues that describe the work well linked to the PR's that describe their impacts.

fossell commented 2 years ago

@JohnHalleyGotway - That makes sense, I know it varies by project, some have all info in PR. I don't have a strong preference, as long as we define what we're doing here for this project and stick to it. I'll approve this now, and then if we wan tto discuss adding/changing in the future when we have time that is fine too.

JohnHalleyGotway commented 2 years ago

@jwolff-ncar are you still planning to review, or should I remove you as a reviewer and proceed with only the review from @fossell?