Fix wrong Provisioning Profile and add directories to desktop upload #13129

staszekscp · 2022-11-29T13:38:54Z

…ktop

Details

This PR fixes passing wrong provisioning profile for ios build_internal lane. Also during upload to S3 the desktop app should appear under desktop/$PULL_REQUEST_NUMBER directory to find it easier and attach it to the correct PR.

Fixed Issues

Passing wrong provisioning profile for ios build_internal lane

$ GH_LINK
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

Run Build and deploy apps for testing workflow. It must be triggered by mobile-deployers team member.
See if fastlane starts building the app correctly

Verify that no errors appear in the JS console

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

…ktop

melvin-bot · 2022-11-29T13:39:44Z

@aldo-expensify 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]

mountiny

The changes look good to me afaik, but I would wait for @AndrewGable and @roryabraham possible to have a look and review, they can also test once this is merged.

.github/workflows/testBuild.yml

mountiny

@staszekscp One comment

.github/workflows/testBuild.yml

mountiny

LGTM @roryabraham @AndrewGable all yours

AndrewGable · 2022-11-30T16:46:07Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

AndrewGable

Looks good - I will test after merge since we cannot test until then.

roryabraham · 2022-11-30T19:00:05Z

.github/workflows/testBuild.yml

@@ -206,6 +193,8 @@ jobs:
          "Use the links below to test this build in android and iOS. Happy testing!
          | android :robot:  | iOS :apple: | 
          | ------------- | ------------- |
-          | ${{fromJson(steps.set_var.outputs.android_paths).html_path}}  | ${{fromJson(steps.set_var.outputs.ios_paths).html_path}}  |"
+          | ${{fromJson(steps.set_var.outputs.android_paths).html_path}}  | ${{fromJson(steps.set_var.outputs.ios_paths).html_path}}  |
+          | ------------- | ------------- |


This ends up rendering kind of weird:

Suggested change

| ------------- | ------------- |

Also, why only Android and iOS instead of all platforms?

Thanks for the suggestion! We wanted to make sure that mobile part works first, and I am not 100% sure where exactly .dmg file is going to land on S3. I've updated the file, but there is a slight chance it is not going to be the right path. As per the web app there is a small problem with hosting multiple react apps in one bucket, it seems that it is not as easy as it seemed at the beginning.

roryabraham · 2022-11-30T19:01:07Z

.github/workflows/testBuild.yml

@@ -206,6 +193,8 @@ jobs:
          "Use the links below to test this build in android and iOS. Happy testing!


Let's add some more emoji 🙂

Suggested change

"Use the links below to test this build in android and iOS. Happy testing!

":test_tube::test_tube: Use the links below to test this build in android and iOS. Happy testing! :test_tube::test_tube:

roryabraham · 2022-11-30T19:01:57Z

config/electronBuilder.config.js

@@ -50,6 +59,7 @@ module.exports = {
                ? s3Bucket[process.env.ELECTRON_ENV]
                : 'ad-hoc-expensify-cash',
            channel: 'latest',
+            path: isCorrectElectronEnv ? s3Path[process.env.ELECTRON_ENV] : '/',


I wonder if we should just fail the Electron build early if process.env.ELECTRON_ENV is not set?

Yes, you may be right, if there isn't any process.env.ELECTRON_ENV it means that the command is not correct. I've removed those checks from the electronBuilder.config.js file

roryabraham · 2022-11-30T19:04:55Z

.github/workflows/testBuild.yml

@@ -140,39 +159,7 @@ jobs:
          APPLE_ID_PASSWORD: ${{ secrets.APPLE_ID_PASSWORD }}
          AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }}
          AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
-
-  # web:


I understand we're still in a testing phase, but why did you remove the web build?

It turned out that it is not so simple to host multiple react apps in on S3 bucket. It seems that we may configure buckets to host a couple of apps with some predefined urls, but we have to dive deeper to find a way to do it dynamically (as we want to keep it in a folder with PR's number). Uploading the web app is not a problem, hosting it unfortunately might be.

roryabraham

Have a few NAB comments. going to merge to let you keep testing, but let's address these minor things in a follow-up PR plz

roryabraham · 2022-12-01T23:12:20Z

.github/workflows/testBuild.yml

+          | android :robot:  | iOS :apple: | desktop :computer: |
+          | ------------- | ------------- | ------------- | 
+          | ${{fromJson(steps.set_var.outputs.android_paths).html_path}}  | ${{fromJson(steps.set_var.outputs.ios_paths).html_path}}  | https://ad-hoc-expensify-cash.us-east-1.amazonaws.com/desktop/${{github.event.number}}/NewExpensify.dmg
+          | ![Android](https://api.qrserver.com/v1/create-qr-code/?size=200x200&data=${{fromJson(steps.set_var.outputs.android_paths).html_path}}) | ![iOS](https://api.qrserver.com/v1/create-qr-code/?size=200x200&data=${{fromJson(steps.set_var.outputs.ios_paths).html_path}}) | ![desktop](https://ad-hoc-expensify-cash.us-east-1.amazonaws.com/desktop/${{github.event.number}}/NewExpensify.dmg)"


Suggested change

| ![Android](https://api.qrserver.com/v1/create-qr-code/?size=200x200&data=${{fromJson(steps.set_var.outputs.android_paths).html_path}}) | ![iOS](https://api.qrserver.com/v1/create-qr-code/?size=200x200&data=${{fromJson(steps.set_var.outputs.ios_paths).html_path}}) | ![desktop](https://ad-hoc-expensify-cash.us-east-1.amazonaws.com/desktop/${{github.event.number}}/NewExpensify.dmg)"

| ![Android](https://api.qrserver.com/v1/create-qr-code/?size=200x200&data=${{fromJson(steps.set_var.outputs.android_paths).html_path}}) | ![iOS](https://api.qrserver.com/v1/create-qr-code/?size=200x200&data=${{fromJson(steps.set_var.outputs.ios_paths).html_path}}) | ![desktop](https://ad-hoc-expensify-cash.us-east-1.amazonaws.com/desktop/${{github.event.number}}/NewExpensify.dmg)" |

I don't think this is technically required, but let's be sure to add it in the next PR please

Thanks, good catch, I haven't noticed this one! The fix is here: #13321

roryabraham · 2022-12-01T23:12:25Z

.github/workflows/testBuild.yml

+          ":test_tube::test_tube: Use the links below to test this build in android and iOS. Happy testing! :test_tube::test_tube:
+          | android :robot:  | iOS :apple: | desktop :computer: |
+          | ------------- | ------------- | ------------- | 
+          | ${{fromJson(steps.set_var.outputs.android_paths).html_path}}  | ${{fromJson(steps.set_var.outputs.ios_paths).html_path}}  | https://ad-hoc-expensify-cash.us-east-1.amazonaws.com/desktop/${{github.event.number}}/NewExpensify.dmg


Suggested change

| ${{fromJson(steps.set_var.outputs.android_paths).html_path}} | ${{fromJson(steps.set_var.outputs.ios_paths).html_path}} | https://ad-hoc-expensify-cash.us-east-1.amazonaws.com/desktop/${{github.event.number}}/NewExpensify.dmg

| ${{fromJson(steps.set_var.outputs.android_paths).html_path}} | ${{fromJson(steps.set_var.outputs.ios_paths).html_path}} | https://ad-hoc-expensify-cash.us-east-1.amazonaws.com/desktop/${{github.event.number}}/NewExpensify.dmg |

roryabraham · 2022-12-01T23:13:44Z

config/electronBuilder.config.js

 const macIcon = {
    production: './desktop/icon.png',
    staging: './desktop/icon-stg.png',
    internal: './desktop/icon-stg.png',
 };

-const isCorrectElectronEnv = ['production', 'staging', 'internal'].includes(


NAB but let's actually throw an error if we find that process.env.ELECTRON_ENV is not on of these?

OSBotify · 2022-12-01T23:20:22Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-12-02T19:43:30Z

🚀 Deployed to staging by @roryabraham in version: 1.2.36-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-12-08T02:03:33Z

🚀 Deployed to production by @francoisl in version: 1.2.36-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

fix wrong profile detection and distinguish directories in s3 for des…

a08eebe

…ktop

staszekscp requested a review from a team as a code owner November 29, 2022 13:38

melvin-bot bot requested review from aldo-expensify and removed request for a team November 29, 2022 13:39

staszekscp mentioned this pull request Nov 29, 2022

[Internal QA] Test mobile deployment #12859

Merged

98 tasks

mountiny requested a review from AndrewGable November 29, 2022 14:21

mountiny previously approved these changes Nov 29, 2022

View reviewed changes

AndrewGable requested changes Nov 29, 2022

View reviewed changes

.github/workflows/testBuild.yml Show resolved Hide resolved

add configure-aws-credentials to jobs

d51f934

staszekscp dismissed mountiny’s stale review via d51f934 November 29, 2022 18:07

roryabraham requested changes Nov 29, 2022

View reviewed changes

.github/workflows/testBuild.yml Show resolved Hide resolved

staszekscp added 6 commits November 30, 2022 09:55

extract configure-aws-credentials action to composite

d8841f0

add specific branch target

0eebd80

fix indentation

c9332f5

change branches wildcard name

4f0d046

fix fastfile error

b55f233

add qr codes to comment

0e9a16e

mountiny requested review from AndrewGable, roryabraham and mountiny November 30, 2022 15:23

mountiny reviewed Nov 30, 2022

View reviewed changes

.github/workflows/testBuild.yml Show resolved Hide resolved

mountiny previously approved these changes Nov 30, 2022

View reviewed changes

luacmartins previously approved these changes Nov 30, 2022

View reviewed changes

AndrewGable previously approved these changes Nov 30, 2022

View reviewed changes

roryabraham reviewed Nov 30, 2022

View reviewed changes

improve comment and make electron builder fail with wrong ELECTRON_ENV

47892bf

staszekscp dismissed stale reviews from AndrewGable and luacmartins via 47892bf December 1, 2022 07:08

staszekscp dismissed mountiny’s stale review via 47892bf December 1, 2022 07:08

roryabraham approved these changes Dec 1, 2022

View reviewed changes

roryabraham merged commit 5fe2645 into Expensify:main Dec 1, 2022

AndrewGable mentioned this pull request Dec 1, 2022

[No QA]Rename file to action.yml to enable testing #13254

Merged

47 tasks

OSBotify mentioned this pull request Dec 2, 2022

Deploy Checklist: New Expensify 2022-12-02 #13283

Closed

38 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wrong Provisioning Profile and add directories to desktop upload #13129

Fix wrong Provisioning Profile and add directories to desktop upload #13129

staszekscp commented Nov 29, 2022 •

edited by AndrewGable

Loading

melvin-bot bot commented Nov 29, 2022

mountiny left a comment

mountiny left a comment

mountiny left a comment

AndrewGable commented Nov 30, 2022 •

edited

Loading

AndrewGable left a comment

roryabraham Nov 30, 2022

roryabraham Nov 30, 2022

staszekscp Dec 1, 2022

roryabraham Nov 30, 2022

staszekscp Dec 1, 2022

roryabraham Nov 30, 2022

staszekscp Dec 1, 2022

roryabraham Nov 30, 2022

staszekscp Dec 1, 2022

roryabraham left a comment

roryabraham Dec 1, 2022

staszekscp Dec 5, 2022

roryabraham Dec 1, 2022

roryabraham Dec 1, 2022

OSBotify commented Dec 1, 2022

OSBotify commented Dec 2, 2022

OSBotify commented Dec 8, 2022

		@@ -206,6 +193,8 @@ jobs:
		"Use the links below to test this build in android and iOS. Happy testing!

	"Use the links below to test this build in android and iOS. Happy testing!
	":test_tube::test_tube: Use the links below to test this build in android and iOS. Happy testing! :test_tube::test_tube:

	\| ![Android](https://api.qrserver.com/v1/create-qr-code/?size=200x200&data=${{fromJson(steps.set_var.outputs.android_paths).html_path}}) \| ![iOS](https://api.qrserver.com/v1/create-qr-code/?size=200x200&data=${{fromJson(steps.set_var.outputs.ios_paths).html_path}}) \| ![desktop](https://ad-hoc-expensify-cash.us-east-1.amazonaws.com/desktop/${{github.event.number}}/NewExpensify.dmg)"
	\| ![Android](https://api.qrserver.com/v1/create-qr-code/?size=200x200&data=${{fromJson(steps.set_var.outputs.android_paths).html_path}}) \| ![iOS](https://api.qrserver.com/v1/create-qr-code/?size=200x200&data=${{fromJson(steps.set_var.outputs.ios_paths).html_path}}) \| ![desktop](https://ad-hoc-expensify-cash.us-east-1.amazonaws.com/desktop/${{github.event.number}}/NewExpensify.dmg)" \|

	\| ${{fromJson(steps.set_var.outputs.android_paths).html_path}} \| ${{fromJson(steps.set_var.outputs.ios_paths).html_path}} \| https://ad-hoc-expensify-cash.us-east-1.amazonaws.com/desktop/${{github.event.number}}/NewExpensify.dmg
	\| ${{fromJson(steps.set_var.outputs.android_paths).html_path}} \| ${{fromJson(steps.set_var.outputs.ios_paths).html_path}} \| https://ad-hoc-expensify-cash.us-east-1.amazonaws.com/desktop/${{github.event.number}}/NewExpensify.dmg \|

Fix wrong Provisioning Profile and add directories to desktop upload #13129

Fix wrong Provisioning Profile and add directories to desktop upload #13129

Conversation

staszekscp commented Nov 29, 2022 • edited by AndrewGable Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

PR Reviewer Checklist

Screenshots/Videos

melvin-bot bot commented Nov 29, 2022

mountiny left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

AndrewGable commented Nov 30, 2022 • edited Loading

Reviewer Checklist

Screenshots/Videos

AndrewGable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roryabraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Dec 1, 2022

OSBotify commented Dec 2, 2022

OSBotify commented Dec 8, 2022

staszekscp commented Nov 29, 2022 •

edited by AndrewGable

Loading

AndrewGable commented Nov 30, 2022 •

edited

Loading