Repository navigation
Add network tags to google cloud operator - #40630
kunaljubce wants to merge 9 commits into
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
|
Static checks failing (I recommend installing pre-commit https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst to fix it automatically). |
I am looking into these @potiuk, thanks for your review! I will confirm here once this PR is ready for review again. |
moiseenkov
left a comment
There was a problem hiding this comment.
Thank you for the contribution!
Could you please update the PR description and the title so it would be clear that changes affect only the DataprocCreateBatchOperator (not every google operator).
| project_id=self.project_id, | ||
| region=self.region, | ||
| gcp_conn_id=self.gcp_conn_id, | ||
| tags=self.tags, |
There was a problem hiding this comment.
In my opinion we shouldn't pass tags to the trigger because the only thing it is responsible for - is polling the batch by batch_id. Because the batch_id is unique and sufficient for the batch identification, we probably shouldn't use tags here. Moreover, this PR doesn't contain any changes for the DataprocBatchTrigger, so it doesn't even accept this parameter.
| batch_id=self.batch_id, | ||
| region=self.region, | ||
| project_id=self.project_id, | ||
| tags=self.tags, |
There was a problem hiding this comment.
The same here. If we are waiting for the batch, batch_id is enough to identify it, so we don't have to use rags here.
| context, | ||
| key=DataprocBatchLink.key, | ||
| value={"batch_id": batch_id, "region": region, "project_id": project_id}, | ||
| value={"batch_id": batch_id, "region": region, "project_id": project_id, "tags": tags}, |
There was a problem hiding this comment.
Does the batch URL contain network tags?
There was a problem hiding this comment.
@moiseenkov I am verifying this, will confirm in a couple of days.
|
Closing this as Not Needed since the related issue #40232 is closed now. |
closes: #40232
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.