ahidalgob commented on code in PR #32843: URL: https://github.com/apache/airflow/pull/32843#discussion_r1277374908
########## airflow/providers/google/cloud/example_dags/google-datapipeline.py: ########## Review Comment: Following [AIP-47](https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-47+New+design+of+Airflow+System+Tests), we want to introduce new example dags as system tests in tests/system/providers/google/cloud/. Please migrate this DAG into that other folder following the new desingn. ########## airflow/providers/google/cloud/example_dags/google-datapipeline.py: ########## @@ -0,0 +1,99 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Example Airflow DAG for testing Google Dataflow Beam Pipeline Operator with Java. +Important Note: + This test downloads Java JAR file from the public bucket. In case the JAR file cannot be downloaded + or is not compatible with the Java version used in the test, the source code for this test can be + downloaded from here (https://beam.apache.org/get-started/wordcount-example) and needs to be compiled + manually in order to work. + You can follow the instructions on how to pack a self-executing jar here: + https://beam.apache.org/documentation/runners/dataflow/ +Requirements: + These operators require the gcloud command and Java's JRE to run. +""" +from __future__ import annotations + +import os +from datetime import datetime + +from airflow import models +from airflow.providers.apache.beam.hooks.beam import BeamRunnerType +from airflow.providers.google.cloud.operators.datapipeline import ( + CreateDataPipelineOperator, + RunDataPipelineOperator, +) +from airflow.providers.google.cloud.operators.gcs import GCSCreateBucketOperator, GCSDeleteBucketOperator +from airflow.providers.google.cloud.transfers.gcs_to_local import GCSToLocalFilesystemOperator +from airflow.utils.trigger_rule import TriggerRule + +DAG_ID = "google-datapipeline" + +GCP_PROJECT_ID = os.environ.get("GCP_PROJECT_ID", "example-project") +GCP_LOCATION = os.environ.get("location", "us-central1") +PIPELINE_NAME = "defualt-pipeline-name" +PIPELINE_TYPE = "PIPELINE_TYPE_BATCH" + +DATAPIPELINES_JOB_NAME = os.environ.get("GCP_DATA_PIPELINES_FLEX_TEMPLATE_JOB_NAME", "default-job-name") + +GCS_FLEX_TEMPLATE_TEMPLATE_PATH = os.environ.get( + "GCP_DATA_PIPELINES_GCS_FLEX_TEMPLATE_TEMPLATE_PATH", + "gs://INSERT BUCKET/templates/word-count.json", +) + +TEMP_LOCATION = os.environ.get("GCP_DATA_PIPELINES_GCS_TEMP_LOCATION", "gs://INSERT BUCKET/temp") +INPUT_FILE = os.environ.get("GCP_DATA_PIPELINES_INPUT_FILE", "gs://INSERT BUCKET/examples/kinglear.txt") +OUTPUT = os.environ.get("GCP_DATA_PIPELINES_OUTPUT", "gs://INSERT BUCKET/results/hello") Review Comment: Seems to me this needs 2 files as resources: `word-count.json` and `kinglear.txt`. It would be better to have those 2 files as resources in this repo (see for example `tests/system/providers/google/cloud/dataflow/resources`) and within this same DAG create a bucket and upload those files to it, reducing the amount of external dependencies needed to run this DAG. Also, would this eliminate the need of an external bucket completely? See for example `tests/system/providers/google/cloud/dataflow/example_dataflow_template.py` ########## airflow/providers/google/cloud/example_dags/google-datapipeline.py: ########## @@ -0,0 +1,99 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Example Airflow DAG for testing Google Dataflow Beam Pipeline Operator with Java. +Important Note: + This test downloads Java JAR file from the public bucket. In case the JAR file cannot be downloaded + or is not compatible with the Java version used in the test, the source code for this test can be + downloaded from here (https://beam.apache.org/get-started/wordcount-example) and needs to be compiled + manually in order to work. + You can follow the instructions on how to pack a self-executing jar here: + https://beam.apache.org/documentation/runners/dataflow/ +Requirements: + These operators require the gcloud command and Java's JRE to run. Review Comment: Is this true for this DAG? Could you please document the external resources needed for this dag to run? ########## airflow/providers/google/cloud/hooks/datapipeline.py: ########## @@ -0,0 +1,88 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""This module contains a Google Data Pipelines Hook.""" +from __future__ import annotations + +from googleapiclient.discovery import build + +from airflow.providers.google.common.hooks.base_google import ( + GoogleBaseHook, +) + +DEFAULT_DATAPIPELINE_LOCATION = "us-central1" + + +class DataPipelineHook(GoogleBaseHook): + """ + Hook for Google Data Pipelines. + All the methods in the hook where project_id is used must be called with + keyword arguments rather than positional. + """ + + def __init__( + self, + gcp_conn_id: str = "google_cloud_default", + impersonation_chain: str | Sequence[str] | None = None, + **kwargs, + ) -> None: + super().__init__( + gcp_conn_id=gcp_conn_id, + impersonation_chain=impersonation_chain, + ) + + def get_conn(self) -> build: + """Returns a Google Cloud Data Pipelines service object.""" + http_authorized = self._authorize() + return build("datapipelines", "v1", http=http_authorized, cache_discovery=False) + + @GoogleBaseHook.fallback_to_default_project_id + def create_data_pipeline( + self, + body: dict, + project_id: str, + location: str = DEFAULT_DATAPIPELINE_LOCATION, + ) -> None: + """ + Creates a new Data Pipelines instance from the Data Pipelines API. + + :param body: The request body (contains instance of Pipeline). See: + https://cloud.google.com/dataflow/docs/reference/data-pipelines/rest/v1/projects.locations.pipelines/create#request-body + :param project_id: The ID of the GCP project that owns the job. + :param location: The location to direct the Data Pipelines instance to (example_dags uses uscentral-1). Review Comment: ```suggestion :param location: The location to direct the Data Pipelines instance to (for example uscentral-1). ``` ########## airflow/providers/google/cloud/example_dags/google-datapipeline.py: ########## @@ -0,0 +1,99 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Example Airflow DAG for testing Google Dataflow Beam Pipeline Operator with Java. +Important Note: + This test downloads Java JAR file from the public bucket. In case the JAR file cannot be downloaded + or is not compatible with the Java version used in the test, the source code for this test can be + downloaded from here (https://beam.apache.org/get-started/wordcount-example) and needs to be compiled + manually in order to work. + You can follow the instructions on how to pack a self-executing jar here: + https://beam.apache.org/documentation/runners/dataflow/ +Requirements: + These operators require the gcloud command and Java's JRE to run. +""" +from __future__ import annotations + +import os +from datetime import datetime + +from airflow import models +from airflow.providers.apache.beam.hooks.beam import BeamRunnerType +from airflow.providers.google.cloud.operators.datapipeline import ( + CreateDataPipelineOperator, + RunDataPipelineOperator, +) +from airflow.providers.google.cloud.operators.gcs import GCSCreateBucketOperator, GCSDeleteBucketOperator +from airflow.providers.google.cloud.transfers.gcs_to_local import GCSToLocalFilesystemOperator +from airflow.utils.trigger_rule import TriggerRule + +DAG_ID = "google-datapipeline" + +GCP_PROJECT_ID = os.environ.get("GCP_PROJECT_ID", "example-project") Review Comment: ```suggestion ENV_ID = os.environ.get("SYSTEM_TESTS_ENV_ID") PROJECT_ID = os.environ.get("SYSTEM_TESTS_GCP_PROJECT") ``` Following other GCP system tests. ########## airflow/providers/google/cloud/operators/datapipeline.py: ########## @@ -0,0 +1,81 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""This module contains Google Data Pipelines operators.""" +from __future__ import annotations + +from airflow import AirflowException +from airflow.providers.google.cloud.hooks.datapipeline import DEFAULT_DATAPIPELINE_LOCATION, DataPipelineHook +from airflow.providers.google.cloud.operators.cloud_base import GoogleCloudBaseOperator + + +class CreateDataPipelineOperator(GoogleCloudBaseOperator): Review Comment: Are we missing `impersonation_chain` here? ########## airflow/providers/google/cloud/hooks/datapipeline.py: ########## @@ -0,0 +1,88 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""This module contains a Google Data Pipelines Hook.""" +from __future__ import annotations + +from googleapiclient.discovery import build + +from airflow.providers.google.common.hooks.base_google import ( + GoogleBaseHook, +) + +DEFAULT_DATAPIPELINE_LOCATION = "us-central1" Review Comment: What's the reasoning behind using a default location? I think normally this is expected to be a required parameter, isn't it? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
