Skip to content
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

refactor: Use mobilestack references in android builds #6039

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

jophish
Copy link
Contributor

@jophish jophish commented Sep 18, 2024

Description

Makes the Android builds more brand agnostic.

Test plan

Related issues

  • Fixes #[issue number here]

Backwards compatibility

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

  • Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

@@ -39,8 +39,8 @@ jobs:
with:
secrets: |-
ANDROID_RELEASE_KEYSTORE:projects/1027349420744/secrets/ANDROID_RELEASE_KEYSTORE
CELO_RELEASE_KEY_PASSWORD:projects/1027349420744/secrets/CELO_RELEASE_KEY_PASSWORD
CELO_RELEASE_STORE_PASSWORD:projects/1027349420744/secrets/CELO_RELEASE_STORE_PASSWORD
MOBILESTACK_RELEASE_KEY_PASSWORD:projects/1027349420744/secrets/MOBILESTACK_RELEASE_KEY_PASSWORD
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've ported over these secrets in GCP

Copy link

codecov bot commented Sep 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.74%. Comparing base (312d7e0) to head (d09bacb).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6039      +/-   ##
==========================================
- Coverage   88.74%   88.74%   -0.01%     
==========================================
  Files         727      727              
  Lines       30785    30785              
  Branches     5623     5319     -304     
==========================================
- Hits        27321    27320       -1     
- Misses       3265     3421     +156     
+ Partials      199       44     -155     

see 67 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 312d7e0...d09bacb. Read the comment docs.

@@ -50,6 +50,9 @@ jobs:
yes | sdkmanager "platform-tools" "platforms;android-${{ inputs.android-api-level }}"
set -o pipefail

# Install Ninja
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here; not sure why this is now needed, though I have to imagine it's related to the rest of the changes in this PR

@jophish
Copy link
Contributor Author

jophish commented Sep 19, 2024

This seems to fail on the nightly release that I tested due to the build looking for celo-release-key.keystore, despite having removed all apparent references of it from the repo... not sure why this is happening.

@jophish jophish changed the title refactor: Use mobilestack reference in android builds refactor: Use mobilestack references in android builds Sep 19, 2024
kathaypacific
kathaypacific previously approved these changes Sep 19, 2024
Copy link
Collaborator

@kathaypacific kathaypacific left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

android/app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@kathaypacific kathaypacific dismissed their stale review September 19, 2024 08:27

Oh wait, I just saw your comment about the release not working with these changes 🙈

@jophish
Copy link
Contributor Author

jophish commented Sep 26, 2024

The nightly release test passed! https://valora-app.slack.com/archives/C02D08P412Q/p1727385960907899

Copy link
Collaborator

@kathaypacific kathaypacific left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 LGTM! remember to populate the PR description before merging :D

Copy link
Member

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

.github/workflows/release-fastlane-android.yml Outdated Show resolved Hide resolved
Comment on lines 21 to 24
# Note that the key alias lacks Mobile Stack naming, since updating this would
# require regenerating the keystore.
MOBILESTACK_RELEASE_STORE_FILE=mobilestack-release-key.keystore
MOBILESTACK_RELEASE_KEY_ALIAS=celo-key-alias
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can update the keystore. Also an opportunity to use the same password for the store and the key inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeanregisser What's the typical process for this? I assume we want to re-use our existing upload key; I can't seem to find an option in Android Studio (which is what I've used in the past for creating brand new upload keys/keystores) to create a new keystore from an existing key.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, this can be done using the keytool cli:

WALLET.md Outdated Show resolved Hide resolved
.github/workflows/release-fastlane-android.yml Outdated Show resolved Hide resolved
.github/workflows/release-fastlane-android.yml Outdated Show resolved Hide resolved
android/app/build.gradle Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants