Skip to content
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

feat: deployment-based firebase config #2235

Merged
merged 15 commits into from
Apr 2, 2024
Merged

Conversation

chrismclarke
Copy link
Member

@chrismclarke chrismclarke commented Mar 14, 2024

PR Checklist

  • PR title descriptive (can be used in release notes)

Breaking Changes

  • Previously deployments would read global firebase config populated from github actions, and use for auth or crashlytics (always enabled). Now deployments need to explicitly include firebase config and opt in to using features. This also means FIREBASE_CONFIG_TS can be removed from github action environments.

In cases where deployments are targetting an older version of the main repo for actions they should also make sure to target the older version of reusable actions if not upgrading to the new syntax

It could be possible in the future to alter github actions to provide config using the reusable actions if useful

Description

  • Remove hardcoded firebaseConfig.ts file (populated locally during dev and via actions for CI)
  • Add support for loading firebase config directly from deployment config
  • Provide opt in/out for firebase auth and crashlytics in config
  • Add utilities to make it easier to load config from encrypted folder

Review Notes

The PR removes .firebaserc and firebase.json hardcode files, but also adds them to the .gitignore to allow users to store local copies. I removed the files via git rm so it should force the deletion through, but would be good to check it actually worked as intended and doesn't leave the files populated in local environment.

There will still be a src\environments\firebaseConfig.ts file left (as was previously gitignored) - this can just be deleted

For testing, there's essentially 4 cases to go through - summary of current findings below.

config exists config missing
firebase enabled Works as expected Deployment set and runtime warnings, crashlytics still loads on native using google-services.json
firebase disabled Runtime warnings if calling actions Crashlytics still initialises but then is disabled

In order to test these any config can be selected and modified to include/exclude loading firebase config and toggling features. E.g. placing a firebase.json config file in the deployment encrypted folder and updating deployment

import { loadEncryptedConfig} from "scripts";

config.firebase = {
  config: loadEncryptedConfig('firebase.json'), // set to null to omit debug missing config or do not populate encrypted
  auth:{enabled:true},
  crashlytics:{enabled:true}
}

Any changes should be recompiled to take effect, either by running the yarn workflow deployment set command or manually editing the compiled .idems_app\deployments\activeDeployment.json

Author Notes

I've marked as legacy (but not removed) documentation that refers to how we used to use deployment branches to deploy multiple apps from the main repo.
I'm not sure if we want to keep this, and whether the documentation I've added related to content-repo actions are already covered elsewhere (or should be?)

I've also removed the references to firebase config from the quickstart guides. It would be useful if someone could update the documentation (or add to future list) to include how to provide project-specific firebase config using encrypted environment, and that it is only required if using auth actions

Deployment configs now have to specify what firebase services are enabled. I've set the defaults to enable crashlytics and disable auth (as I think that is the most common setup for projects) - so deployments using auth will need to be manually updated, and we may want to consider explicitly setting the crashlytics option on deployments in case we ever decide to revert the default to enabled: false

firebase: {
    auth: { enabled: false },
    crashlytics: { enabled: true },
  }

There is a new utility function loadEncryptedConfig(fileName) function that can be used to load json files from the encrypted folder. I've made a pr to implement these changes on debug repo here:
IDEMSInternational/app-debug-content#54

import { loadEncryptedConfig} from "scripts";

config.firebase = {
  config: loadEncryptedConfig('firebase.json'),
  auth:{enabled:true},
  crashlytics:{enabled:true}
}

Dev Notes

The PR includes templated .firebaserc and firebase.json files, however as of now I don't think these are actually used as the reusable github actions overwrite the files. e.g. reusable-deploy-pr-preview.yml

We may want to consider simplifying the approach to work from templates with some sort environment variable substitution (I've defined the template in a way that should be compatible with tools like env_substr) - will make follow-up issue and consult with Chris M

I haven't include a way to fully opt out of firebase altogether as this would require modifying android build.gradle dependencies and capacitor.config.ts, however runtime services are disabled.

I'm hoping that the new loadEncryptedConfig function should make it easier to assign config properties with graceful fail. I've updated the supabaseConfig to operate in a similar way, although wondering if it might also make sense to destructure a little more like firebase, namely {supabase: {config: loadEncryptedConfig()} and assume enable/disabled based on whether config exists (?)

I've removed the @angular/fire package as it was causing issue with conditional app initialisation (possibly related to https://github.com/angular/angularfire/issues/3443), and actually as it is only a wrapper around firebase js sdk we're not really using because we use the @capacitor-firebase wrappers instead. The one use case we had was to initialise the core firebase app globally, so now I've just added a service to handle that instead

Git Issues

Closes #1976

Screenshots/Videos

Example - deployment set warning if encrypted config defined but not available
image

Example - runtime console warning during service initialisation if firebase config missing
image

Example - runtime console error when using auth actions without firebase config (non-breaking)
image

Example - android console when firebase config missing (still enables crashlytics using data from google-services.json)
image

@github-actions github-actions bot added the maintenance Core updates, refactoring and code quality improvements label Mar 14, 2024
@chrismclarke chrismclarke changed the title chore: remove hardcoded firebase config files feat: deployment-based firebase config Mar 14, 2024
@github-actions github-actions bot added feature Work on app features/modules and removed maintenance Core updates, refactoring and code quality improvements labels Mar 14, 2024
@github-actions github-actions bot added feature Work on app features/modules and removed feature Work on app features/modules labels Mar 14, 2024
@github-actions github-actions bot removed the feature Work on app features/modules label Mar 14, 2024
@github-actions github-actions bot added the feature Work on app features/modules label Mar 14, 2024
@github-actions github-actions bot added feature Work on app features/modules and removed feature Work on app features/modules labels Mar 14, 2024
@github-actions github-actions bot added feature Work on app features/modules and removed feature Work on app features/modules labels Mar 14, 2024
@github-actions github-actions bot added feature Work on app features/modules documentation Improvements or additions to documentation and removed feature Work on app features/modules labels Mar 14, 2024
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Mar 19, 2024
@jfmcquade
Copy link
Collaborator

As the filename passed to loadEncryptedConfig() is expected to be already decrypted, I think we need an additional action, or step in the existing build action, that decrypts the contents of the encrypted folder for a deployment, using a private key. Otherwise we can only enable functionality that depends on encrypted config values when building the app locally.

This could make use of the existing yarn workflow deployment decrypt, after populating a file to .idems_app/deployments/<deplyment_name>/encrypted/private.key from the value of a PRIVATE_KEY secret, for example.

@chrismclarke
Copy link
Member Author

As the filename passed to loadEncryptedConfig() is expected to be already decrypted, I think we need an additional action, or step in the existing build action, that decrypts the contents of the encrypted folder for a deployment, using a private key. Otherwise we can only enable functionality that depends on encrypted config values when building the app locally.

This could make use of the existing yarn workflow deployment decrypt, after populating a file to .idems_app/deployments/<deplyment_name>/encrypted/private.key from the value of a PRIVATE_KEY secret, for example.

The reusable build action should already include this when running on CI
image

And the deployment set action automatically includes decrypt as part of the workflow
image

Not sure if there's another case I'm missing here?

@jfmcquade
Copy link
Collaborator

jfmcquade commented Mar 22, 2024

The reusable build action should already include this when running on CI

Right you are. This was the case I was thinking of, I'd missed this step in the action.

I think should be good to merge pending review from @esmeetewinkel

@jfmcquade jfmcquade mentioned this pull request Mar 25, 2024
1 task
@jfmcquade
Copy link
Collaborator

jfmcquade commented Mar 26, 2024

@esmeetewinkel, to summarise our discussion for clarity: this PR does not effect web preview deployments to Firebase hosting.

Deploying to Firebase hosting is handled by the reusable-deploy-web-preview github action, which takes its variables from hardcoded github secrets on the deployment repo (minimally required are: a Firebase project ID, e.g. plh-teens-app1; a Firebase hosting target, e.g. plh-kids-tz; and the credentials of a service account that is authorised to deploy to Firebase hosting for the relevant project). These properties could potentially be included as (encrypted) parts of the deployment config in the future if desired.

The "Firebase config" that is the concern of this PR is used for runtime Firebase functionality (auth and crashlytics). This is complicated a bit by the fact that Android apps are configured in a different way to web apps – as part of the build process, an Android app needs to pull its Firebase config data from a google-services.json file. This is handled via a secret on the deployment repo and is not changed by this PR (again, this could potentially be configured to be read from the deployment config if desired). So technically the changes of this PR only affect the web app version. However, the way that the deployment config is interpreted, in order to enable Firebase features, e.g. auth, a valid web app-related Firebase config must be provided, even if the final app will be an Android app.

A major benefit of this PR is that authors and devs are no longer required to populate a secret firebaseConfig file as part of setting up the dev environment. When working on a deployment locally that does not make explicit use of Firebase features (auth), the locally built web app will not connect to Firebase at all. This wasn't previously possible, as all apps connected to Firebase regardless.

@chrismclarke – feel free to clarify/correct anything

@github-actions github-actions bot added feature Work on app features/modules documentation Improvements or additions to documentation and removed documentation Improvements or additions to documentation feature Work on app features/modules labels Mar 26, 2024
Copy link
Collaborator

@esmeetewinkel esmeetewinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, I've read through and I think I'm understanding the main points.

To test this on PLH Teens TZ, I made the changes described above, see this content PR. I believe I can only test whether the Google Auth still works with the Appetize action on this PR, but I don't know how to enable it (see #2182)

@chrismclarke
Copy link
Member Author

chrismclarke commented Mar 26, 2024

Thanks for this, I've read through and I think I'm understanding the main points.

To test this on PLH Teens TZ, I made the changes described above, see this content PR. I believe I can only test whether the Google Auth still works with the Appetize action on this PR, but I don't know how to enable it (see #2182)

I think with the way the reusable actions are setup it might be hard to test action changes that are part of a PR .

A content repo refers directly to the master branch action, so in order to call the code that is sitting in this pr the content repo would also have to update the target for the action, e.g.

.github/workflows/deploy-pr-preview.yml

jobs:
  pr_preview:
    uses: IDEMSInternational/parenting-app-ui/.github/workflows/reusable-deploy-pr-preview.yml@master
    secrets: inherit

There is then an added challenge that the action also checks out the repo at a given reference, using an environment variable set at the parent repo level e.g.

.github/workflows/reusable-app-build.yml

   steps:
        - name: Check out app code
          uses: actions/checkout@v3
          with:
            repository: "IDEMSInternational/parenting-app-ui.git"
            ref: ${{env.APP_CODE_BRANCH}}

So I think we would likely need to update the content repo actions to use the same APP_CODE_BRANCH variable, and then create some means of triggering the actions with that variable (likely by creating a 3rd action within the content repo itself which can be manually triggered)

- uses: IDEMSInternational/parenting-app-ui/.github/workflows/reusable-deploy-pr-preview.yml@master
+ uses: IDEMSInternational/parenting-app-ui/.github/workflows/reusable-deploy-pr-preview.yml@${{env.APP_CODE_BRANCH}}

(although I doubt you can even pass a dynamic string to a github uses block, would need testing)

We'd also need to do a bit of extra work on the debug content repo to provide some sort of test_action action that takes the name of the reusable action to trigger and override of any other environment variables as required.
(not sure if this makes sense to you @jfmcquade and whether something you might have capacity to do - otherwise we could create a follow-up issue and try to clarify a bit more)

Otherwise the only way to functionally test will be post-merge

@jfmcquade
Copy link
Collaborator

We'd also need to do a bit of extra work on the debug content repo to provide some sort of test_action action that takes the name of the reusable action to trigger and override of any other environment variables as required. (not sure if this makes sense to you @jfmcquade and whether something you might have capacity to do - otherwise we could create a follow-up issue and try to clarify a bit more)

I think a follow up issue with a bit more clarity would be good. I'm happy to look into it but am unlikely to do so immediately

@chrismclarke
Copy link
Member Author

We'd also need to do a bit of extra work on the debug content repo to provide some sort of test_action action that takes the name of the reusable action to trigger and override of any other environment variables as required. (not sure if this makes sense to you @jfmcquade and whether something you might have capacity to do - otherwise we could create a follow-up issue and try to clarify a bit more)

I think a follow up issue with a bit more clarity would be good. I'm happy to look into it but am unlikely to do so immediately

I've added as #2264

@github-actions github-actions bot added feature Work on app features/modules documentation Improvements or additions to documentation and removed documentation Improvements or additions to documentation feature Work on app features/modules labels Apr 2, 2024
@jfmcquade jfmcquade merged commit fd99bd4 into master Apr 2, 2024
7 checks passed
@jfmcquade jfmcquade deleted the feat/deployment-firebase branch April 2, 2024 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feature Work on app features/modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Make firebase opt-in
3 participants