-
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] Skip tests, lint and deploys for unrelated changes #30315
Conversation
Part of the reason I did this PR was as an exploratory measure for #9725 – if we can skip unnecessary workflows that would run on large runners that will save us money in the form of GitHub Actions minutes |
I think you can run these tests against this PR before merging, can we do that? |
# Conflicts: # .github/workflows/preDeploy.yml
How? |
Push changes to |
A preview of your ExpensifyHelp changes have been deployed to https://15f7a4d6.helpdot.pages.dev ⚡️ |
This is now well-tested and working well |
The only thing that hasn't been live-tested is the new welcome message. However, I did do a fair amount of local testing on the gh cli stuff, which is prettymuch the only new thing. To see that in action locally:
read -r number author < <(gh pr list --search e51b21b19d3aa345f3fcf8f3080ae19138cd5ed8 --state merged --json 'number,author' | jq -r '.[0] | [.number, .author.login] | join(" ")')
echo "$number"
echo "$author" works ✅ #30099 |
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.
Love it, thank you for all the testing!
Reviewer Checklist
Screenshots/VideosWebN/A -Workflow test changes only Mobile Web - ChromeN/A -Workflow test changes only Mobile Web - SafariN/A -Workflow test changes only DesktopN/A -Workflow test changes only iOSN/A -Workflow test changes only AndroidN/A -Workflow test changes only |
✋ 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 staging by https://github.com/AndrewGable in version: 1.3.93-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.93-1 🚀
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 1.3.94-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/AndrewGable in version: 1.3.94-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.94-2 🚀
|
Details
This is a time optimization to skip workflows in certain scenarios:
js
,ts
,tsx
, orsh
files, because those are all the files we lintFixed Issues
$ n/a
Tests
.github
paths-ignore
Offline tests
n/a
QA Steps
none.
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)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)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