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

Move UpdateStagingDeployCash to after when tags are pushed #1869

Merged
merged 2 commits into from
Mar 18, 2021

Conversation

roryabraham
Copy link
Contributor

Broken workflow: https://github.com/Expensify/Expensify.cash/runs/2143576420?check_suite_focus=true

What happened: we tried to use a git log command with a tag that was not yet created.

This PR should introduce a temporary fix (because eventually we'll want to only push tags in the deploy workflow, not the preDeploy workflow. Long-term, the fix would probably be to just create the tag locally in the preDeploy workflow but never push it and let it die with the runner.

@roryabraham roryabraham self-assigned this Mar 18, 2021
@roryabraham roryabraham requested a review from a team as a code owner March 18, 2021 21:35
@botify botify requested review from Luke9389 and removed request for a team March 18, 2021 21:35
@roryabraham
Copy link
Contributor Author

Interestingly, I think this bug would always happen if a PR is merged to master and the preDeploy workflow executes while there's no open StagingDeployCash. Technically, there should always be an open StagingDeployCash, except for the window of time when the finishDeployCycle workflow is running. But it's possible that the preDeploy and finishDeployCycle workflows could happen at the same time, so we'll want to be aware of this and make sure we fix this bug when we move the git push --tags from preDeploy to deploy.

Luke9389
Luke9389 previously approved these changes Mar 18, 2021
@Luke9389
Copy link
Contributor

Ok, I'm not entirely following what happened here, but I'm assuming you cc'ed Andrew bc he has more context on the problem. If that's not the case, I think you should give more background info.

Also, is there nothing we can do to test this? I know it's tricky bc we're dealing with the deploy process.

@roryabraham
Copy link
Contributor Author

Haha yeah @Luke9389 yeah there's prettymuch no way to locally test our deploy workflows. 😅 We do what we can to test stuff in different repos and using automated tests for the JS, but it only goes so far. As a result there are 1,000,000 of these quick patch PRs so it would take a ton of time to provide full context on each of them. But basically:

  • The preDeploy workflow executes when code is pushed to master (a PR is merged)
  • As part of that workflow, we typically execute a custom Node.js script called createOrUpdateStagingDeploy to add the newly-merged PR to a deploy tracking issue called a StagingDeployCash
  • Under normal circumstances, there is always a StagingDeployCash open. But there is an edge case where that isn't always true, and while we're building this out we got in a weird state that shouldn't normally happen.
  • If there's not an open StagingDeployCash, then we try to create a new one. As part of that process, we look up the previous one, and generate a URL that compares changes between the latest app version on master and the final version of the previous StagingDeployCash. That code is here, and it relies on tags and the git log shell command.
  • In this case, it failed because we hadn't yet created a tag for the new version. So all we need to do to resolve that is create a the tag first, then create the StagingDeployCash second.

And that's what this PR does 🙃

@AndrewGable
Copy link
Contributor

Merging before e2e to hopefully un break deploys!

@AndrewGable AndrewGable merged commit 1dc4201 into master Mar 18, 2021
@AndrewGable AndrewGable deleted the Rory-FixUpdateStagingDeployCash branch March 18, 2021 22:04
@botify
Copy link

botify commented Mar 18, 2021

@AndrewGable looks like this was merged without passing tests. Please add a note explaining why this was done or remove the Emergency label if this is not an emergency.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 18, 2021
@roryabraham
Copy link
Contributor Author

First preDeploy workflow: https://github.com/Expensify/Expensify.cash/actions/runs/666051963
Version Bump PR against master: #1871
Master automerge workflow: https://github.com/Expensify/Expensify.cash/runs/2143908229?check_suite_focus=true
Second preDeploy workflow: https://github.com/Expensify/Expensify.cash/actions/runs/666059370
Update staging PR: #1872
Staging automerge workflow: https://github.com/Expensify/Expensify.cash/runs/2143920797?check_suite_focus=true ❌ didn't merge

Staging deploy happened ✅
image

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants