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

Comment on deploy checklist when a pull request is CP'd #7802

Merged
merged 10 commits into from
Feb 23, 2022

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Feb 17, 2022

Details

  1. Comments on the deploy checklist like so when a pull request is CP'd:


    👏 Heads up @Expensify/applauseleads 👏

    A new pull request has been 🍒 cherry-picked 🍒 to staging, and will be deployed to staging in version 1.1.39-2 🚀


  2. Then waits for the staging deploy for that version to complete

  3. then comments again like so:


    🚀 All set?…You bet! https://github.com/Expensify/App/releases/tag/1.1.39-2 has been deployed to staging 🚀


Fixed Issues

$ #7178

Tests

Unit tests added.

QA Steps

  1. Merge a PR with the CP Staging label with the checklist locked.
  2. Verify that the first comment appears on the checklist and is correctly formatted.
  3. Verify that the staging deploy completes, then the second comment appears on the checklist and is correctly formatted.
  4. Merge another PR without the CP Staging label with the checklist locked.
  5. Verify that no comments appear on the checklist.
  6. Manually CP the PR.
  7. Repeat steps 2 and 3.

@roryabraham roryabraham self-assigned this Feb 17, 2022
@roryabraham
Copy link
Contributor Author

This failure looks like it might be a bug in action-lint. Going to check it out more tomorrow.

@roryabraham roryabraham marked this pull request as ready for review February 18, 2022 22:33
@roryabraham roryabraham requested a review from a team as a code owner February 18, 2022 22:33
@roryabraham roryabraham added the InternalQA This pull request required internal QA label Feb 18, 2022
@MelvinBot MelvinBot requested review from tylerkaraszewski and removed request for a team February 18, 2022 22:33
@github-actions
Copy link
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@roryabraham
Copy link
Contributor Author

cc @mountiny in case you want to review as well since you've taken an interesting in our GitHub Actions. Don't want to hold on that because I know you're only working ~30% right now.

@roryabraham
Copy link
Contributor Author

Only added CP Staging so we can start QA upon merge.

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Thank you for the ping indeed, I am very happy to learn from how others will review it too. It is still interesting to me how much we can do through the actions.

I did not see any problems with the code and the unit tests probably show it works well. Can't say much about best practices yet.

@AndrewGable
Copy link
Contributor

Feel free to merge when e2e is done 👍

@roryabraham roryabraham merged commit 9c75ded into main Feb 23, 2022
@roryabraham roryabraham deleted the Rory-AlertApplauseLeadsForCP branch February 23, 2022 00:23
@MelvinBot
Copy link

Triggered auto assignment to @luacmartins (InternalQA), see https://stackoverflow.com/c/expensify/questions/5042 for more details.

@botify
Copy link

botify commented Feb 23, 2022

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

@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by @roryabraham in version: 1.1.39-3 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@roryabraham
Copy link
Contributor Author

Not sure why it said it was merged without passing tests ... pretty sure I waited for tests to pass first 🤔

@luacmartins
Copy link
Contributor

Got assigned QA, @roryabraham is there a proactive way of testing this or should I just monitor PRs that are merged with the CP staging label/manually CP'd?

@timszot
Copy link
Contributor

timszot commented Feb 23, 2022

I would say this works just fine per this comment that happened on the current checklist.

@luacmartins
Copy link
Contributor

Ah nice once @timszot! Yea, seems to be working as intended!

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @timszot in version: 1.1.39-3 🚀

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

@luacmartins
Copy link
Contributor

Checking this off since it was already deployed in Feb!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
InternalQA This pull request required internal QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants