Open dzuluaga opened 6 months ago
Thanks @dzuluaga for bringing this to our attention!
While integrating Google Secrets plugin might sound optimal at first. However, after partially trying to integrate it into the project, it became clear to us that it's quite limited in it's functionality.
For instance, beside injecting FACEBOOK_APP_ID
and FACEBOOK_CLIENT_TOKEN
values into the AndroidManifest.xml file, we also need to specify some additional values like: android:path="@string/microsoft_path"
android:scheme="@string/fb_login_protocol_scheme"
and android:scheme="@string/db_login_protocol_scheme"
for the Facebook, Microsoft and Dropbox plugins to work properly. Unfortunately Google Secrets Plugin only supports injecting values into android:value=""
.
Moreover, we need to access the MICROSOFT_CLIENT_ID
and MICROSOFT_SIGNATURE_HASH
values inside the build.gradle file for additional configuration. As @artus9033 mentioned, Google Secrets plugin is limited and won't allow us to access those values through resString
which instead we will have to manually read from the local.properties file like we are doing currently.
We might be able to get rid of 2 lines of code for injecting the FACEBOOK_APP_ID
and FACEBOOK_CLIENT_TOKEN
values automatically using the Google Secrets Plugin but with the cost of making the documentation less clear as using this plugin is not common among Android developers so we would have to explain the magic that it does behind and also it's limitations.
Ultimately, we will have an AndroidManifest.xml file that both injects values from the Google Secrets Plugin and String resources. In my opinion this creates unnecessary complications and confusion with some potential code minimizations that are not worth it.
Happy to hear your thoughts!
Thanks, Andrei. Please see my inline comments. If it's easier I can jump on a call to discuss further.
For instance, beside injecting FACEBOOK_APP_ID and FACEBOOK_CLIENT_TOKEN values into the AndroidManifest.xml file, we also need to specify some additional values like: android:path="@string/microsoft_path" android:scheme="@string/fb_login_protocol_scheme" and android:scheme="@string/db_login_protocol_scheme" for the Facebook, Microsoft and Dropbox plugins to work properly. Unfortunately Google Secrets Plugin only supports injecting values into android:value="".
I noticed the requirements for integrating the Facebook, Microsoft, and Dropbox plugins involve injecting FACEBOOK_APP_ID
and FACEBOOK_CLIENT_TOKEN
into the AndroidManifest.xml
, along with specifying additional values like android:path
and android:scheme
for each plugin. It's mentioned that these additional values, such as microsoft_path
and fb_login_protocol_scheme
, are stored in local.properties
, which is unconventional. Typically, non-secret configurations are placed in the res/values/strings.xml
file, simplifying management and access without extra code in the build.gradle
.
Could you elaborate on the decision to store these non-secret arguments in local.properties
? Is there a specific requirement or advantage to this approach?
Moreover, we need to access the MICROSOFT_CLIENT_ID and MICROSOFT_SIGNATURE_HASH values inside the build.gradle file for additional configuration. As @artus9033 https://github.com/openmobilehub/android-omh-maps/pull/23#issuecomment-1983172088, Google Secrets plugin is limited and won't allow us to access those values through resString which instead we will have to manually read from the local.properties file like we are doing currently.
Thanks for highlighting the limitations with the Google Secrets plugin and our current method for accessing MICROSOFT_CLIENT_ID
and MICROSOFT_SIGNATURE_HASH
. I propose a simpler approach: instead of generating the ms_auth_config.json
dynamically, we could guide developers to manually create this file, as detailed in our readme.md
. This method removes the need to fetch these values in the build.gradle
file and utilizes the MSAL SDK's ability to read the ms_auth_config.json
directly, aligning with the MSAL documentation. This should make the setup process more straightforward and our build scripts less complex.
I'm keen to hear your thoughts on this suggestion or if there are any alternatives or concerns I might have missed.
We might be able to get rid of 2 lines of code for injecting the FACEBOOK_APP_ID and FACEBOOK_CLIENT_TOKEN values automatically using the Google Secrets Plugin but with the cost of making the documentation less clear as using this plugin is not common among Android developers so we would have to explain the magic that it does behind and also it's limitations.
Ultimately, we will have an AndroidManifest.xml file that both injects values from the Google Secrets Plugin and String resources. In my opinion this creates unnecessary complications and confusion with some potential code minimizations that are not worth it.
Happy to hear your thoughts!
Hey @dzuluaga, indeed, inline comments seem to clarify things a little bit better.
I noticed the requirements for integrating the Facebook, Microsoft, and Dropbox plugins involve injecting
FACEBOOK_APP_ID
andFACEBOOK_CLIENT_TOKEN
into theAndroidManifest.xml
, along with specifying additional values likeandroid:path
andandroid:scheme
for each plugin. It's mentioned that these additional values, such asmicrosoft_path
andfb_login_protocol_scheme
, are stored inlocal.properties
, which is unconventional. Typically, non-secret configurations are placed in theres/values/strings.xml
file, simplifying management and access without extra code in thebuild.gradle
.
Just to clarify, the microsoft_path
and fb_login_protocol_scheme
are NOT stored in the local.properties file. They are being created in the build.gradle and injected into the res/values/strings.xml file during build time using the resValue
function.
In the local.properties file, we only store our secrets which we don't want to have in our repo. They are also being used for our GitHub build actions.
Thanks for highlighting the limitations with the Google Secrets plugin and our current method for accessing
MICROSOFT_CLIENT_ID
andMICROSOFT_SIGNATURE_HASH
. I propose a simpler approach: instead of generating thems_auth_config.json
dynamically, we could guide developers to manually create this file, as detailed in ourreadme.md
. This method removes the need to fetch these values in thebuild.gradle
file and utilizes the MSAL SDK's ability to read thems_auth_config.json
directly, aligning with the MSAL documentation. This should make the setup process more straightforward and our build scripts less complex.
Initially we were using the ms_auth_config.json file to initialize the MSAL SDK, however we decided to generate this file during the build process instead, in order to be able to run the GitHub build actions with only referencing the secret values from GitHub.
Another approach we can take is to try to generate the ms_auth_config.json file during the GitHub build action, however this might be a little bit tricky and we would need to investigate this approach a little bit more.
Happy to hear your thoughts on this!
Just to clarify, the microsoft_path and fb_login_protocol_scheme are NOT stored in the local.properties file. They are being created in the build.gradle and injected into the res/values/strings.xml file during build time using the resValue function.
Let's stick to the official guidelines for integrating values like facebook_app_id
and fb_login_protocol_scheme
, and so on directly into strings.xml
, as recommended by the FB Login Quickstart guide. Injecting these values through build scripts complicates things unnecessarily. Unless there's a specific reason we're not following the standard approach, we should align with the official documentation. This will make our process more straightforward and in line with expectations from the framework's docs, avoiding over-reliance on build script configurations.
Another approach we can take is to try to generate the ms_auth_config.json file during the GitHub build action, however this might be a little bit tricky and we would need to investigate this approach a little bit more.
I agree that generating the ms_auth_config.json file should be part of the GitHub Actions workflow rather than the build script. This approach aligns better with its role in the process. Below is a concise example of how we can automate the creation of this file during our GitHub build process:
name: Build and Deploy
on: [push, pull_request]
jobs:
build:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
# Generate ms_auth_config.json
- name: Generate MS Auth Config
run: |
cat <<EOF > ms_auth_config.json
{
"microsoft_client_id": "${{ secrets.MICROSOFT_CLIENT_ID }}"
}
EOF
shell: bash
# Use the generated file in your Gradle build
- name: Build with Gradle
run: ./gradlew build
I've noticed an inconsistency in the way our sample app handles app secrets, specifically how we're using
resValue
for readingFACEBOOK_APP_ID
,FACEBOOK_CLIENT_TOKEN
,MICROSOFT_CLIENT_ID
, etc., while simultaneously using the secrets plugin for theGOOGLE_CLIENT_ID
. This approach seems to diverge in methodology for handling similar types of secrets, which could potentially lead to confusion and maintenance issues down the line.In the context of our project's goal to maintain consistency and ease of maintainability, I believe it would be beneficial to standardize the method we use for retrieving these variables. My suggestion aligns with the comments I've made in the pull request here: Pull Request #23 Comment.
To summarize my recommendation from the PR comment:
local.properties
file for local development, which can then be easily overridden by CI for production builds.GOOGLE_CLIENT_ID
,FACEBOOK_APP_ID
,FACEBOOK_CLIENT_TOKEN
,MICROSOFT_CLIENT_ID
, etc.).I believe that adopting a uniform approach will not only simplify the codebase but also make it easier for new contributors to understand and for existing contributors to maintain. It would be great to hear your thoughts on this and discuss potential paths forward.
Thank you for considering my feedback.