-
-
Notifications
You must be signed in to change notification settings - Fork 2.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
Added fallback-release-keystore.jks file as a fallback for release builds #17961
base: main
Are you sure you want to change the base?
Conversation
@mikehardy can you please review this once. |
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.
Not ready for review - fails the testing that depends on it in CI
* What went wrong:
Execution failed for task ':AnkiDroid:packagePlayReleaseAndroidTest'.
> A failure occurred while executing com.android.build.gradle.tasks.PackageAndroidArtifact$IncrementalSplitterRunnable
> com.android.ide.common.signing.KeytoolException: Failed to read key nrkeystorealias from store "/home/runner/work/Anki-Android/Anki-Android/tools/fallback-release-keystore.jks": keystore password was incorrect
You may reproduce this test result locally by doing what CI does and setting this variable:
TEST_RELEASE_BUILD: true |
Note that the CI workflow should probably be changed as well - if we are bundling a test keystore in the repository now, we no longer need the step "Test Credential Prep"
Anki-Android/.github/workflows/tests_emulator.yml
Lines 54 to 61 in 1d24177
- name: Test Credential Prep | |
run: | | |
echo "KSTOREPWD=$STOREPASS" >> $GITHUB_ENV | |
echo "KEYPWD=$KEYPASS" >> $GITHUB_ENV | |
mkdir $STOREFILEDIR | |
cd $STOREFILEDIR | |
echo y | keytool -genkeypair -dname "cn=AnkiDroid, ou=ankidroid, o=AnkiDroid, c=US" -alias $KEYALIAS -keypass $KEYPASS -keystore "$STOREFILE" -storepass $STOREPASS -keyalg RSA -validity 20000 | |
shell: bash |
...nor does the workflow need to set all the variables related to the test keystore (location, passwords, key alias etc) that are all over the file - will be nice to simplify it
Also note that you have left the description of the PR blank it still just says:
That's your opportunity to make a first impression with the PR. Please put something in there that explains why the PR exists, and what the PR does to address that reason Similarly, the commit currently says:
We don't strictly follow the "conventional commits" standard, but I do in other repos, and the 'update' appears to be a keyword for conventional commits but isn't a recognized keyword. I'd expect "feat:" for feature - this is a cool doc explaining conventional commits tags used by lots of open source repositories: https://www.conventionalcommits.org/en/v1.0.0/#specification historically, feat, fix, docs, chore, build, release are the ones I use most of the time "add fallback-release-keystore.jks file as a fallback" with no other comment won't help people going through commits if they're looking to fix an issue later Maybe something like:
|
@mikehardy Thanks for the review, working on the changes. |
this should help people trying to test release builds that haven't set up a release signing keystore - we'll provide one along with all the config key alias / password info to use it if release build is requested but no environment variables are present
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.
almost there - looks like the fallback will work, but now it works every time with no option of env vars, and we still use env vars for the final + real + official publish
storeFile file(System.getenv("KEYSTOREPATH") ?: "${homePath}/src/android-keystore") | ||
storePassword System.getenv("KEYSTOREPWD") ?: System.getenv("KSTOREPWD") | ||
keyAlias System.getenv("KEYALIAS") ?: "nrkeystorealias" | ||
keyPassword System.getenv("KEYPWD") | ||
storeFile file("${rootDir}/tools/fallback-release-keystore.jks") | ||
storePassword "Test@123" | ||
keyAlias "my-key" | ||
keyPassword "Test@123" |
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.
Close but not quite - we still need the ability to specify things by environment variable.
Why?
./tools/release.sh
drives the real release process using environment variables and a keystore / keystore secrets that are stored in github secrets and used in the workflow
So we need the environment variables specified here to continue working:
Anki-Android/.github/workflows/publish.yml
Lines 63 to 76 in c5ecff9
- name: Credential Prep | |
run: | | |
echo "KEYSTOREPATH=$HOME/src/android-keystore" >> $GITHUB_ENV | |
echo "KEYALIAS=nrkeystorealias" >> $GITHUB_ENV | |
echo "KEYSTOREPWD=${{ secrets.KEYSTORE_PASSWORD }}" >> $GITHUB_ENV | |
echo "KEYPWD=${{ secrets.KEYSTORE_KEY_PASSWORD }}" >> $GITHUB_ENV | |
mkdir ~/src | |
cd ~/src | |
echo "${{ secrets.AMAZON_PUBLISH_CREDENTIALS }}" | base64 -d > ./AnkiDroid-Amazon-Publish-Security-Profile.json.gz | |
echo "${{ secrets.GOOGLE_PUBLISH_CREDENTIALS }}" | base64 -d > ./AnkiDroid-GCP-Publish-Credentials.json.gz | |
echo "${{ secrets.RELEASES_PUBLISH_TOKEN }}" | base64 -d > ./my-github-personal-access-token.gz | |
echo "${{ secrets.KEYSTORE }}" | base64 -d > ./android-keystore.gz | |
gunzip *.gz | |
shell: bash |
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 still need the ability to specify things by environment variable.
I anticipated this while updating build.gradle 😅.
@mikehardy Got it, Would it make sense to check only KEYSTOREPATH? If it's missing or doesn't point to a valid keystore, we switch entirely to the local keystore, including its associated credentials. Something like this:
Would this approach work for our use case? Also, should I add the part where we set the environment variables in tests_emulator.yml, or should I leave it as is? |
I think if someone explicitly set an incorrect keystorepath, the build should fail and that's a good thing, let it fail I do not think we need to set env vars in the emulator test, the only place that will use them now is publish.yml, that's okay |
One approach would be to use
However, my concern with this approach is that KEYSTOREPATH might not be provided, while any one or more of the other environment variables (KEYSTOREPWD, KSTOREPWD, KEYALIAS, KEYPWD) could still be set, leading to an inconsistent configuration.
This approach ensures that if KEYSTOREPATH is explicitly set but incorrect, the build will fail as expected. If it's missing, we fall back to the local keystore.. |
Purpose / Description
Fixes
Approach
How Has This Been Tested?
Checklist