-
Notifications
You must be signed in to change notification settings - Fork 339
feat(rtdb): Support RTDB Emulator via FIREBASE_DATABASE_EMULATOR_HOST. #313
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
Conversation
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 @yuchenshi for implementing this. It looks pretty good. I added a few suggestions on how the code can be cleaned up a bit.
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.
Looks pretty good. I've pointed out a few things that can be done to make the code a bit more clear and hopefully simpler.
firebase_admin/db.py
Outdated
return base_url, {'ns': namespace}, use_fake_creds | ||
|
||
@classmethod | ||
def _parse_production_url(cls, parsed_url): |
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.
Make this and _parse_db_url instance methods. Then you don't need to pass emulator_host around. It can be read from self.
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, but no, thanks. I just tried that and it made testing a nightmare. I don't want to initialize a full _DatabaseService
class with all the options and monkey-patch os.environ
just to test the parsing logic.
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 @yuchenshi. Looks pretty good. Just a couple of nits and questions. We can merge once they are resolved.
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. LGTM 👍
Discussion
(Discussed internally.)
Testing
I've manually ran the integration tests with and without the emulator and verified that they pass.
We may consider running the emulator-based integration test on Travis in a follow up PR.
API Changes
No public API change except FakeCredential and the FIREBASE_DATABASE_EMULATOR_HOST env var.
RELEASE NOTE: Developers can now test their {{database}} API calls by directing the SDK traffic to the RTDB emulator. Set the
FIREBASE_DATABASE_EMULATOR_HOST
environment variable to specify the emulator endpoint inhost:port
format.