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 pull request template #47

Merged
merged 1 commit into from
Feb 21, 2023
Merged

Conversation

jsf9k
Copy link
Member

@jsf9k jsf9k commented Oct 28, 2022

🗣 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

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All new and existing tests pass.

@jsf9k jsf9k added improvement This issue or pull request will add or improve functionality, maintainability, or ease of use documentation This issue or pull request improves or adds to documentation labels Oct 28, 2022
@jsf9k jsf9k self-assigned this Oct 28, 2022
@jsf9k jsf9k marked this pull request as ready for review October 28, 2022 18:42
Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

👍 👍

@jsf9k jsf9k force-pushed the improvement/update-pr-template branch from 70d7cd4 to 1060b23 Compare November 21, 2022 15:55
@mcdonnnj
Copy link
Member

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
Copy link
Member Author

jsf9k commented Nov 21, 2022

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
Copy link
Member Author

jsf9k commented Nov 21, 2022

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
Copy link
Member Author

jsf9k commented Feb 14, 2023

See also @dav3r's comment here.

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

jsf9k commented Feb 15, 2023

@mcdonnnj - please see #50.

@jsf9k jsf9k requested a review from a team February 15, 2023 20:07
Copy link
Member

@felddy felddy left a comment

Choose a reason for hiding this comment

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

Stellar. 🌟

Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

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).

@jsf9k
Copy link
Member Author

jsf9k commented Feb 21, 2023

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.

@jsf9k jsf9k merged commit d6be02d into develop Feb 21, 2023
@jsf9k jsf9k deleted the improvement/update-pr-template branch February 21, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This issue or pull request improves or adds to documentation improvement This issue or pull request will add or improve functionality, maintainability, or ease of use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants