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

chore: Refactor release workflow to allow workflow execution on generated PRs #13095

Merged
merged 4 commits into from
Mar 18, 2024

Conversation

jimblanc
Copy link
Contributor

@jimblanc jimblanc commented Mar 7, 2024

Description of changes

This PR proposes a change to our usage of [ci skip] in releases by introducing a new [skip release] syntax which will skip the push-latest-release.yml workflow specifically (which is what we're achieving with our current usage of [ci skip]).

This is intended to address the following issue:

  • A recent change (chore: Improve hot-fix & release process to generate a PR for merging back to main #13044) refactored our release process to create a PR for merging release back into main instead of force-pushing.
  • Because we're using [ci skip] in the "update API docs" & "Publish" commits which are generated during releases, the resulting PR does not trigger the codeql & pr workflows required to actually merge the PR as per the branch protection rules on main. As a result, admin intervention or additional commit is required to merge the PR.

Alternatives considered:

  • Use pull_request_target instead of pull_request for the CodeQL & PR workflows, which bypasses the [ci skip] check
    • This was discarded because pull_request_target changes the security posture of the repo for PRs originating from forks
  • Automatically merge the generated PR using gh pr merge
    • This was discarded because there may be merge conflicts due to changes on main (particularly in the case of hot-fixes) that would require manual intervention anyway

Other consequences of this change:

  • The API doc generation & Publish commits will trigger the push-main-release workflow when they're merged back to main. I don't believe that this should be an issue, and in fact updating unstable with the results of the latest release is arguably more correct.

Issue #, if available

Description of how you validated changes

Tested that this logic correctly skips the release workflow when [skip release] is present in the head commit of the triggering branch: https://github.com/aws-amplify/amplify-js/actions/runs/8180781321

Tested that PRs into main containing [skip release] still trigger the required PR workflows. Also verified that branches containing [ci skip] exhibit the same behavior we observed in the release (i.e. stuck PR workflows).

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jimblanc jimblanc marked this pull request as ready for review March 7, 2024 01:29
@jimblanc jimblanc requested review from a team as code owners March 7, 2024 01:29
@@ -28,7 +28,7 @@
"unlink-all": "lerna exec --no-bail --parallel -- yarn unlink; exit 0",
"publish:preid": "./scripts/preid-env-vars-exist.sh && lerna publish --canary --force-publish --dist-tag=${PREID_PREFIX} --preid=${PREID_PREFIX}${PREID_HASH_SUFFIX} --yes",
"publish:main": "lerna publish --canary --force-publish --dist-tag=unstable --preid=unstable${PREID_HASH_SUFFIX} --yes",
"publish:release": "lerna publish --conventional-commits --message 'chore(release): Publish [ci skip]' --yes",
"publish:release": "lerna publish --conventional-commits --message 'chore(release): Publish [skip release]' --yes",
Copy link
Member

Choose a reason for hiding this comment

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

nit question,
If we have multiple variables would we prefer my commit message [skip release] [skip e2e] or my commit message [skip release e2e]

Copy link
Contributor Author

@jimblanc jimblanc Mar 7, 2024

Choose a reason for hiding this comment

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

It's a good question, I think I'd prefer to keep it narrowly tailored to the overall "intents" in our CI 🤔 Open to other opinions though

Copy link
Member

@stocaaro stocaaro left a comment

Choose a reason for hiding this comment

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

I would be a fan of putting a PR/human into this process to avoid using the write token for security reasons. This isn't changing the existing process, so as an amendment I don't see any risk in moving forward as is.

Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

Looks good

Thanks @jimblanc 🎖️

@jimblanc jimblanc merged commit 25adcfe into aws-amplify:main Mar 18, 2024
30 checks passed
jimblanc added a commit to jimblanc/amplify-js that referenced this pull request Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants