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

[No QA] Add deployer GH handle to deploy messages #4274

Merged
merged 3 commits into from
Aug 11, 2021

Conversation

roryabraham
Copy link
Contributor

@AndrewGable Will you please review this?

Details

This adds the deployer's GH handle to deploy messages, where the deployer is defined as:

  1. For production deploys, the person who closed the checklist to trigger the deploy.
  2. For normal staging deploys, the person who merged the PR.
  3. For "automatic" CPs (using the CP Staging label), the person who merged the PR.
  4. For manual CPs using the GH UI, the person who triggered the workflow. (We are going to save this by including the actor's GH handle in the CP branch name.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/168264

Tests

  1. Ensure that the deploy checklist is unlocked.
  2. Merge this PR, and after the staging deploy completes, verify that it gets a "Deployed to staging by <whoever merged it>" comment.
  3. Lock the deploy checklist.
  4. Create and merge another PR with the CP Staging label. Verify that it gets a "Cherry-picked to staging by <whoever merged it>" comment.
  5. Manually CP a PR using the GH UI. For testing purposes, the person who manually CPs the PR should be different than the person who merged it. After the staging deploy completes. verify that it gets a "Cherry-picked to staging by <whoever manually triggered the CP>" comment.

QA Steps

None.

Tested On

n/a, GH only. But I did do a lot of incremental testing using a local octokit instance while developing.

@roryabraham roryabraham requested a review from AndrewGable July 28, 2021 17:33
@roryabraham roryabraham requested a review from a team as a code owner July 28, 2021 17:33
@roryabraham roryabraham self-assigned this Jul 28, 2021
@MelvinBot MelvinBot requested review from luacmartins and removed request for a team July 28, 2021 17:33
Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

Code LGTM! Couldn't test locally, so I'm interested to see how that goes.

@@ -128,7 +128,7 @@ jobs:
# Version: 2.4.3
uses: repo-sync/pull-request@65194d8015be7624d231796ddee1cd52a5023cb3
with:
source_branch: cherry-pick-staging-${{ github.event.inputs.PULL_REQUEST_NUMBER }}
source_branch: ${{ github.actor }}-cherry-pick-staging-${{ github.event.inputs.PULL_REQUEST_NUMBER }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no way to get this or pass this along a different way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe via the API to check for a workflow https://docs.github.com/en/rest/reference/actions#list-workflow-runs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree this feels hacky, but the difficult part with using this API is identifying the workflow run that's associated with a given PR or version. Let me dig into it a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, the information we have when we're running markPullRequestsAsDeployed is:

  1. The version
  2. The PR number

Then given those, we would need to find the cherryPick.yml workflow run that:

  1. Generated that version, OR
  2. Was executed with that PR number as input.

But unfortunately the inputs to a workflow run are not saved by their API, so I wasn't able to see how we could identify the correct workflow run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to suggestions if you see something I'm missing

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see anything on the API docs on first read, but let me keep looking a bit further.

@AndrewGable
Copy link
Contributor

Looks great- Feel free to merge when you are ready to test @roryabraham

@roryabraham
Copy link
Contributor Author

Looks like we missed the chance to test this while the current checklist was still unlocked 😞 Gotta test it during the next deploy cycle. @isagoico can you ping or DM me before adding the lock label to the next checklist? Thanks!

@roryabraham
Copy link
Contributor Author

Testing this out now

@roryabraham roryabraham merged commit a07a28d into main Aug 11, 2021
@roryabraham roryabraham deleted the Rory-ManualCPAuditTrail branch August 11, 2021 17:21
@roryabraham
Copy link
Contributor Author

Unfortunately it looks like this didn't work as expected https://github.com/Expensify/App/runs/3303693182?check_suite_focus=true

@roryabraham
Copy link
Contributor Author

roryabraham commented Aug 11, 2021

We've got one successful test for a PR merged w/ the CP Staging label: #4568 (comment)

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @roryabraham in version: 1.0.83-5 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 cancelled 🔪
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@roryabraham
Copy link
Contributor Author

Okay, just verified over here that this is fully functional now

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @isagoico in version: 1.0.85-9 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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