-
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
Add PR number badge for internal builds #16735
Add PR number badge for internal builds #16735
Conversation
@francoisl 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] |
.github/workflows/testBuild.yml
Outdated
@@ -71,6 +71,9 @@ jobs: | |||
with: | |||
ref: ${{ github.event.pull_request.head.sha || needs.getBranchRef.outputs.REF }} | |||
|
|||
- name: Add PULL_REQUEST_NUMBER to .env.staging file |
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 can appreciate the simplicity of this but I wonder if creating a .env.adhoc
is more clear?
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 took that into consideration, but quoting Rory from this discussion :)
we could create a separate environment file for ad-hoc builds. The downside is that we'd be introducing a new environment file that we have to maintain, and that environment file would be different from the staging environment. In general I think we want these ad-hoc builds to be as similar to staging builds as possible.
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.
@roryabraham - Curious for your thoughts here now that we have a PR up. My sense is adhoc is not a ton to maintain and it's more explicit as we now have two or three different terms to describe "adhoc" "pull request" "internal testing", etc.
@AndrewGable could you re-run the build workflow? The previous one used the workflow from |
I can probably checkout the branch, then push my own version? |
Yep, I think that would be the easiest solution! |
Ran here https://github.com/Expensify/App/actions/runs/4566142454 Used |
@AndrewGable There was a tiny bug I haven't spotted before. The |
Hey @AndrewGable! Could you rerun the workflow to see if everything is ok? :) |
Yes, will do. Updated |
Hi @AndrewGable! Thanks for running the workflow, it seems that everything works as expected :) |
@AndrewGable, sorry about the About page, I didn't know it has to be changed there too. It won't be a problem, I'll fix it tomorrow. If it comes to the badge on the screen with logging in I can change it. I just want to inform that for now it is an |
I think no splash screen is fine for now, I can get you a replacement SVG for AdHoc that looks similar to the Staging one. |
@AndrewGable That would be great! |
@Expensify/design or @ADeWitt - Would it be possible to get https://github.com/Expensify/App/blob/main/assets/images/new-expensify-dev.svg in another variant? Ideally it would say "AdHoc", if that does not fit, maybe we could do "PR"? The "pill" color could be a different color if we have another brand color to use, otherwise we can use the "Dev" color. |
Do we need that for all of the icon possibilities? Like the desktop icon as well? |
Maybe we can simplify the CI input labels? Even with context I've messed up twice now. New run: https://github.com/Expensify/App/actions/runs/4734387668 |
Hi @Julesssss, thanks for the run! I've just merged the newest |
The last commit should fix the issue, hopefully it's the last one :) |
Thanks, triggered another build here |
Ok, android is already downloadable, and the fix worked. :) I've probably installed the previous version last time, and I haven't spotted the difference :/ |
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪 |
@Julesssss all of them seem to be working fine now 😄 |
That is strange, on my iPhone everything works just fine... I've just double checked it to make sure. Could you try a bit later? |
Yeah, it's weird. It works fine for me still on Android. Anyway, good news... I was able to verify on a simulator, so I think we're good to go. |
npm has a |
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
@AndrewGable maybe you wanna review again |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@staszekscp @AndrewGable We have no easy way to confirm variety of build for this PR. Do you mind QA-ing it internally? |
Yes, you can mark it off. We tested the steps for this PR before merging. |
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.3-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.3-2 🚀
|
Details
This PR customises the environmental badge for internal builds. The internal builds are going to run on staging environment, but there will be a badge with information from which PR the app was built.
The PR doesn't include screenshots, since a real internal builds would have to be checked to test it properly.
@AndrewGable Could you run the
testBuild
workflow against this PR to see if everything is fine?Fixed Issues
$ #13993
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android