-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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] Fix too-big checklists #31458
Conversation
# Conflicts: # .github/workflows/deployBlocker.yml # .github/workflows/platformDeploy.yml
# Conflicts: # .github/workflows/deployBlocker.yml # tests/unit/CIGitLogicTest.sh
No C+ review needed here |
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 @roryabraham for digging into this one, this is last thing holding the deploy, the changes make sense to me and automated tests have been added, I will go ahead and merge this to get the deploy going sooner than later.
@@ -6,27 +6,19 @@ on: | |||
- labeled | |||
|
|||
jobs: | |||
updateChecklist: | |||
if: github.event.label.name == 'DeployBlockerCash' | |||
uses: ./.github/workflows/createDeployChecklist.yml |
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 think it would be cleaner to call this create or update too
uses: ./.github/workflows/createDeployChecklist.yml | |
uses: ./.github/workflows/createOrUpdateDeployChecklist.yml |
Had to resolve merge conflicts |
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.
Reviewing @mountiny's addition
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.1-13 🚀
|
Details
This PR should hopefully fix #27123, but includes a few changes:
--shallow-exclude
for both thefromTag
and thetoTag
(see testing details for why I think this change will fix the issue).createDeployChecklist.yml
. That way, we can re-create or refresh the checklist whenever we want.createOrUpdateStagingDeploy
JS action by removing theNPM_VERSION
input. It always just needs to compare the most recent version to the previous version on the checklist (previous checklist in the case of a production deploy / new checklist, current checklist in the case of updating an existing checklist)CIGitLogicTest.sh
and adds a new test case w/ rebase + force-push.Fixed Issues
$ #27123
Tests
Slack context: https://expensify.slack.com/archives/C07J32337/p1700246543029419
The background here is bad workflow runs that cause deploy checklists to include PRs from the previous checklist. For example this one. In that example, #31066 was originally cherry-picked in
1.3.96-15
. So it should not appear in this log:git log --oneline --grep "Merge pull request #" 1.3.97-7...1.3.98-0
But in the workflow run, it does. In order to reproduce the problem locally, do the following:
Now, if you run the following:
Offline tests
None.
QA Steps
None – monitor checklist creation and verify that they no longer contain old PRs.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop