[NoQA] Web job for the testBuild workflow#13354
Merged
Merged
Conversation
melvin-bot
Bot
requested review from
PauloGasparSv
and removed request for
a team
|
@PauloGasparSv Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
luacmartins
previously approved these changes
Dec 6, 2022
luacmartins
left a comment
luacmartins
left a comment
Contributor
There was a problem hiding this comment.
LGTM but I'd definitely like more eyes since these workflows aren't my strength
Contributor
|
Please do not merge - I need to confirm with the infra team this is how we want to set it up. |
Contributor
|
Is this P.R. ready for review? I saw some changes earlier today and no GH issue linked so I was waiting for an update to start reviewing. |
AndrewGable
changed the title
Contributor
|
@coleaeason said he would review the infra side of things, thank you @coleaeason ❤️ !! |
coleaeason
requested changes
Dec 6, 2022
| PULL_REQUEST_NUMBER: ${{ github.event.number }} | ||
|
|
||
| - name: Create CloudFront distribution for the PR if needed | ||
| run: ./.github/scripts/verifyActions.sh "$PULL_REQUEST_NUMBER" |
Contributor
There was a problem hiding this comment.
It looks like this is calling the wrong script? should this be the new script you added?
| #!/bin/bash | ||
| set -e | ||
|
|
||
| DISTRIBUTION_ID=$(echo aws cloudfront list-distributions --query "DistributionList.Items[?Origins.Items[?OriginPath=='/web/$1']].Id" --output text) |
Contributor
There was a problem hiding this comment.
echo likely isn't needed here?
Contributor
Author
There was a problem hiding this comment.
Removed, thank you, I missed this one!
|
|
||
| DISTRIBUTION_ID=$(echo aws cloudfront list-distributions --query "DistributionList.Items[?Origins.Items[?OriginPath=='/web/$1']].Id" --output text) | ||
|
|
||
| if [[ $(aws cloudfront list-distributions --query "DistributionList.Items[0].Id") != null ]] && [[ $DISTRIBUTION_ID ]] ; then |
Contributor
There was a problem hiding this comment.
is null a string? null doesn't exist in bash; so i think you mean "null"?
Also can you make the second item explicit? i.e. $DISTRIBUTION_ID != ""?
Contributor
Author
There was a problem hiding this comment.
Actually a string "null" is returned, that's why I was checking it. I've just updated the script by adding the "
| if [[ $(aws cloudfront list-distributions --query "DistributionList.Items[0].Id") != null ]] && [[ $DISTRIBUTION_ID ]] ; then | ||
| echo "Distribution for PR #$1 already exists! Invalidating cache..." | ||
| aws cloudfront create-invalidation --distribution-id $DISTRIBUTION_ID --paths '/' | ||
| exit 0; |
| exit 0; | ||
| else | ||
| echo "A new distribution for PR #$1 is being created" | ||
| echo $(aws cloudfront create-distribution --origin-domain-name ad-hoc-expensify-cash.s3.us-east-1.amazonaws.com --default-root-object index.html) >> cloudfront.config.json |
Contributor
There was a problem hiding this comment.
the echo and subprocess isn't needed here. Also why are we writing this to a file? why not:
CLOUDFRONT_CONFIG=$(aws cloudfront create-distribution --origin-domain-name ad-hoc-expensify-cash.s3.us-east-1.amazonaws.com --default-root-object index.html)| echo "A new distribution for PR #$1 is being created" | ||
| echo $(aws cloudfront create-distribution --origin-domain-name ad-hoc-expensify-cash.s3.us-east-1.amazonaws.com --default-root-object index.html) >> cloudfront.config.json | ||
|
|
||
| CONFIG=$(cat "./cloudfront.config.json") |
Contributor
There was a problem hiding this comment.
if you do my above suggestion you can delete this line
| echo $(aws cloudfront create-distribution --origin-domain-name ad-hoc-expensify-cash.s3.us-east-1.amazonaws.com --default-root-object index.html) >> cloudfront.config.json | ||
|
|
||
| CONFIG=$(cat "./cloudfront.config.json") | ||
| DISTRIBUTION_ID=$(echo $CONFIG | jq -r .Distribution.Id) |
Contributor
There was a problem hiding this comment.
nab, you can do:
jq -r .Distribution.Id <<< $CONFIG|
|
||
| CONFIG=$(cat "./cloudfront.config.json") | ||
| DISTRIBUTION_ID=$(echo $CONFIG | jq -r .Distribution.Id) | ||
| ETAG=$(echo $CONFIG | jq -r .ETag) |
Contributor
There was a problem hiding this comment.
same here, use a herestring
| DISTRIBUTION_ID=$(echo $CONFIG | jq -r .Distribution.Id) | ||
| ETAG=$(echo $CONFIG | jq -r .ETag) | ||
|
|
||
| echo $CONFIG | jq -r .Distribution.DistributionConfig >> dist.config.json |
Contributor
There was a problem hiding this comment.
why are we saving a json file here? Can't we just use a bash variable?
Contributor
Author
There was a problem hiding this comment.
aws cloudfront update-distribution command requires sending a file with the config, that's why I decided to create it
|
|
||
| tmp=$(mktemp /tmp/tmp.XXXXXXX) | ||
| NEW_ORIGIN_PATH=$(echo "/web/$1") | ||
| jq --arg originPath "$NEW_ORIGIN_PATH" '(.Origins.Items[] | select(.OriginPath == "")).OriginPath |= $originPath' dist.config.json > "$tmp" && mv "$tmp" dist.config.json |
Contributor
There was a problem hiding this comment.
I'm not sure I understand why we need this tmp file?
Contributor
Author
There was a problem hiding this comment.
As far as I know jq can't edit files, that's why I create a temporary file in order to make a change
Contributor
|
NAB but I put up a PR here to lint bash scripts in this repo with ShellCheck, and it might be beneficial to make sure that the script you created here is passing ShellCheck using: npx shellcheck -e SC1091 .github/scripts/createCloudfrontDistribution.sh |
Contributor
|
Apologies, we believe we are going to go a different direction and use Cloudflare workers instead of Cloudfront as mentioned here: https://expensify.slack.com/archives/C04878MDF34/p1671061957278119 @coleaeason will circle back soon with how this is all going to tie together, thank you for your patience! |
coleaeason
requested changes
Dec 15, 2022
Comment on lines
+221
to
+223
| - name: Create CloudFront distribution for the PR if needed | ||
| run: ./.github/scripts/createCloudfrontDistribution.sh "$PULL_REQUEST_NUMBER" | ||
|
|
Contributor
There was a problem hiding this comment.
As Andrew mentioned, we are going to remove Cloudfront form this setup, so you can remove this step.
| @@ -0,0 +1,24 @@ | |||
| #!/bin/bash | |||
Contributor
There was a problem hiding this comment.
This can also be removed now
Comment on lines
+252
to
+257
| - name: Get CloudFront URL | ||
| id: get_cloudfront_url | ||
| run: | | ||
| cloudfront_url=$(aws cloudfront list-distributions --query "DistributionList.Items[?Origins.Items[?OriginPath=='/web/$PULL_REQUEST_NUMBER']].DomainName" --output text) | ||
| echo "cloudfront_url=$cloudfront_url" >> "$GITHUB_OUTPUT" | ||
|
|
Contributor
There was a problem hiding this comment.
This can also be removed.
| | .html_path}}) | .html_path}}) |  |" | ||
| | android :robot: | iOS :apple: | desktop :computer: | web :spider_web: | | ||
| | ------------- | ------------- | ------------- | ------------- | | ||
| | ${{fromJson(steps.set_var.outputs.android_paths).html_path}} | ${{fromJson(steps.set_var.outputs.ios_paths).html_path}} | https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/$PULL_REQUEST_NUMBER/NewExpensify.dmg | https://${{steps.get_cloudfront_url.outputs.cloudfront_url}} | |
Contributor
There was a problem hiding this comment.
The URL now is:
https://<PR_NUMBER>.pr-testing.expensify.com
Contributor
Author
|
@coleaeason Thanks for the info, I've just applied the changes! |
AndrewGable
changed the title
coleaeason
approved these changes
Dec 16, 2022
coleaeason
left a comment
coleaeason
left a comment
Contributor
There was a problem hiding this comment.
LGTM -- Not sure why checks are failing. @AndrewGable ?
Contributor
Reviewer Checklist
Screenshots/VideosN/A these changes only affect GH actions. WebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
Contributor
|
@staszekscp we've updated the author checklist. Could you please update your checklist accordingly? You can find the latest checklist in this template |
luacmartins
approved these changes
Dec 16, 2022
Contributor
Author
|
@luacmartins Done! |
AndrewGable
approved these changes
Dec 16, 2022
Contributor
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Contributor
Author
|
@AndrewGable @luacmartins @coleaeason The link works, that is cool! Great job guys! https://13354.pr-testing.expensify.com/ I've just noticed something - if the QR code doesn't work, here's the fix: #13673 |
Contributor
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
Contributor
|
Nice! Awesome job @staszekscp! |
Contributor
|
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪 |
34 tasks
Contributor
|
🚀 Deployed to staging by @AndrewGable in version: 1.2.42-0 🚀
|
mountiny
changed the title
Contributor
|
🚀 Deployed to production by @yuwenmemon in version: 1.2.42-2 🚀
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
The PR adds a job to build the web testing app in the
testBuildworkflowFixed Issues
$ GH_LINK
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android