medic / cht-android

A native Android container for Community Health Toolkit (CHT) applications
GNU Affero General Public License v3.0
25 stars 51 forks source link

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

Closed sugat009 closed 7 months ago

sugat009 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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: https://github.com/medic/cht-android/pull/334#pullrequestreview-1707062417 Ref: https://github.com/medic/cht-android/pull/334#issuecomment-1802634663

kennsippell commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

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

Also, @sugat009 regarding the test failure, since we have not fixed https://github.com/medic/cht-android/issues/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 https://github.com/medic/cht-android/issues/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 commented 8 months ago

@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 commented 7 months ago

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

kennsippell commented 7 months ago

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