Rulesets proposals

[gmaclennan]

Some thoughts about what rules we should activate to avoid human-error in the release process. I'll use the term "release branch" to refer to branches that match release/* and "RC branch" to refer to branches that match rc/*.

• Restrict creation of release and RC branches to the release-bot (Rule: restrict creations). Why? Because it's important that they are created together, and there is the commit to prepare the release (bump the package version) by the bot, so we don't want people doing this manually.
• Require all commits to release branches to be via PR (Rule: Require a pull request before merging). Why? It stops anyone accidentally pushing a commit to a release branch, ensuring that all releases go through the PR review process.
• Require PRs to release branch to be approved, and dismiss reviews when new commits are pushed. Why? Visibility on who approves an actual release, to avoid questions about why a release has happened
• Only allow merge (not rebase or squash) when merging a PR to the release branch. Why? Better visibility of what commits are in a release.
• For now don't require someone other than the commit author to review the commit. Why? Seems like too much bureaucracy for now.
• Require status checks to pass before merges to a release branch. Why? We don't want failing tests for a release, and there are other things we need to enforce with status checks that cannot be enforced with a ruleset.
• Only allow PRs to a release branch from a RC branch (I don't think this is possible with a ruleset, so will need to be enforced by a required status check matching PRs that target a release branch). Why? We don't want someone accidentally merging a feature branch to a release branch and triggering a release.
• Require a linear history for commits to an RC branch. Why? Clear history of changes during RC process
• Block force pushes on develop and any RC or release branch. Why? It affects other contributors, and disrupts the history of what code has been released or included in RCs.
• Block a RC branch from being merged twice. I don't think this is possible with any ruleset; maybe the easiest way to do this is to auto-delete the RC branch after merge. Why? Stops a user from pushing to an RC branch that has already been merged by mistake, and potentially merging with the RC branch again (duplicate release).
• Require that any code merged with a release branch (A) does not have a pre-release version name and (B) increments the version name and (C) the version name matches the RC branch name. Why? This is to avoid a user accidentally changing the version name with a commit to the RC branch. This would have to be enforced by a status check.

I think we should also create release tags so that we know exactly what code is included in a release (because either the RC branch refs, which will point to the release commit, can be modified by pushing new commits, or we delete them). We should set up tag rules for these release tags so that they can only be created by the release bot, and they cannot be updated or removed by anyone including the release bot.

If we are going to be tagging the release commit, then I'm not sure whether it is best to trigger the release based on the PR merge, or trigger the tag based on the PR merge, then trigger the release based on the tag. I think maybe the former, since I think it will mean that the release build shows in the merged PR status?

[achou11]

Restrict creation of release and RC branches to the release-bot (Rule: restrict creations). Why? Because it's important that they are created together, and there is the commit to prepare the release (bump the package version) by the bot, so we don't want people doing this manually.

agreed, and done via RC and Release rulesets!

Require all commits to release branches to be via PR (Rule: Require a pull request before merging). Why? It stops anyone accidentally pushing a commit to a release branch, ensuring that all releases go through the PR review process.

agreed, and done via Release ruleset!

Require PRs to release branch to be approved, and dismiss reviews when new commits are pushed. Why? Visibility on who approves an actual release, to avoid questions about why a release has happened

agreed, and (maybe?) done via Release ruleset! A little confused about the difference between Dismiss stale pull request approvals when new commits are pushed and Require approval of the most recent reviewable push. I have the former enabled but not the latter right now.

Only allow merge (not rebase or squash) when merging a PR to the release branch. Why? Better visibility of what commits are in a release.

agreed, and done via Release ruleset!

For now don't require someone other than the commit author to review the commit. Why? Seems like too much bureaucracy for now.

agreed!

Require status checks to pass before merges to a release branch. Why? We don't want failing tests for a release, and there are other things we need to enforce with status checks that cannot be enforced with a ruleset.

agreed, and done via Release ruleset! As we've discovered the hard way, the GitHub UI for configuring this is incredibly confusing, but it's working as expected right now.

Only allow PRs to a release branch from a RC branch (I don't think this is possible with a ruleset, so will need to be enforced by a required status check matching PRs that target a release branch). Why? We don't want someone accidentally merging a feature branch to a release branch and triggering a release.

agreed! and confirming that this doesn't seem possible via rulesets or classic branch protection rules

• Add required status check to compare opened PRs that point to release branch EDIT: done via add checks for PRs to release branchs #7

Require a linear history for commits to an RC branch. Why? Clear history of changes during RC process

agreed, and done via RC ruleset!

Block force pushes on develop and any RC or release branch. Why? It affects other contributors, and disrupts the history of what code has been released or included in RCs.

agreed, and done via Default, RC, and Release rulesets!

Block a RC branch from being merged twice. I don't think this is possible with any ruleset; maybe the easiest way to do this is to auto-delete the RC branch after merge. Why? Stops a user from pushing to an RC branch that has already been merged by mistake, and potentially merging with the RC branch again (duplicate release).

agreed, and done via repo general settings! I always prefer automatically deleting the branch after merging PRs, so this is covered by that setting.

Require that any code merged with a release branch (A) does not have a pre-release version name and (B) increments the version name and (C) the version name matches the RC branch name. Why? This is to avoid a user accidentally changing the version name with a commit to the RC branch. This would have to be enforced by a status check.

agreed, still need to implement. just to clarify, you're talking about doing a status check on RC PRs that point to a release branch?

• implement status check to ensure changes in version are correct EDIT: deprioritized and planning to do an eventual follow-up (see add checks for PRs to release branchs #7 (comment))

[achou11]

I think we should also create release tags so that we know exactly what code is included in a release (because either the RC branch refs, which will point to the release commit, can be modified by pushing new commits, or we delete them). We should set up tag rules for these release tags so that they can only be created by the release bot, and they cannot be updated or removed by anyone including the release bot.

agreed, still need to implement.

• implement tag ruleset to make sure that release tags: 1) can only be created by release bot, 2) cannot be deleted or modified by anyone
  • EDIT: done via Release tags ruleset, I think. Not really sure how to make it so that we can have Release Bot bypass the creation restriction but not the update+delete restrictions. From what I can tell, you can't set that level of bypass granularity for a ruleset, and creating a separate ruleset doesn't seem like it would work 🤔
  • EDIT: I've created separate rulesets - one for restricting release tag creation to just Release Bot and the other preventing anyone from updating or deleting release tags. I think rulesets layer so my expectation is that this achieves the desired restrictions when applied, but could be very wrong about that. Some relevant docs here

If we are going to be tagging the release commit, then I'm not sure whether it is best to trigger the release based on the PR merge, or trigger the tag based on the PR merge, then trigger the release based on the tag. I think maybe the former, since I think it will mean that the release build shows in the merged PR status?

agree with option of triggering the release based on the PR merge. To clarify, do you envision this being done within the build-release workflow, or an entirely separate workflow definition?

• implement creation of release tag after merging a PR into a release branch. EDIT: done via create tag as part of release workflow #6

[gmaclennan]

agree with option of triggering the release based on the PR merge. To clarify, do you envision this being done within the build-release workflow, or an entirely separate workflow definition?

• implement creation of release tag after merging a PR into a release branch.

Yes I think this can be done at the end of the build release workflow e.g.

    - name: Parse version
        id: version
        uses: ./.github/parse-version
    - name: Tag Release
        run: |
            git config --global user.name 'awana-release-bot[bot]'
            git config --global user.email '${{ vars.RELEASE_BOT_USER_ID }}+awana-release-bot[bot]@users.noreply.github.com'
            git tag v${{ steps.version.outputs.releaseShort }}
            git push origin v${{ steps.version.outputs.releaseShort }}