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

🎉 New Destination: Google Firestore #7231

Merged
merged 10 commits into from
Nov 21, 2021

Conversation

ad-m
Copy link
Contributor

@ad-m ad-m commented Oct 21, 2021

What

That PR add connectors to write to Google Firestore. Connector have been implemented in Python.

That PR is part of Airbyte hacktoberfest contest

How

Describe the solution

I have implemented connectors in Python. It support application default credentials (including Google metadata service when Airbyte is running on Google-backed insfructure eg. virtual machines) and JSON credentials.

I have been inspired by existing kvdb destination to keep code coherent.

Considering the existing cooperation between Google and Airbyte, I am not sure how to proceed with the access data to the demo environment, and also to prove the operation.

$ python -m pytest integration_tests --full-trace
=============================================================================================== test session starts ===============================================================================================
platform linux -- Python 3.9.4, pytest-6.2.5, py-1.10.0, pluggy-1.0.0 -- /home/adas/Devel/airbyte/airbyte-integrations/connectors/destination-firestore/.venv/bin/python
cachedir: .pytest_cache
rootdir: /home/adas/Devel/airbyte, configfile: pytest.ini
collected 4 items                                                                                                                                                                                                 

integration_tests/integration_test.py::test_check_valid_config PASSED
integration_tests/integration_test.py::test_check_invalid_config PASSED
integration_tests/integration_test.py::test_write_append PASSED
integration_tests/integration_test.py::test_write_overwrite PASSED

================================================================================================ 4 passed in 5.80s ================================================================================================

Recommended reading order

  1. y.python

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

  • Community member? Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • docs/SUMMARY.md
    • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
    • docs/integrations/README.md
    • airbyte-integrations/builds.md
  • PR name follows PR naming conventions
  • Connector added to connector index like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub by running the /publish command described here

@CLAassistant
Copy link

CLAassistant commented Oct 21, 2021

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Oct 21, 2021
@ad-m ad-m changed the title 🎉 Destination Firestore: add initial implementation of new connection 🎉 New Destionation: Google Firestore Oct 21, 2021
@ad-m ad-m changed the title 🎉 New Destionation: Google Firestore 🎉 New Destination: Google Firestore Oct 21, 2021
@harshithmullapudi
Copy link
Contributor

Hey @ad-m thanks for your contribution. Can you share you credentials over slack (DM) so that I can test this source.

@ad-m
Copy link
Contributor Author

ad-m commented Oct 21, 2021

@harshithmullapudi I believe I already shared credentials via Slack.

@marcosmarxm
Copy link
Member

thanks for this contribution @ad-m I requested the review to our team.

@mik-laj
Copy link

mik-laj commented Oct 25, 2021

@marcosmarxm;what does gl label mean?

Copy link
Contributor

@antixar antixar left a comment

Choose a reason for hiding this comment

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

Hi @ad-m,
Thank you for this PR.
Could you correct the following points?

  • add Python annotation to your writer.py
  • does this library support the native oauth2 auth (not system account)? I mean some refresh_token with client_id/client_secret. I ask because The Aitbyte project is planning to add support of Oauth2 user authorization for connectors which can use it.

Comment on lines 12 to 15
def __init__(self, project_id=None, credentials_json=None, collection=None):
connection = {}

if project_id:
Copy link
Contributor

Choose a reason for hiding this comment

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

is the project_id optional or not? I see to it is required into your spec file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, it's requried. Fixed.

Comment on lines +8 to +9
from google.oauth2 import service_account

Copy link
Contributor

Choose a reason for hiding this comment

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

this library is not set as dependence obviously into the setup file. I see that it is installed with the google-cloud-firestore lib but it isn't evident. Could you add all used libraries into the setup file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All used libraries added into the setup file.

@ad-m
Copy link
Contributor Author

ad-m commented Oct 29, 2021

  • add Python annotation to your writer.py

OK.

  • does this library support the native oauth2 auth (not system account)? I mean some refresh_token with client_id/client_secret. I ask because The Aitbyte project is planning to add support of Oauth2 user authorization for connectors which can use it.

Yes, that library might be used for OAuth 2.0 flow: https://google-auth.readthedocs.io/en/master/user-guide.html#user-credentials

@ad-m ad-m requested a review from antixar November 4, 2021 18:06
@ad-m
Copy link
Contributor Author

ad-m commented Nov 4, 2021

@marcosmarxm is there anything how I can push forward that PR?

@harshithmullapudi
Copy link
Contributor

@antixar is it a +1 for you on this PR? can I process to publish this ?

@marcosmarxm
Copy link
Member

@ad-m please share the acceptance tests screenshot showing those tests are passing too.

@ad-m
Copy link
Contributor Author

ad-m commented Nov 5, 2021

@marcosmarxm is there any documentation regarding how to use acceptance tests for destination connector in Python?

I can see that there are open issue to enable Destination Acceptance Tests cross-language support ( #4698 ) and improve documentation for DAT ( airbytehq/airbyte-internal-issues#268 ). However, these issues do not imply that such a standard exists for destinations connector already.

Tests pass despite of small tweaks since PR have been created:

image

@ad-m
Copy link
Contributor Author

ad-m commented Nov 8, 2021

@marcosmarxm , according to @sherifnada :

Currently the only available option is to write acceptance tests in java

As it stands, should I try to create a mixed connector that would run Java acceptance tests for the Python connector (I haven't analyzed whether it makes sense to accept acceptance tests due to the level of abstraction)? I do not have a full picture of the management and standards of the project yet, hence this question. I have the impression that mixing languages in one connector will be problematic in the long run.

Copy link
Contributor

@antixar antixar left a comment

Choose a reason for hiding this comment

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

Looks good! Sorry for delay of this review

@marcosmarxm
Copy link
Member

Hello! Sorry to not finished your contribution before the date stipulated in the contest. All contributions made before 15-November are eligible to receive the award. We're trying to review your contribution as soon as possible.

@marcosmarxm
Copy link
Member

@harshithmullapudi can you ship this one?

@ad-m
Copy link
Contributor Author

ad-m commented Nov 15, 2021

Hello! Sorry to not finished your contribution before the date stipulated in the contest. All contributions made before 15-November are eligible to receive the award. We're trying to review your contribution as soon as possible.

Is there anything how I can help to handle that PR?

@harshithmullapudi
Copy link
Contributor

Yeah @marcosmarxm just trying to understand is there anything more we need to do other than publishing the connector ?

@harshithmullapudi
Copy link
Contributor

@ad-m hey were you able to build docker image of this connector ?

@harshithmullapudi harshithmullapudi merged commit 519ea2a into airbytehq:master Nov 21, 2021
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
* destination-firestore: add connector

* destination-firestore: Add connector to indexes in docs

* destination-firestore: fix sync mode spec

* destination-firestore: fix typo after connector rename

* destination-firestore: fix required field in spec

* destination firestore: fix code formatting

* destionation-firestore: add python adnotation for writer

* destination firestore: add all used libraries into the setup file

* destionation-firestore: fix python formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: No status
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants