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

Provides Firebase Admin services #543

Open
loicmathieu opened this issue Dec 11, 2023 · 13 comments
Open

Provides Firebase Admin services #543

loicmathieu opened this issue Dec 11, 2023 · 13 comments

Comments

@loicmathieu
Copy link
Collaborator

Provides Firebase Admin services

@jfbenckhuijsen
Copy link
Contributor

We don't already have devservices for the firebase admin emulator right?

@loicmathieu
Copy link
Collaborator Author

No, not yet ;)

@jfbenckhuijsen
Copy link
Contributor

gotcha ;)

@jfbenckhuijsen
Copy link
Contributor

@loicmathieu Question: working on this topic. The issue I see is that the Firebase emulator encapsulates both the PubSub and the Firestore emulator. I.e. if we run the Firebase emulator, but also have the DevService for PubSub enabled, I'd say we want the Firebase emulator to handle both and not run that separate container.

Currently I see multi possible approaches to this:

  • Add a config property use the Firebase emulator (quarks.google.cloud.firebbase.devservice.enabled = true or something). If this is set, dont' run the PubSub (and Firestore and ...) services. Would introduce some coupling at the -deployment level modules to get that hooked up.
  • Ditch the existing PubSub and Firestore emulators and always run the Firebase emulator (adjusting the --only flag accordingly). Downside of this is that the imageName config property would be a bit hard to support. So that would imply a breaking change in the config.
  • ....?

your thoughts?

@loicmathieu
Copy link
Collaborator Author

@jfbenckhuijsen all deveservices can be disabled by config properties (if not, it's a bug).

To keep things simple, I would only run the firebase emulator from the firebase extension if possible so that there are no coupling between extensions.

@jfbenckhuijsen
Copy link
Contributor

Yeah true, that I got, the issue is the firebase emulator is not just a firebase-auth emulator, it packs a whole lot of emulators. And they also have some interaction when packed together I think (e.g. hosting and functions forwarding rules) which only work when run as a whole.

The current setup I have just lets you specify you want a pub-sub emulator, and the logic internally automagically figures out if you want the firebase-packed one, or the plain one. Ill create a working PR, easier to discuss probably

@jfbenckhuijsen
Copy link
Contributor

#706 is the ground work for this change, without the work needed to exclude the pubsub or Firestore emulators in case the Firebase emulator is running.

#707 is a way to disable the Pubsub/Firestore emulators using MP Config properties which are automatically set when the Firebase module is included. The assumption is in case that module is present, you want to use the Firebase version.

This way, we just have a config dependency between the Pubsub/Firestore modules and the Firebase module, not any runtime or dev-time dependency. Lemme know what you think about this setup. Seems cleanest, as the user can also override this and still use the non-Firebase versions if needed.

Note that these all depend on #705 which isn't ready yet, and I'll need to add some more testing, so don't merge just yet.

@loicmathieu
Copy link
Collaborator Author

@jfbenckhuijsen I just merged #705, I didn't saw that you considered it non-ready. If I need to revert it, please tell me.
In those case, better to open it in Draft (or at least add some comment).

For the other one, they are huge, I'll need some time to review them.

@jfbenckhuijsen
Copy link
Contributor

jfbenckhuijsen commented Nov 4, 2024

No worries, this one is fine, the others are work in progress so don't merge those just yet.

#706 and #707 are marked as draft.

As for the size, lemme know what you think about the way to handle that conflict as indicated in #707 that one in itself is not that large.

There is still some work in getting it to actually work, but that's mostly work for #706, not #707.

@jfbenckhuijsen
Copy link
Contributor

jfbenckhuijsen commented Nov 14, 2024

@loicmathieu quite a bit of progress. Current state is that the code works and should be ready for an initial review.

It's not yet feature complete fully yet, but the additions are mostly convenience stuff, not the core of the changes.

Also spend some time in cleaning and splitting the code, so it should be a bit easier to review (mind you, it's still quite large).

Current features planned for now are:

  • Support for storage rules/firestore rules/firestore indexes
  • Support for including your own custom firebase json instead of the auto-generated one. This should also auto-configure the extension based on the firebase.json file, so you end up with a real nice integrated setup.

However, as the current PR is already quite large, lets focus on getting this part reviewed and merged first, I'll add those changes to a separate PR.

Please focus on #707, that one is the complete one.

@jfbenckhuijsen
Copy link
Contributor

@loicmathieu btw if I can do anything to make the review easier for you, lemme know

@loicmathieu
Copy link
Collaborator Author

I'll try to review your PRs soon.

@jfbenckhuijsen
Copy link
Contributor

@loicmathieu should be fixed with merging #707 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants