feat(firestore): Added multiple firestore database support #719
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
lahirumaramba
changed the title
| self._client = firestore.Client(credentials=credentials, project=project) | ||
| def __init__(self, credentials, project, database_id=None): | ||
| if database_id: | ||
| self._client = firestore.Client(credentials=credentials, project=project, database=database_id) |
There was a problem hiding this comment.
Wasn't sure the best way to do this but I included a version check if attempting to use older that v2.12.0
There was a problem hiding this comment.
I think we can update the firestore version to the latest in the next release so this check probably won't be necessary (we don't check the version in other SDKs). I understand this PR has been sitting here for a while so I will first merge this and then remove the version check in a follow up PR :)
firebase_admin/_utils.py
Outdated
| @@ -93,8 +93,10 @@ def _get_initialized_app(app): | |||
|
|
|||
|
|
|||
|
|
|||
| def get_app_service(app, name, initializer): | |||
| def get_app_service(app, options, name, initializer): | |||
There was a problem hiding this comment.
I see this is a private module, but adding a new positional argument into the middle of the list seems dangerous. Can this be an optional kwarg?
| self._client = firestore.Client(credentials=credentials, project=project, database=database_id) | ||
| else: | ||
| self._client = firestore.Client(credentials=credentials, project=project) | ||
|
|
There was a problem hiding this comment.
we should add some tests for this new functionality
There was a problem hiding this comment.
I added a test to cover when an explicit database id is given
lahirumaramba
requested review from
lahirumaramba
and removed request for
tom-andersen and
wu-hui
Check firestore ver is > 2.12.0 is using database id Added test for explicit database id
|
Any ETA on when this will be pushed? Urgently need this feature. |
| self._client = firestore.Client(credentials=credentials, project=project) | ||
| def __init__(self, credentials, project, database_id=None): | ||
| if database_id: | ||
| self._client = firestore.Client(credentials=credentials, project=project, database=database_id) |
There was a problem hiding this comment.
I think we can update the firestore version to the latest in the next release so this check probably won't be necessary (we don't check the version in other SDKs). I understand this PR has been sitting here for a while so I will first merge this and then remove the version check in a follow up PR :)
#718 Added support for multiple Firestore databases within a single app