-
Notifications
You must be signed in to change notification settings - Fork 3k
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] Tag staging instead of main before updating checklist #5013
Conversation
Also cc @Jag96 since I know you've worked on this issue before. |
.github/workflows/preDeploy.yml
Outdated
@@ -105,6 +101,13 @@ jobs: | |||
WORKFLOW: cherryPick.yml | |||
INPUTS: '{ "PULL_REQUEST_NUMBER": "${{ needs.chooseDeployActions.outputs.mergedPullRequest }}", "NEW_VERSION": "${{ env.NEW_VERSION }}" }' | |||
|
|||
# Note: we need to create this tag but not push it, because GitUtils.getPullRequestsMergedBetween uses tags to generate a list of merged pull requests |
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.
Thanks for working on this comment! I think there is still some room for improvement though. It needs to explain why the tag isn't being pushed, and I think it should also explain how tags are used in getPullRequestsMergedBetween. Something like this:
Create a tag for this version so that GitUtils.getPullRequestsMergedBetween can generate a list of merged pull requests that happened between two release version tags. It's not necessary to push this tag because... XYZ.
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 great feedback! One of my goals for the GHA code is to make it more approachable to new contributors, and there are definitely a few "gotchas" hiding in here. So it's definitely good to explain this one in more detail :)
I updated the comment, let me know what you think!
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 see 2 other places where the old comment still exists, can you update those as well?
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.
Updated again!
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.
Ah, that's a much better comment. Thanks! I like the mention of the trigger for the staging deploy. It made me think of another question though...
What happens to the local tag? Does the tag get pushed as part of the staging deploy, or does the tag just remain on the local repo and never get used again?
Maybe instead of saying:
It's important not to push this tag now
It is better to say
Don't push this tag to the remote now because it will be pushed when the staging deploy is triggered in the deploy.yml workflow.
Updated my previous copy suggestion |
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.
Thanks for that last change!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to production by @yuwenmemon in version: 1.0.94-1 🚀
|
Details
Right here we're pulling
main
and creating a tag with the new version. Then in the next two steps we:However, the tag created from
main
might include PRs that were merged but not deployed to staging. For example:main
, and includes PR 2 in its history. But PR 2 was not deployed to staging ❌This PR should fix this issue by tagging
staging
instead ofmain
before updating the checklist.Fixed Issues
Hopefully might (at least partially) fix https://github.com/Expensify/Expensify/issues/168349
Tests
QA Steps
None.