-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Flask] bitrise pipelines #6645
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
5aa5768
to
f621274
Compare
@@ -8,7 +8,12 @@ FILE=./android/app/build/outputs/apk/qa/release/app-qa-release.apk | |||
if test -f "$FILE"; then | |||
shasum -a 512 "$FILE" > ./android/app/build/outputs/apk/qa/release/sha512sums.txt | |||
fi | |||
elif [ "$MODE" == "Flask" ]; then | |||
FILE=./android/app/build/outputs/apk/flask/release/app-flask-release.apk |
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.
Note: Is this the correct 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.
should be
scripts/build.sh
Outdated
echo "Remapping flask env variable names to match production" | ||
|
||
# js.env variables | ||
remapEnvVariable "MM_FLASK_PUBNUB_SUB_KEY" "MM_PUBNUB_SUB_KEY" |
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.
Remove PubNub keys
bitrise.yml
Outdated
- pipeline_intermediate_files: ios/build/output/MetaMask-QA.ipa:BITRISE_APP_STORE_IPA_PATH | ||
- deploy_path: ios/build/output/MetaMask-QA.ipa | ||
- pipeline_intermediate_files: ios/build/output/MetaMask.ipa:BITRISE_APP_STORE_IPA_PATH | ||
- deploy_path: ios/build/output/MetaMask.ipa |
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.
This will be MetaMask-Flask.ipa
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 accidentally edited the QA steps here. I will remove these.
bitrise.yml
Outdated
is_always_run: false | ||
is_skippable: true | ||
inputs: | ||
- deploy_path: ios/build/MetaMask.xcarchive |
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.
Same name update here
bitrise.yml
Outdated
inputs: | ||
- deploy_path: ios/build/MetaMask-QA.xcarchive | ||
inputs: | ||
- pipeline_intermediate_files: ios/build/output/MetaMask.ipa:BITRISE_APP_STORE_IPA_PATH |
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.
same here
bitrise.yml
Outdated
is_always_run: false | ||
is_skippable: true | ||
inputs: | ||
- deploy_path: ios/build/MetaMask.xcarchive |
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.
same here
a6a090c
to
56f03d4
Compare
@@ -671,6 +690,115 @@ workflows: | |||
- pipeline_intermediate_files: sourcemaps/ios/index.js.map:BITRISE_APP_STORE_SOURCEMAP_PATH | |||
- deploy_path: sourcemaps/ios/index.js.map | |||
title: Deploy Source Map | |||
build_ios_flask_release: |
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.
C: I wonder if we have a way to prevent duplication of these big yaml blocks and just have one called with params. they really mostly look the same.
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.
@NicolasMassart regarding the sourcemap comment, yes they will be different. I am wondering if I need to change the sourcemap logic since it is failing on bitrise
app/util/sentryUtils.js
Outdated
const environment = | ||
__DEV__ || !METAMASK_ENVIRONMENT ? 'development' : METAMASK_ENVIRONMENT; | ||
__DEV__ || !METAMASK_ENVIRONMENT |
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.
Can you add tests with explicit output and comments? This thing is getting out of hand.
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.
On extension we found it easier to disable Sentry entirely in development builds (see https://github.com/MetaMask/metamask-extension/blob/230c0c6fa1626bc1afd68c22321f5c166425dd12/app/scripts/lib/setupSentry.js#L88). Most of our Sentry testing was done with QA or production-like builds as part of regression testing. On the rare occasion that we wanted to test Sentry with a development build, it's easy to comment out that line.
That would reduce this condition to a single ternary:
const environment = METAMASK_BUILD_TYPE === 'main' ?
METAMASK_ENVIRONMENT :
`${METAMASK_ENVIRONMENT}-${METAMASK_BUILD_TYPE}`;
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.
If we wanted to go that route here, and disable Sentry for development builds, that would be simpler to do in a separate PR. Better to keep this one focused if we can.
A more direct solution to cleaning this up for now would be to split this into two steps: determine the environment, then the sentry environment. e.g.
const metamaskEnvironment = __DEV__ || !METAMASK_ENVIRONMENT ? 'development' : METAMASK_ENVIRONMENT;
const sentryEnvironment = METAMASK_BUILD_TYPE === 'main' ?
METAMASK_ENVIRONMENT :
`${METAMASK_ENVIRONMENT}-${METAMASK_BUILD_TYPE}`;
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 have went ahead and modified this logic to live inside a function I am calling deriveSentryEnvironment
.
/**
* Derives the Sentry environment based on input parameters.
* This function is similar to the environment logic used in MetaMask extension.
* - https://github.com/MetaMask/metamask-extension/blob/34375a57e558853aab95fe35d5f278aa52b66636/app/scripts/lib/setupSentry.js#L91
*
* @param {boolean} isDev - Represents if the current environment is development (__DEV__ global variable).
* @param {string} [metamaskEnvironment='local'] - The environment MetaMask is running in
* (process.env.METAMASK_ENVIRONMENT).
* It defaults to 'local' if not provided.
* @param {string} [metamaskBuildType='main'] - The build type of MetaMask
* (process.env.METAMASK_BUILD_TYPE).
* It defaults to 'main' if not provided.
*
* @returns {string} - The Sentry environment. Possible values are 'development', 'local',
* 'production', or a string in the format `${metamaskEnvironment}-${metamaskBuildType}`.
* 'development' is returned if 'isDev' is true or 'metamaskEnvironment' is not provided.
* 'metamaskEnvironment' is returned if 'metamaskBuildType' is 'main' or undefined.
* `${metamaskEnvironment}-${metamaskBuildType}` is returned for other cases,
* for example 'production-flask' or 'debug-flask'.
*/
export function deriveSentryEnvironment(
isDev,
metamaskEnvironment = 'local',
metamaskBuildType = 'main',
) {
const environment =
isDev || !metamaskEnvironment
? 'development'
: metamaskBuildType === 'main'
? metamaskEnvironment
: `${metamaskEnvironment}-${metamaskBuildType}`;
return environment;
}
This function is well tested in the new sentryUtils.test.ts
file. I opted to not split up the meta mask environment and sentry environment to keep continuity with what we have already published in sentry. what would be the use case for having a separate metamaskEnvironment
if we don't log it in sentry?
app/util/sentry/sentryUtils.js
Outdated
metamaskEnvironment = 'local', | ||
metamaskBuildType = 'main', | ||
) { | ||
const environment = |
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.
Nit: Thoughts on splitting this up onto multiple statements? The standard metamask ESLint config (not yet used here) disallows nested ternaries. They're a bit hard to read.
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 have broken it up into multiple return statements in my latest commit.
app/util/sentry/sentryUtils.js
Outdated
* (process.env.METAMASK_BUILD_TYPE). | ||
* It defaults to 'main' if not provided. | ||
* | ||
* @returns {string} - The Sentry environment. Possible values are 'development', 'local', |
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.
Nit: it might be better to include less specifics here, so we don't have to update it each time we add a new environment
app/util/sentry/sentryUtils.js
Outdated
) { | ||
const environment = | ||
isDev || !metamaskEnvironment | ||
? 'development' |
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.
Should we include the build type even for development builds? Seems like it could be useful
It's not yet clear to me whether the metric events are being flagged as being from Flask rather than from the main release. This is quite important. We rely on those metrics a lot, so we need to ensure we know precisely what they mean. I'm not familiar with how metrics are setup on mobile at the moment, so I'm not sure how to approach this. |
a985c90
to
37aeb8a
Compare
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is filesystem access?Accesses the file system, and could potentially read sensitive data. If a package must read the file system, clarify what it will read and ensure it reads only what it claims to. If appropriate, packages can leave file system access to consumers and operate on data passed to it instead. What is a mild CVE?Contains a low severity Common Vulnerability and Exposure (CVE). Remove or replace dependencies that include known low severity CVEs. Consumers can use dependency overrides or npm audit fix --force to remove vulnerable dependencies. What are unmaintained packages?Package has not been updated in more than a year and may be unmaintained. Problems with the package may go unaddressed. Package should publish periodic maintenance releases if they are maintained, or deprecate if they have no intention in further maintenance. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
@@ -128,14 +126,16 @@ | |||
<key>test</key> | |||
<string>$(MM_BRANCH_KEY_TEST)</string> | |||
</dict> | |||
<key>branch_universal_link_domains</key> |
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.
we should be adding the iOS bundle identifier and android apk hash to branch or deep linking won't work
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 will work with Andrea to get this working after we submit the builds.
scripts/build.sh
Outdated
|
||
# js.env variables`` | ||
remapEnvVariable "FLASK_MOONPAY_API_KEY_STAGING" "MOONPAY_API_KEY_STAGING" | ||
remapEnvVariable "SEGMENT_FLASK_DEV_KEY" "SEGMENT_DEV_KEY" |
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.
format error
Development & PR Process
release-xx
label to identify the PR slated for a upcoming release (will be used in release discussion)needs-dev-review
label when work is completedneeds-qa
label when dev review is completedQA Passed
label when QA has signed offDescription
Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions,
1. What is the reason for the change?
2. What is the improvement/solution?
"Invalid App Store Icon. The App Store Icon in the asset catalog in 'MetaMask-Flask.app' can't be transparent nor contain an alpha channel. (ID: 9e93a511-109f-43f3-b3a5-a80ee40734cd)";
when I tried to upload to testflight so I needed to modify the ios app icons to remove the alpha.Test Builds
Issue
Progresses https://github.com/MetaMask/mobile-planning/issues/1044
Progresses https://app.zenhub.com/workspaces/metamask-accounts-team-62505028c3853700162f65e0/issues/gh/metamask/mobile-planning/1043
Checklist