fix(#345): Hardcode the values ANDROID_KEY_ALIAS and ANDROID_KEYSTORE_PATH in make command check-env

[sugat009]

The changes include hardcoding the values ANDROID_KEY_ALIAS AND ANDROID_KEYSTORE_PATH in the make command check-env such that they don't use the secrets in the repository.

Fixes: (#345 )

[kennsippell]

Nice work @sugat009. I suspect this is the simplest functional version of the change possible.

I recommend our roadmap to be:

1. Get tests passing
2. Test that publication still works for all flavors. For this, I'd recomment you publish an "alpha release". I can't find the documentation explaining how to do that, so recommend you ask on forum or #development.
3. Assuming that works, polish this change. There are a few more changes required like not outputting the values on line 219.
4. Get a real code review from core dev
5. Commit and tag as latest stable
6. I will delete the unneeded secrets in GitHub

[sugat009]

The instrumentation tests seem to be failing for this PR but I'm not sure if these changes have anything to do with the test failing. After going through the already existing issues and PR, the issue seems to be persisting even in the main branch and an issue has been created to address it as well #337 . So, I guess we can move on with the action items for this PR.
Ref: #334 (review)
Ref: #334 (comment)

[jkuester]

👍 I think Kenn is spot on here! These changes look good to me (have just requested a few additional clean-ups below). Before merging this PR, lets follow Kenn's suggestion and try to run a tag build (basically just going to be steps #2-3 of the Alpha for release testing steps except we are just going to do it in the branch before even going to master). Should be able to create a tag right on the HEAD of this branch called v1.1.2-secrets-345 and push it to GitHub. Then watch to see if the publish workflow succeeds. If it does, then I think we are good to go.

5. Commit and tag as latest stable

@kennsippell Is there a reason we need a new tag at this time? (These changes seem to just be with the build process and so will be needed for the next release, but I guess we don't need to actually release anything right now...)

Also, @sugat009 regarding the test failure, since we have not fixed #337, I have no reason to expect that test to pass. 😞 Since nothing is actively blocked yet by this PR, I think we need to bite the bullet and try to solve #337 for this PR 😬. The sad fact is that if we don't do it now, it may never get done. 😓

The bad news for that is that I pretty much exhausted all my ideas for that test with the investigation/documentation that I already did. @sugat009 how much experience do you have with Android tests? 😅 Maybe you can see what I missed. Otherwise, my next best idea is to maybe just try and upgrade our way out of the problem (always a winning strategy). Some ideas to try (from most to least promising):

• build.yml:
  • I notice that our targetSdk in the build.gradle is 33, but the api-level in our CI build.yml jobs for running the tests is 29. Maybe bumping that will help.
  • Can also try bumping the image versions used by those CI job (if we are not on the latest)
• build.gradle
  • Updating the versions of our androidTestImplementation dependencies (where possible)

[kennsippell]

Is there a reason we need a new tag at this time? (These changes seem to just be with the build process and so will be needed for the next release, but I guess we don't need to actually release anything right now...)

Don't need to release per se, but all future releases will need to be based off this. Once the secrets are gone, old versions will not be publishable. I just noticed this comment

Check out the tag from the last stable release in CHT Android repository and create a branch
https://docs.communityhealthtoolkit.org/apps/guides/android/branding/#2-new-brand

So my comment about the stable tag is just to ensure it is the base branch for future work.

[sugat009]

@jkuester I've made the changes requested and also did the steps from the alpha-release by creating a tag(v1.1.2-secrets-test) (v1.1.2-secrets-345 tag makes the CI fail due to a constraint in the build.gradle file). The pipeline is passing now for the tag-push atleast. 😅
@kennsippell Once the PR is approved the secrets can hopefully be deleted.

@jkuester @lorerod Please review.

The bad news for that is that I pretty much exhausted all my ideas for that test with the investigation/documentation that I already did. @sugat009 how much experience do you have with Android tests? 😅 Maybe you can see what I missed. Otherwise, my next best idea is to maybe just try and upgrade our way out of the problem (always a winning strategy). Some ideas to try (from most to least promising):

@jkuester Unfortunately, I have no experience in java or android at all. 😅 I guess this needs to be done another time separately as this issue will be prolonged.

[lorerod]

@sugat009 @jkuester regarding these two comments about the test failure:

Also, @sugat009 regarding the test failure, since we have not fixed #337, I have no reason to expect that test to pass. 😞 Since nothing is actively blocked yet by this PR, I think we need to bite the bullet and try to solve #337 for this PR 😬. The sad fact is that if we don't do it now, it may never get done. 😓

I guess this needs to be done another time separately as this issue will be prolonged.

I'm happy with both options. If we decide to fix the failing test now for this PR, I can actively help debug and find a solution; I can put all the effort into what is left of this week.
If we decide to do another time separately, I can assign this issue to myself and prioritize it after I return from time off after the meetup.
Unfortunately, I also don't have experience with the test suite of this repository and Android, but I can learn.

Please let me know what you think.

[sugat009]

@lorerod In my opinion, it would be preferable to utilize the existing issue #337 to tackle the failing tests in a different PR. Addressing this concern within the current task would lead us to deviate from its intended scope.

[jkuester]

LGTM!

In other good news, I think I got the CI issues fixed over in #348.

[jkuester]

@kennsippell when you get the chance, can you remove the unneeded secrets from the repo now?

[kennsippell]

@jkuester @sugat009 I've deleted unneeded secrets. There are now only 75 tokens in the cht-android repository.