Chore/externalize env config vars #68
Conversation
…les and potentially unintentionally committing secrets.
|
boo looks like our CI tries to build app. Not sure what best practices are here (I can see value add for ensuring build/compile as a basic smoke test, but this may not be the best way?). I suspect I should update |
|
converting to a draft for now so it can be a learning ground in its current state |
| @@ -2,6 +2,15 @@ plugins { | |||
| id 'com.android.application' | |||
| id 'org.jetbrains.kotlin.android' | |||
| } | |||
| // Creates a variable called keystorePropertiesFile, and initializes it to the | |||
There was a problem hiding this comment.
nitpick(blocking): Copied over example/reference code comments when digging into best practices. Will remove.
| @@ -24,6 +33,12 @@ android { | |||
| release { | |||
| minifyEnabled false | |||
| proguardFiles getDefaultProguardFile('proguard-android-optimize.txt'), 'proguard-rules.pro' | |||
| buildConfigField("String", "ACCESS_TOKEN_ID", "\"${localEnvProperties['ACCESS_TOKEN_ID']}\"") | |||
There was a problem hiding this comment.
question: Should these be duplicated? How does "defaulting" and "overriding" config values work here (/best practices for gradle)
There was a problem hiding this comment.
Each module has its own BuildConfig file, there is no duplication or overriding of BuildConfig files between modules.
|
|
||
| # For help on creating Mux Access tokens, see: https://docs.mux.com/guides/system/make-api-requests#access-token-permissions | ||
| ACCESS_TOKEN_ID=YOUR_MUX_TOKEN_ID | ||
| ACCESS_TOKEN_SECRET=YOUR_MUX_TOKEN_SECRET |
There was a problem hiding this comment.
nitpick(blocking): Will add newline before EOF
| @@ -2,6 +2,15 @@ plugins { | |||
| id 'com.android.application' | |||
| id 'org.jetbrains.kotlin.android' | |||
| } | |||
| // Creates a variable called keystorePropertiesFile, and initializes it to the | |||
There was a problem hiding this comment.
question(blocking): Since this file isn't guaranteed to exist but "should" exist for a functioning app, how should we handle this case? A hard failure on the build task is informative for local dev, but (at least currently) we also build the example app on our CI builds, which currently fails as a result of this change. Would love input/suggestions/best practices here.
There was a problem hiding this comment.
For a real production app, you're definitely on the right track. Templates for properties files are the way to go, and it's reasonable to expect programmers to copy over the files. For our case, this confers no benefit; customer keys don't make their way upstream. My best advice is to just commit a real .properties with "YOUR KEY HERE" or (better) "SET THIS IN local-env.properties" placeholders.
My second-best advice is to print a warning to the logs if the file is missing, and provide default values when you're declaring your buildConfigFields so nothing melts. "" would be just fine for this. Failing the build here might be seen as an unnecessary "gotcha," so please prefer to log at the warning level, which I think should even surface in the IDE.
There was a problem hiding this comment.
Great research! This is basically what you do when you want to put keys and stuff into your build. (but!)
Because app is an example app, I think we can make things a little easier than we would for a real app. I left two suggestions at different levels of complexity for inspiration
| @@ -2,6 +2,15 @@ plugins { | |||
| id 'com.android.application' | |||
| id 'org.jetbrains.kotlin.android' | |||
| } | |||
| // Creates a variable called keystorePropertiesFile, and initializes it to the | |||
There was a problem hiding this comment.
For a real production app, you're definitely on the right track. Templates for properties files are the way to go, and it's reasonable to expect programmers to copy over the files. For our case, this confers no benefit; customer keys don't make their way upstream. My best advice is to just commit a real .properties with "YOUR KEY HERE" or (better) "SET THIS IN local-env.properties" placeholders.
My second-best advice is to print a warning to the logs if the file is missing, and provide default values when you're declaring your buildConfigFields so nothing melts. "" would be just fine for this. Failing the build here might be seen as an unnecessary "gotcha," so please prefer to log at the warning level, which I think should even surface in the IDE.
|
@cjpillsbury Re: verifying that Overall, yes I think you should provide some sane defaults (enough to build) and warn to the logs. This seems more in-line with the expectations people would have of example code. |
|
Closing since this was a POC spike and is stale. |
Pulled out our local dev env variables into a properties file that isn't tracked by github I think using best practices.