cisagov / .github

Default community health files for cisagov
Creative Commons Zero v1.0 Universal
6 stars 11 forks source link

Update pull request template #47

Closed jsf9k closed 1 year ago

jsf9k commented 2 years ago

๐Ÿ—ฃ Description

This pull request updates the pull request template to not mention manually creating a tag.

๐Ÿ’ญ Motivation and context

All tags should be created via the pre-release or release process so that there is some documentation and changelog associated with the tag.

See also cisagov/action-lineage#65.

๐Ÿงช Testing

None, other than my ocular orbs.

โœ… Pre-approval checklist

mcdonnnj commented 1 year ago

I'm not sure I'm on board with this strategy. I think leveraging something like softprops/action-gh-release to create a release when a tag is pushed using the tag.sh script or similar is a better approach. This isn't as obvious with things like our Packer and Docker projects since the release artifacts are not on GitHub but things like our Python projects would involve cutting a release manually (including manually entering the tag as part of the release creation process) and then manually adding the appropriate wheel and source distributions once the tag push triggered workflow is finished running. I think automatically generating a release on a pushed tag and then editing the release notes as necessary/desired is more straightforward and less prone to human error.

jsf9k commented 1 year ago

I'm not sure I'm on board with this strategy. I think leveraging something like softprops/action-gh-release to create a release when a tag is pushed using the tag.sh script or similar is a better approach. This isn't as obvious with things like our Packer and Docker projects since the release artifacts are not on GitHub but things like our Python projects would involve cutting a release manually (including manually entering the tag as part of the release creation process) and then manually adding the appropriate wheel and source distributions once the tag push triggered workflow is finished running. I think automatically generating a release on a pushed tag and then editing the release notes as necessary/desired is more straightforward and less prone to human error.

In what follows, I'm thinking in particular about our *-packer repos, where AMIs for staging and develop are currently generated via a developer manually creating a pre-release or release in GitHub, respectively.

You make a good point, and I'm in general supportive of as much automation as possible. It should be noted, though, that going with your method would require considerable changes to the existing workflows.

Another point I'd like to bring up is that the GH Actions code should pre-populate the pre-release/release notes with the correct content to cut down on the amount of manual intervention required. We don't have separate develop and release branches right now, so normally a pre-release is created from a feature branch. This means that the PR for the feature branch should be included in the pre-release notes, but GitHub does not do this and the developer must add it manually. (Perhaps the best solution in this case would be to use separate develop and release branches where pre-releases are created when code is merged into develop and releases are created when code is merged into release. In this case, though, we would have to do something to create an AMI when working in a feature branch; otherwise, there is no way to test AMI changes before the code is merged into develop. I'd be OK with letting developers create AMIs manually for pre-staging testing.)

It is also worth noting that the versioning gets a little complicated when we rebuild AMIs and bump the build portion of the semver. For instance, the release for 1.2.3+build.4 should generate release notes based on the PRs that have been merged since 1.2.3+build.2. I'd be fine with changing our versioning scheme for these cases where we don't make any code changes but rebuild the AMI just to freshen the packages it contains, but I haven't found anything else that makes sense.

jsf9k commented 1 year ago

Even better would be to:

  1. Add a GH Actions check to verify that the code in the PR bumps the version appropriately
  2. Have GH Actions cut pre-releases/releases when code is merged to develop or release, respectively

This removes the human from even performing the tagging.

jsf9k commented 1 year ago

See also @dav3r's comment here.

jsf9k commented 1 year ago

@mcdonnnj - please see #50.

jsf9k commented 1 year ago

Just as a note I would like us to define somewhere (cisagov/development-guide?) what changes constitute a reason for a new release to be cut. In the same vein I think we should point out example cases of how the version should be modified in line with semver (which I believe we are all on board with using as a standard).

I will work on those changes.