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

Feature/firebase emulator config switch #707

Conversation

jfbenckhuijsen
Copy link
Contributor

No description provided.

@jfbenckhuijsen jfbenckhuijsen marked this pull request as draft November 4, 2024 12:54
… separated from the quarkus logic.

Added support for
- UI Logging port
- UI Hub endpoint
- Firestore websocket port
- Cloud Storage
…based on the Quarkus configuration into a separate class

Moved the exposed ports to be part of the container configuration.
@jfbenckhuijsen
Copy link
Contributor Author

jfbenckhuijsen commented Dec 17, 2024

@loicmathieu

Ok, quite a bit of work so far:

  • The code for the underlying FirebaseContainerEmulator has seen quite some changes. Most of them are to

    1. support some additional features (like enable debugging, pipe logging from the container to the host, additional flags to setup the emulator etc
    2. Added a Builder API to create the configuration, which results in quite a bit of cleaner code than before, especially when translating between the Quarkus config and the emulator config
    3. The emulator classes are pulled to a separate package
    4. The loading logic for a custom firebase.json file has been greatly expanded, meaning the code will detect the majority of the settings there and use that. I.e. if you create a new firebase project and use your firebase.json file (with some minimal changes) you're good to go and can easily switch between the normal emulator (running via terminal or whatever) and the dockerized version.
  • ConfigRoot issues should be resolved now. Everything is neatly pulled into a "q.g.c.devserices.*" config root

  • The creation code should automatically detect a firebase.json file in the root of the project (i.e. the current working directory for tests), so just including the module should be enough to get this running for any firebase project (the docs have been updated accordingly).

SCRATCH THIS BELOW, IM AN IDIOT ;)

The only issue I'm debugging right now (hence marked the branch as work in progress) is that the firebase-admin tests are failing. The issue is that the io.quarkiverse.googlecloudservices.firebase.admin.runtime.authentication.http.FirebaseSecurityAuthMechanism in the branch is not detected as a CDI bean for some reason. Haven't been able to figure out why. Injecting it from a test inside the module itself works fine, but for some reason I can't inject it from the firebase-auth integration test module

@jfbenckhuijsen
Copy link
Contributor Author

@loicmathieu question w.r.t. the currently failing build. Cause of that is that the tests try to validate that a function is running, which requires the packages needed for that function are installed (i.e. run npm install). As this hasn't been done in the build, it fails. Currently see the following options to resolve this:

  1. Disable the unit test fully. Functions work, but we won't see any regressions
  2. Run a npm install before running the maven build to get those dependencies installed
  3. Move those tests to the integration tests and only run them manually.
  4. ...?

any preference?

@loicmathieu
Copy link
Collaborator

If a simple npm install would make the test pass, let's do it!
If it's more complex, move it to integration tests and runs it manually.

Copy link
Collaborator

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

There is a lot of config/rules file, I'm not sure what it's used for, if we really need them, and if they should not be moved to META-INF so they are included in the JAR and loaded from there.

@jfbenckhuijsen
Copy link
Contributor Author

If a simple npm install would make the test pass, let's do it! If it's more complex, move it to integration tests and runs it manually.

Its a simple npm install, I'll put that in the actions

@jfbenckhuijsen
Copy link
Contributor Author

There is a lot of config/rules file, I'm not sure what it's used for, if we really need them, and if they should not be moved to META-INF so they are included in the JAR and loaded from there.

To summarize, those are all used for testing purposes, not used at runtime, at kept in those locations as this mimics the way a regular Firebase project would be setup, i.e. having those files there is part of the test setup.

@loicmathieu
Copy link
Collaborator

To summarize, those are all used for testing purposes, not used at runtime

OK, great, understood.
In this case, they must be inside the test directory, not anywhere else.

@loicmathieu
Copy link
Collaborator

Everything that is test related should either be in src/test or in an integration-test module.

@jfbenckhuijsen
Copy link
Contributor Author

If a simple npm install would make the test pass, let's do it! If it's more complex, move it to integration tests and runs it manually.

Fixed:
f353ec0
39608c8

@jfbenckhuijsen
Copy link
Contributor Author

Everything that is test related should either be in src/test or in an integration-test module.

Fixed: 8be975c

@jfbenckhuijsen
Copy link
Contributor Author

All comments processed, your turn again ;)

Copy link
Collaborator

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!
This is a huge contribution.

I'll certainly do a round of cleanup later, if so, I'll send you the PR for validation.
I also didn't check the doc in details.
But overall, the PR is a nice addition so I'll merge it as is as it's already too old ;).

Thanks again for your patience

@loicmathieu loicmathieu merged commit 83718a8 into quarkiverse:main Dec 24, 2024
1 check passed
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

Successfully merging this pull request may close these issues.

4 participants