Skip to content

Conversation

@leahecole
Copy link
Contributor

Hi all! This is related to #23129. In BigQuery, fully qualified dataset names can be made using the GCP project ID even if the project ID isn't needed for the underlying API to work. It's not unusual to make project_id a default arg and to have it be templated, especially if you're using many BQ operators together. I've added it to two operators where I have used it to construct a dataset name.

I'm not 100% sure how we test templated fields - I looked at the BQ operator test and didn't see any kind of validation, and the test values we're passing to the mocked calls are not templated. Any recommendations for how to handle that are appreciated!

@leahecole leahecole requested a review from turbaszek as a code owner June 30, 2022 20:09
@boring-cyborg boring-cyborg bot added area:providers provider:google Google (including GCP) related issues labels Jun 30, 2022
@leahecole leahecole requested a review from eladkal June 30, 2022 20:09
@eladkal
Copy link
Contributor

eladkal commented Jun 30, 2022

I'm not 100% sure how we test templated fields - I looked at the BQ operator test and didn't see any kind of validation, and the test values we're passing to the mocked calls are not templated. Any recommendations for how to handle that are appreciated!

You can check this example:

def test_templates(self, _, create_task_instance_of_operator):
dag_id = 'TestS3ToGoogleCloudStorageTransferOperator_test_templates'
ti = create_task_instance_of_operator(
CloudDataTransferServiceS3ToGCSOperator,
dag_id=dag_id,
s3_bucket='{{ dag.dag_id }}',
gcs_bucket='{{ dag.dag_id }}',
description='{{ dag.dag_id }}',
object_conditions={'exclude_prefixes': ['{{ dag.dag_id }}']},
gcp_conn_id='{{ dag.dag_id }}',
task_id=TASK_ID,
)
ti.render_templates()
assert dag_id == ti.task.s3_bucket
assert dag_id == ti.task.gcs_bucket
assert dag_id == ti.task.description
assert dag_id == ti.task.object_conditions['exclude_prefixes'][0]
assert dag_id == ti.task.gcp_conn_id

@eladkal
Copy link
Contributor

eladkal commented Jul 1, 2022

@leahecole I'm fine with merging this without the test (most operators don't have it)

can you please fix the static tests?

@leahecole
Copy link
Contributor Author

Yup - sorry, was OOO for a US holiday, I'll look into it in the next couple of days

@potiuk potiuk merged commit 77626b7 into apache:main Jul 13, 2022
@leahecole
Copy link
Contributor Author

ah - you beat me to it, sorry for slowness! been focused on my intern. thank you @potiuk and @eladkal

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

Labels

area:providers provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants