-
Notifications
You must be signed in to change notification settings - Fork 25
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
Conversation
As the filename passed to This could make use of the existing |
The reusable build action should already include this when running on CI And the deployment set action automatically includes decrypt as part of the workflow Not sure if there's another case I'm missing here? |
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 |
@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 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 A major benefit of this PR is that authors and devs are no longer required to populate a secret @chrismclarke – feel free to clarify/correct anything |
There was a problem hiding this 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)
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
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.
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 - 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 Otherwise the only way to functionally test will be post-merge |
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 |
PR Checklist
Breaking Changes
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
firebaseConfig.ts
file (populated locally during dev and via actions for CI)Review Notes
The PR removes
.firebaserc
andfirebase.json
hardcode files, but also adds them to the.gitignore
to allow users to store local copies. I removed the files viagit 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 deletedFor testing, there's essentially 4 cases to go through - summary of current findings below.
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 deploymentAny 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
actionsDeployment 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
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
Dev Notes
The PR includes templated
.firebaserc
andfirebase.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.ymlWe 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 MI haven't include a way to fully opt out of firebase altogether as this would require modifying android
build.gradle
dependencies andcapacitor.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 tohttps://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 insteadGit Issues
Closes #1976
Screenshots/Videos
Example - deployment set warning if encrypted config defined but not available
Example - runtime console warning during service initialisation if firebase config missing
Example - runtime console error when using auth actions without firebase config (non-breaking)
Example - android console when firebase config missing (still enables crashlytics using data from google-services.json)