[AIRFLOW-7064] Add CloudFirestoreExportDatabaseOperator#7725

Merged

mik-laj merged 6 commits intoapache:masterfrom

PolideaInternal:firestore-gcloud

Mar 18, 2020

Member

mik-laj commented Mar 14, 2020 •

edited

Loading

It is WIP. I have to prepare documentation, but I would like to ask for opinions. gcloud is not a standard solution in operators. A more complex example that will use BigQuery would also be helpful.

Issue link: AIRFLOW-7064

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID^*
Unit tests coverage for changes (not needed for documentation changes)
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
[Xe] I will engage committers as explained in Contribution Workflow Example.

^* For document-only changes commit message can start with [AIRFLOW-XXXX].

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

mik-laj force-pushed the firestore-gcloud branch from 6dcbab9 to 8df4534 Compare

March 17, 2020 09:35

boring-cyborg bot added area:docs provider:google labels

mik-laj force-pushed the firestore-gcloud branch 2 times, most recently from 17fa2e4 to 3582f81 Compare

March 17, 2020 19:17


          [AIRFLOW-7064] Add CloudFirestoreExportDatabaseOperator

146dec8

mik-laj force-pushed the firestore-gcloud branch from 3582f81 to 146dec8 Compare

March 17, 2020 19:20

Kamil Breguła added 2 commits

March 18, 2020 04:46


          fixup! [AIRFLOW-7064] Add CloudFirestoreExportDatabaseOperator

90a26eb


          fixup! fixup! [AIRFLOW-7064] Add CloudFirestoreExportDatabaseOperator

9dc048e

mik-laj changed the title ~~[AIRFLOW-7064][WIP] Add CloudFirestoreExportDatabaseOperator~~ [AIRFLOW-7064] Add CloudFirestoreExportDatabaseOperator

mik-laj requested review from potiuk and turbaszek

March 18, 2020 06:18

potiuk reviewed

View reviewed changes

Member

potiuk left a comment

Just a few comment copy&paste victims to fix 👍

airflow/providers/google/firebase/hooks/firestore.py Outdated

+                  def get_conn(self):
+                      """
+                      Retrieves the connection to Cloud Build.

Member

potiuk Mar 18, 2020

Suggested change

      
                    Retrieves the connection to Cloud Build.
          
                    Retrieves the connection to Cloud Firestore.

airflow/providers/google/firebase/hooks/firestore.py Outdated

+                      """
+                      Retrieves the connection to Cloud Build.
+                      :return: Google Cloud Build services object.

Member

potiuk Mar 18, 2020

Suggested change

      
                    :return: Google Cloud Build services object.
          
                    :return: Google Cloud Firestore services object.

airflow/providers/google/firebase/hooks/firestore.py

+                          # then it will get the message below:
+                          # > Request contains an invalid argument.
+                          # At the same time, the Non-Authorized Client has no problems.
+                          non_authorized_conn = build("firestore", self.api_version, cache_discovery=False)

Member

potiuk Mar 18, 2020

:(

Member Author

mik-laj Mar 18, 2020

I created a ticket in google-api-python-client.
googleapis/google-api-python-client#845
I am not sure if this is a good place to report a bug, so I do not add a comment in the code. The error is in the service, not in the library, but I don't know a better place to report this error.

airflow/providers/google/firebase/hooks/firestore.py Outdated

+                      self, body: Dict, database_id: str = "(default)", project_id: Optional[str] = None
+                  ) -> None:
+                      """
+                      Starts a build with the specified configuration.

Member

potiuk Mar 18, 2020

Suggested change

      
                    Starts a build with the specified configuration.
          
                    Starts export with the specified configuration.

airflow/providers/google/firebase/hooks/firestore.py Outdated

+                          See:
+                          https://firebase.google.com/docs/firestore/reference/rest/v1beta1/projects.databases/exportDocuments
+                      :type body: dict
+                      :param project_id: Optional, Google Cloud Project project_id where the function belongs.

Member

potiuk Mar 18, 2020

Suggested change

      
                    :param project_id: Optional, Google Cloud Project project_id where the function belongs.
          
                    :param project_id: Optional, Google Cloud Project project_id where the database belongs.

turbaszek reviewed

View reviewed changes

Member

turbaszek left a comment

LGTM, some questions and additional copy-past fixes :)

airflow/providers/google/firebase/hooks/firestore.py

+                              .get(name=operation_name)
+                              .execute(num_retries=self.num_retries)
+                          )
+                          if operation_response.get("done"):

Member

turbaszek Mar 18, 2020

This is ok, however, I wonder isn't if "done" in operation_response more pythonic?

Member Author

mik-laj Mar 18, 2020 •

edited

Loading

We want to check if the done value is "true"

tests/providers/google/cloud/utils/gcp_authenticator.py

               GCP_SPANNER_KEY = 'gcp_spanner.json'
               GCP_TASKS_KEY = 'gcp_tasks.json'
               GMP_KEY = 'gmp.json'
+              G_FIREBASE_KEY = 'g_firebase.json'

Member

turbaszek Mar 18, 2020

Suggested change

      
            G_FIREBASE_KEY = 'g_firebase.json'
          
            G_FIREBASE_KEY = 'gcp_firebase.json'

Let's keep consistency in case of GCP :)

Member Author

mik-laj Mar 18, 2020

It's not GCP, but Firebase service. This database is available in the Pantheon, but is most often used by Firebase and was created as part of Firebase. It also has a common name with Firebase.

tests/providers/google/firebase/hooks/test_firestore.py Outdated

		TEST_PROJECT_ID = "firestore--project-id"


		class TestCloudBuildHookWithPassedProjectId(unittest.TestCase):

Member

turbaszek Mar 18, 2020

Suggested change

      
            class TestCloudBuildHookWithPassedProjectId(unittest.TestCase):
          
            class TestCloudFirestoreHookWithPassedProjectId(unittest.TestCase):

tests/providers/google/firebase/hooks/test_firestore.py Outdated

		self.hook.export_documents(body=EXPORT_DOCUMENT_BODY, project_id=TEST_PROJECT_ID)


		class TestGcpComputeHookWithDefaultProjectIdFromConnection(unittest.TestCase):

Member

turbaszek Mar 18, 2020

Suggested change

      
            class TestGcpComputeHookWithDefaultProjectIdFromConnection(unittest.TestCase):
          
            class TestCloudFirestoreHookWithDefaultProjectIdFromConnection(unittest.TestCase):

tests/providers/google/firebase/hooks/test_firestore.py Outdated

		self.hook.export_documents(body=EXPORT_DOCUMENT_BODY)


		class TestCloudBuildHookWithoutProjectId(unittest.TestCase):

Member

turbaszek Mar 18, 2020

Suggested change

      
            class TestCloudBuildHookWithoutProjectId(unittest.TestCase):
          
            class TestCloudFirestoreHookWithoutProjectId(unittest.TestCase):

tests/providers/google/firebase/operators/test_firestore_system.py Outdated

+              @pytest.mark.system("google.firebase")
+              @pytest.mark.system("google.cloud")
+              @pytest.mark.credential_file(G_FIREBASE_KEY)
+              class CampaignManagerSystemTest(GoogleSystemTest):

Member

turbaszek Mar 18, 2020

Suggested change

      
            class CampaignManagerSystemTest(GoogleSystemTest):
          
            class CloudFirestoreSystemTest(GoogleSystemTest):

tests/providers/google/firebase/operators/test_firestore_system.py

+                      super().tearDown()
+                  def clean_up(self):
+                      self.execute_with_ctx(["gsutil", "rm", "-r", f"{EXPORT_DESTINATION_URL}"], G_FIREBASE_KEY)

Member

turbaszek Mar 18, 2020

Can't we use delete_gcs_bucket method of GoogleSystemTest?

Member Author

mik-laj Mar 18, 2020

I do not want to delete the entire bucket, but only its contents, because this test has complex requirements for the location of the bucket and permissions. Information is available in example_firestore.py.

Kamil Breguła added 3 commits

March 18, 2020 09:12


          fixup! fixup! fixup! [AIRFLOW-7064] Add CloudFirestoreExportDatabaseO…

d4102a8

…perator


          fixup! fixup! fixup! fixup! [AIRFLOW-7064] Add CloudFirestoreExportDa…

0a8efac

…tabaseOperator


          fixup! fixup! fixup! fixup! fixup! [AIRFLOW-7064] Add CloudFirestoreE…

f4c60d6

…xportDatabaseOperator

turbaszek approved these changes

View reviewed changes

mik-laj merged commit 63a3102 into apache:master

mik-laj mentioned this pull request

Unable to create Firestore client when user is authorized googleapis/google-api-python-client#845

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

provider:google