-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[No QA] Move version-bump and automerge code to its own workflow #2329
Conversation
Okay, there are still more simplifications we can do after this, but we can move in increments. I think this PR is enough to resolve the problem at hand (i.e: only deploy to staging when the staging branch is updated, and same for production). There are several more simplifications we can do after this before adding complexity with the CP flows. |
I took this off draft, and I could use some reviews at this point. That said, the likelihood that this won't break anything and will just work first try is slim. To minimize disruptions, @AndrewGable and I were thinking we would schedule a day to live-test this, and let Applause skip QA that day. So please review but don't merge 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I like where this is headed for sure. It seems like there are still some TODOs left before we can talk about testing this, is that correct?
Correct, we need to either get those 3rd-party PRs merged or point at my fork of the repos for now. We could probably benefit from doing some more atomic testing of the updated 3rd-party actions using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably benefit from doing some more atomic testing of the updated 3rd-party actions using Public-Test-Repo before diving in to try and test this whole PR
This sounds like a good idea, what specific tests need to be run?
|
||
# TODO: only need to create this tag because of how the GitUtils work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what this means, is this still a TODO?
return exec(`npm --no-git-tag-version version ${newVersion} -m "Update version to ${newVersion}"`); | ||
}) | ||
console.log(`Setting npm version to ${newVersion}`); | ||
exec(`npm --no-git-tag-version version ${newVersion} -m "Update version to ${newVersion}"`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just do "Update version to %s"
here. (ref)
|
||
- name: Set up git | ||
run: | | ||
git fetch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: Do you know if these are chained like &&
in bash, in that the next command won't run unless the previous command exits with a return code of 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question ... I do not know the answer.
Got some movement on this PR and updated it after the maintainer's review. |
.github/workflows/automerge.yml
Outdated
needs: getPullRequestMergeability | ||
if: github.event.pull_request.base.ref == 'production' && github.actor == 'OSBotify' && github.event.label.name == 'automerge' | ||
steps: | ||
# TODO: Once https://github.com/hmarr/auto-approve-action/pull/186 is merged, point back at the non-forked repo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'm not sure if just pointing at a forked repo like this will work, or if I need to checkout that repo in the workflow or publish the action in that repo or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this works just fine as-is: https://github.com/Andrew-Test-Org/Public-Test-Repo/pull/49/checks?check_run_id=2568980822
🎉 pascalgn/automerge-action#149 has been merged 🎉 |
Okay, taking this off hold. Let's do this thang. |
@deetergp dismissing the requested changes here since they were for minor refactors and we're going to be testing this today. Will follow up w/ any minor fixes. |
🚀 Deployed to staging in version: 1.0.43-6🚀
|
🚀 Deployed to production in version: 1.0.44-0🚀
|
Details
This pull request does a few things:
At an abstract level, it establishes a pattern by which we can make a "utility workflow" or "subroutine", which can be synchronously or asynchronously executed by another workflow. Nifty! Here's how it works:
OS_BOTIFY_TOKEN
instead of the defaultGITHUB_TOKEN
.Ultimately, this should promote code reuse and greatly simplify the handling of race conditions, the creation of new workflows, and the update of branches.
Now, specifically what this PR does:
Creates a new workflow called
createNewVersion
that:SEMVER_LEVEL
main
to update the app version.It's important to note that the
createNewVersion
workflow uses softprops/turnstyle to ensure that only one instance of the workflow is executing at a time. By doing so, it ensures that one version is created and committed tomain
before we allow the next version to be created.Updates the
bumpVersion
action to use the local version instead of tagsThe
bumpVersion
action previously used tags to prevent version conflicts due to race conditions. i.e: when a version was created we would immediately tag it and push the tags. Then when creating a new version we would fetch the tags, get the highest one, and do that + 1. The problem created by that method was that we could not "register" a new version without pushing tags, and pushing tags triggers a staging deploy.Now, we're using a "subroutine" and softprops/turnstyle to prevent race conditions, and we don't want to deploy to staging until the staging branch has been successfully updated. So I changed this action to once again just use the local version +1.
Replaces the
automerge
workflow with a newupdateProtectedBranch
subroutinestaging
can only be updated frommain
, andproduction
can only be updated fromstaging
createNewVersion
workflow, to prevent version conflict race conditions.automerge
workflow.Fixed Issues
Fixes https://github.com/Expensify/Expensify/issues/157916 https://github.com/Expensify/Expensify/issues/163576
Testing plan
StagingDeployCash
is not lockedBUILD
version. There should be no version conflict. A tag for each version should not be pushed yet.main
branch, followed by another PR to update thestaging
branch frommain
.staging
branch is updated, thedeploy
workflow should begin, create a tag for the updatedBUILD
version on thestaging
branch, and push tags.platformDeploy
workflow. When that workflow completes, a comment should appear on this and the dummy PR stating that it was deployed to staging.StagingDeployCash
.StagingDeployCash
, and thelockCashDeploys
workflow should begin.PATCH
version should be created.main
branch should be created and automerged, followed by a PR to update thestaging
branch.staging
is merged, thedeploy
workflow should begin, create a tag with the updatedPATCH
version, and push tags.platformDeploy
workflow.This PR was not deployed to staging...
.StagingDeployCash
.StagingDeployCash
checklist, and close it with the:shipit:
comment.finishReleaseCycle
workflow should begin, and a pull request withproduction
as the base andstaging
as the head should be created and automerged.production
is merged, thedeploy
workflow should start, and create a new GH release for thePATCH
version that was created in step 10.platformDeploy
workflow should begin a prod deploy. Once that completes, a comment should appear on this PR, the dummy PR, and all the PRs that were on the now-closedStagingDeployCash
stating that they were deployed to production.finishReleaseCycle
workflow should also bump theBUILD
version and create a PR to updatestaging
from main. This should result in a staging deploy.finishReleaseCycle
workflow should also create a newStagingDeployCash
issue, with theBUILD
version from the previous step.StagingDeployCash
.StagingDeployCash
(you wrote these down in step 15). The deploy blockers will no longer be deploy blockers, but should still be worked on with as much urgency as possible, as they will now be bugs on production.Testing Template
Merging regular PR (w/ race condition)
1.
2.
preDeploy
workflow run:updateProtectedBranch
workflow run:updateProtectedBranch
workflow run:deploy
workflow run:platformDeploy
workflow run:StagingDeployCash
updated with new PRs andBUILD
version?Locking
StagingDeployCash
lockCashDeploys
workflow run:updateProtectedBranch
workflow run:updateProtectedBranch
workflow run:StagingDeployCash
updated with newPATCH
version?Merging while
StagingDeployCash
is lockedpreDeploy
workflow:Prod deploy
finishReleaseCycle
workflow run:updateProtectedBranch
workflow run:deploy
workflow run (prod):StagingDeployCash
platformDeploy
workflow run (prod):StagingDeployCash
checklist?BUILD
version created?main
updated with newBUILD
version?updateProtectedBranch
workflow run:Staging
updated with newBUILD
version?updateProtectedBranch
workflow run:deploy
workflow run (staging):platformDeploy
workflow run (staging):StagingDeployCash
was locked now say "deployed to staging"?StagingDeployCash
created:BUILD
version?StagingDeployCash
was locked?