You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Ajay9704 (via GitHub)" <gi...@apache.org> on 2026/01/31 10:09:06 UTC
[PR] Return full GCS URIs from Google Calendar and Display Video operators [airflow]
Ajay9704 opened a new pull request, #61284:
URL: https://github.com/apache/airflow/pull/61284
I improved the return values of two Google ToGCS operators to provide full gs:// URIs instead of partial paths, making them more consistent and directly usable.
Changes made:
GoogleCalendarToGCSOperator - Now returns complete GCS URIs (e.g. gs://bucket/calendar_id.json) instead of just object names
GoogleDisplayVideo360SDFtoGCSOperator - Now returns proper GCS URIs (e.g., gs://bucket/object_name) instead of bucket/object format
These changes address part of issue #11323 by standardizing the return format to provide more useful destination URIs for users
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar and Display Video operators [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on PR #61284:
URL: https://github.com/apache/airflow/pull/61284#issuecomment-3828683547
@Ajay9704
Could you please run `prek run --all-files`, and fix manually where needed? (it seems that you just did)
One last thing (half related) - it seems that some of the tests are written using Python's native unittest module, instead of pytest. I'll carefully assume that it is due to the usage of AI, which is not forbidden - but it should be supervised and declared explicitly in the PR's description. Please refer to the related [docs](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions) and note for your next PRs.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar [airflow]
Posted by "yuseok89 (via GitHub)" <gi...@apache.org>.
yuseok89 commented on code in PR #61284:
URL: https://github.com/apache/airflow/pull/61284#discussion_r2787742807
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
@@ -159,9 +174,9 @@ def _upload_data(
object_name=dest_file_name,
filename=temp_file.name,
)
- return dest_file_name
+ return f"gs://{self.destination_bucket}/{dest_file_name}"
Review Comment:
@shahar1
Just a question. Both `unwrap_single` and the suggested `return_gcs_uri` flag seem to exist mainly to avoid breaking changes. Would it make sense to consolidate them into a single flag in this case? Or do you prefer keeping both flags for clarity and consistency with other operators?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #61284:
URL: https://github.com/apache/airflow/pull/61284#discussion_r2787851560
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
@@ -159,9 +174,9 @@ def _upload_data(
object_name=dest_file_name,
filename=temp_file.name,
)
- return dest_file_name
+ return f"gs://{self.destination_bucket}/{dest_file_name}"
Review Comment:
> @shahar1 Just a question. Both `unwrap_single` and the suggested `return_gcs_uri` flag seem to exist mainly to avoid breaking changes. Would it make sense to consolidate them into a single flag in this case? Or do you prefer keeping both flags for clarity and consistency with other operators?
Good question!
I prefer keeping both flags for clarity and consistency with the other operators.
The `return_gcs_uri` should be deprecated first out of the two.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #61284:
URL: https://github.com/apache/airflow/pull/61284#discussion_r2791598925
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
@@ -108,6 +114,8 @@ def __init__(
destination_path: str | None = None,
gcp_conn_id: str = "google_cloud_default",
impersonation_chain: str | Sequence[str] | None = None,
+ unwrap_single: bool | None = None,
+ return_gcs_uri: bool | None = False,
Review Comment:
```suggestion
return_gcs_uri: bool | None = None,
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #61284:
URL: https://github.com/apache/airflow/pull/61284#discussion_r2787581438
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
@@ -159,9 +174,9 @@ def _upload_data(
object_name=dest_file_name,
filename=temp_file.name,
)
- return dest_file_name
+ return f"gs://{self.destination_bucket}/{dest_file_name}"
Review Comment:
I went over the PR again and noticed that changing the returned value from `dest_file_name` to the full URI could be a breaking change, so we should deprecate it slowly (similar to the `unwrap_single=False`).
Add another flag `return_gcs_uri` with `False` as default.
Only if `unwrap_single` is also `False`, it should return `dest_file_name`.
`return_gcs_uri` cannot be set to `True` altogether with `unwrap_single=True` (it should raise `ValueError`, as in the future we plan to align all operators with returning list of URIs.
Please handle these case, including unit tests.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #61284:
URL: https://github.com/apache/airflow/pull/61284#discussion_r2787581438
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
@@ -159,9 +174,9 @@ def _upload_data(
object_name=dest_file_name,
filename=temp_file.name,
)
- return dest_file_name
+ return f"gs://{self.destination_bucket}/{dest_file_name}"
Review Comment:
I went over the PR again and noticed that changing the returned value from `dest_file_name` to the full URI could be a breaking change, so we should deprecate it slowly (similar to the `unwrap_single=False`).
Add another flag `return_gcs_uri` with `False` as default.
Only if `unwrap_single` is also `False`, it should return `dest_file_name`.
`return_gcs_uri` cannot be set to `True` altogether with `unwrap_single=True` (it should raise `ValueError`.
Please handle these use cases including unit tests.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar and Display Video operators [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on PR #61284:
URL: https://github.com/apache/airflow/pull/61284#issuecomment-3829022753
> > > @shahar1 i am unable to understand what went wrong can you please help me with this
> > > > [Tests / Integration and System Tests / Integration: providers celery (pull_request)] (https://github.com/apache/airflow/actions/runs/21547359138/job/62091006890?pr=61284)Failing after 8m
> >
> > Sure! I tried to rerun it - it doesn't seem related to this PR specifically, so it's either a flaky integration test or something else that should be solved by merging from main. Please leave this PR as-is for now and feel free to work on other issues, I'll take care of merging it once it's solved.
>
> Sure sir, and thank you for helping throughout
Seems to pass succesfully :)
Before merging, I want to give a couple of days for the Google provider team to review it if they have additional comments.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar and Display Video operators [airflow]
Posted by "Ajay9704 (via GitHub)" <gi...@apache.org>.
Ajay9704 commented on PR #61284:
URL: https://github.com/apache/airflow/pull/61284#issuecomment-3828125632
> 1. Please read my edit to the original issue regarding handling of single entities - we should have a flag for returning a list of strings (`unwrap_single`).
> 2. Please add unit tests.
> 3. Before creating new PRs related to this issue, please ask for assignment in the original issue (I've assigned you for these two operators - but please consider it for the next times).
I've made changes accordingly , please review and let me know if anything else is required
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #61284:
URL: https://github.com/apache/airflow/pull/61284#discussion_r2788543777
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
@@ -75,6 +75,11 @@ class GoogleCalendarToGCSOperator(BaseOperator):
If set as a sequence, the identities from the list must grant
Service Account Token Creator IAM role to the directly preceding identity, with first
account from the list granting this role to the originating account (templated).
+ :param unwrap_single: If True (default), returns a single URI string when there's only one file.
+ If False, always returns a list of URIs. Default will change to False in a future release.
+ :param return_gcs_uri: If True, returns the full GCS URI (e.g., ``gs://bucket/path/to/file``).
+ If False (default), returns the destination file name only (e.g., ``bucket/path/to/file``).
+ This parameter cannot be used together with ``unwrap_single=True``.
Review Comment:
```suggestion
When this parameter's value is false, it cannot be used altogether with ``unwrap_single=False``.
```
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
@@ -132,11 +139,32 @@ def __init__(
self.destination_bucket = destination_bucket
self.destination_path = destination_path
self.impersonation_chain = impersonation_chain
+ if unwrap_single is None:
+ self.unwrap_single = True
+ import warnings
+
+ warnings.warn(
+ "The default value of unwrap_single will change from True to False in a future release. "
+ "Please set unwrap_single explicitly to avoid this warning.",
+ FutureWarning,
+ stacklevel=2,
+ )
+ else:
+ self.unwrap_single = unwrap_single
+
+ self.return_gcs_uri = return_gcs_uri
+
+ if self.unwrap_single and self.return_gcs_uri:
+ raise ValueError(
+ "return_gcs_uri cannot be True together with unwrap_single=True. "
+ "In the future, all operators will return list of URIs, so please set "
+ "unwrap_single=False when using return_gcs_uri=True."
+ )
Review Comment:
```suggestion
if not self.unwrap_single and not self.return_gcs_uri:
raise ValueError(
"return_gcs_uri cannot be False together with unwrap_single=False. "
"In the future, all operators will return list of URIs, so please set "
"unwrap_single=False when using return_gcs_uri=True."
)
```
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
@@ -108,6 +113,8 @@ def __init__(
destination_path: str | None = None,
gcp_conn_id: str = "google_cloud_default",
impersonation_chain: str | Sequence[str] | None = None,
+ unwrap_single: bool | None = None,
+ return_gcs_uri: bool = False,
Review Comment:
```suggestion
return_gcs_uri: bool | None = False,
```
+ Add a similar deprecation warning to `unwrap_single` (its value will be set to True in future release)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar [airflow]
Posted by "Ajay9704 (via GitHub)" <gi...@apache.org>.
Ajay9704 commented on code in PR #61284:
URL: https://github.com/apache/airflow/pull/61284#discussion_r2791705052
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
@@ -108,6 +114,8 @@ def __init__(
destination_path: str | None = None,
gcp_conn_id: str = "google_cloud_default",
impersonation_chain: str | Sequence[str] | None = None,
+ unwrap_single: bool | None = None,
+ return_gcs_uri: bool | None = False,
Review Comment:
Done sir
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar and Display Video operators [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on PR #61284:
URL: https://github.com/apache/airflow/pull/61284#issuecomment-3858734445
Are you able to run the system tests for the changes operators? Google team asks to do that before merging to ensure that they still pass after making changes.
If you're not able to do so, we'll have to wait until Google team or someone else does it (if it takes too long I'll handle it on my own).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar and Display Video operators [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on PR #61284:
URL: https://github.com/apache/airflow/pull/61284#issuecomment-3875801525
> @shahar1 I ran the system tests for `GoogleCalendarToGCSOperator`.
>
> ## Test Screenshots
> ### unrwap_single = True
> <img alt="image" width="1044" height="162" src="https://private-user-images.githubusercontent.com/3938973/547101411-21640340-a1d5-4f4a-a03d-5e2ab260e58e.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NzA3MDgyMjAsIm5iZiI6MTc3MDcwNzkyMCwicGF0aCI6Ii8zOTM4OTczLzU0NzEwMTQxMS0yMTY0MDM0MC1hMWQ1LTRmNGEtYTAzZC01ZTJhYjI2MGU1OGUucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI2MDIxMCUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNjAyMTBUMDcxODQwWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NzU2NDU1NmM0YjJjNWVjMDMzNzdkNzZmYTMzZDZkMWI2OWUwOGM4NzRmMWE0MTM3Mjg2MTU2OGU5M2M0YjYxOSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.zxbEXvqgXqwDEbCjnmVyjE_UwdQqRGRMePIviRZO3iY">
> ### unrwap_single = False
> <img alt="image" width="1044" height="177" src="https://private-user-images.githubusercontent.com/3938973/547101179-cbe0a3cf-3a14-4ac0-a913-87157b69cd7d.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NzA3MDgyMjAsIm5iZiI6MTc3MDcwNzkyMCwicGF0aCI6Ii8zOTM4OTczLzU0NzEwMTE3OS1jYmUwYTNjZi0zYTE0LTRhYzAtYTkxMy04NzE1N2I2OWNkN2QucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI2MDIxMCUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNjAyMTBUMDcxODQwWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9OTg0ZmE4MmMyYzYyZWUyNDcxOWY5YTU2YTM1ODc2OGNjOGFmZDBiY2Q1OTBiY2IwYjIzMjYxNmNiYzM4MDJlNyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.oav-M3J3kRfbqXWjm5_EzlqWAsKQmibqOGQlTcB_Vxs">
> As for `GoogleDisplayVideo360SDFtoGCSOperator`, I'm not sure an individual account can run the system tests, as they likely require DV360 enterprise access.
Thanks a lot, and good point!
@VladaZakharova @MaksYermak What should we do regarding the `GoogleDisplayVideo360SDFtoGCSOperator`? Are you able to test it on your premises?
@Ajay9704 Until we figure out the above, could you please split the `GoogleDisplayVideo360SDFtoGCSOperator` from this PR into a separate one?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GoogleCalendarToGCSOperator: add unwrap_single flag and return full GCS URIs [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 merged PR #61284:
URL: https://github.com/apache/airflow/pull/61284
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar and Display Video operators [airflow]
Posted by "yuseok89 (via GitHub)" <gi...@apache.org>.
yuseok89 commented on PR #61284:
URL: https://github.com/apache/airflow/pull/61284#issuecomment-3871576038
@shahar1
I ran the system tests for `GoogleCalendarToGCSOperator`.
## Test Screenshots
### unrwap = True
<img width="1044" height="162" alt="image" src="https://github.com/user-attachments/assets/21640340-a1d5-4f4a-a03d-5e2ab260e58e" />
### unrwap = False
<img width="1044" height="177" alt="image" src="https://github.com/user-attachments/assets/cbe0a3cf-3a14-4ac0-a913-87157b69cd7d" />
---
As for `GoogleDisplayVideo360SDFtoGCSOperator`, I'm not sure an individual account can run the system tests, as they likely require DV360 enterprise access.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar and Display Video operators [airflow]
Posted by "Ajay9704 (via GitHub)" <gi...@apache.org>.
Ajay9704 commented on PR #61284:
URL: https://github.com/apache/airflow/pull/61284#issuecomment-3828707887
> @Ajay9704 Could you please run `prek run --all-files`, and fix manually where needed? (it seems that you just did)
>
> One last thing (half related) - it seems that some of the tests are written using Python's native unittest module, instead of pytest. I'll carefully assume that it is due to the usage of AI, which is not forbidden - but it should be supervised and declared explicitly in the PR's description. Please refer to the related [docs](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions) and note for your next PRs.
Sorry sir, while resolving I got stuck at one point, so I took help from AI. I will remember this and won’t do it again, and even if I take a small help next time, I will mention it in the PR
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar and Display Video operators [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on PR #61284:
URL: https://github.com/apache/airflow/pull/61284#issuecomment-3828957648
> @shahar1 i am unable to understand what went wrong can you please help me with this
>
> > [Tests / Integration and System Tests / Integration: providers celery (pull_request)] (https://github.com/apache/airflow/actions/runs/21547359138/job/62091006890?pr=61284)Failing after 8m
Sure! I tried to rerun it - it doesn't seem related to this PR specifically, so it's either a flaky integration test or something else that should be solved by merging from main.
Please leave this PR as-is for now and feel free to work on other issues, I'll take care of merging it once it's solved.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar and Display Video operators [airflow]
Posted by "Ajay9704 (via GitHub)" <gi...@apache.org>.
Ajay9704 commented on PR #61284:
URL: https://github.com/apache/airflow/pull/61284#issuecomment-3828936957
@shahar1 i am unable to understand what went wrong can you please help me with this
>[Tests / Integration and System Tests / Integration: providers celery (pull_request)] (https://github.com/apache/airflow/actions/runs/21547359138/job/62091006890?pr=61284)Failing after 8m
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar [airflow]
Posted by "Ajay9704 (via GitHub)" <gi...@apache.org>.
Ajay9704 commented on code in PR #61284:
URL: https://github.com/apache/airflow/pull/61284#discussion_r2788420514
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
@@ -159,9 +174,9 @@ def _upload_data(
object_name=dest_file_name,
filename=temp_file.name,
)
- return dest_file_name
+ return f"gs://{self.destination_bucket}/{dest_file_name}"
Review Comment:
> I went over the PR again and noticed that changing the returned value from `dest_file_name` to the full URI could be a breaking change, so we should deprecate it slowly (similar to the `unwrap_single=False`). Add another flag `return_gcs_uri` with `False` as default. Only if `unwrap_single` is also `False`, it should return `dest_file_name`. `return_gcs_uri` cannot be set to `True` altogether with `unwrap_single=True` (it should raise `ValueError`, as in the future we plan to align all operators with returning list of URIs. Please handle these case, including unit tests.
@shahar1 All requested changes have been implemented and pushed.
Please review when you have time
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar [airflow]
Posted by "Ajay9704 (via GitHub)" <gi...@apache.org>.
Ajay9704 commented on PR #61284:
URL: https://github.com/apache/airflow/pull/61284#issuecomment-3883025474
> Very last small fix and we're good to go :)
The requested fix has been applied, and all checks are now successful. Please review when convenient.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar and Display Video operators [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #61284:
URL: https://github.com/apache/airflow/pull/61284#discussion_r2749377017
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
@@ -186,6 +189,9 @@ def execute(self, context):
time_zone=self.time_zone,
updated_min=self.updated_min,
)
- gcs_path_to_file = self._upload_data(events)
-
- return gcs_path_to_file
+ gcs_uri = self._upload_data(events)
+ result = [gcs_uri]
+
+ if self.unwrap_single:
+ return gcs_uri
+ return result
Review Comment:
For a slightly better consistency:
```suggestion
if self.unwrap_single:
return result[0]
return result
```
##########
providers/google/tests/unit/google/cloud/transfers/test_calendar_to_gcs.py:
##########
@@ -124,3 +163,53 @@ def test_execute(self, mock_upload_data, mock_calendar_hook):
)
mock_upload_data.assert_called_once_with(data)
+
+ @mock.patch("airflow.providers.google.cloud.transfers.calendar_to_gcs.GoogleCalendarHook")
+ @mock.patch(
+ "airflow.providers.google.cloud.transfers.calendar_to_gcs.GoogleCalendarToGCSOperator._upload_data"
+ )
+ def test_execute_with_unwrap_single_false(self, mock_upload_data, mock_calendar_hook):
+ context = {}
+ data = [EVENT]
+ mock_upload_data.side_effect = [f"gs://{BUCKET}/{PATH}"]
+ mock_calendar_hook.return_value.get_events.return_value = data
+
+ op = GoogleCalendarToGCSOperator(
+ api_version=API_VERSION,
+ task_id="test_task",
+ calendar_id=CALENDAR_ID,
+ destination_bucket=BUCKET,
+ destination_path=PATH,
+ gcp_conn_id=GCP_CONN_ID,
+ impersonation_chain=IMPERSONATION_CHAIN,
+ unwrap_single=False,
+ )
+ result = op.execute(context)
+
+ mock_calendar_hook.assert_called_once_with(
+ api_version=API_VERSION,
+ gcp_conn_id=GCP_CONN_ID,
+ impersonation_chain=IMPERSONATION_CHAIN,
+ )
+
+ mock_calendar_hook.return_value.get_events.assert_called_once_with(
+ calendar_id=CALENDAR_ID,
+ i_cal_uid=None,
+ max_attendees=None,
+ max_results=None,
+ order_by=None,
+ private_extended_property=None,
+ q=None,
+ shared_extended_property=None,
+ show_deleted=None,
+ show_hidden_invitation=None,
+ single_events=None,
+ sync_token=None,
+ time_max=None,
+ time_min=None,
+ time_zone=None,
+ updated_min=None,
+ )
+
+ mock_upload_data.assert_called_once_with(data)
+ assert result == [f"gs://{BUCKET}/{PATH}"]
Review Comment:
Add a new line
##########
providers/google/src/airflow/providers/google/marketing_platform/operators/display_video.py:
##########
@@ -133,6 +133,7 @@ class GoogleDisplayVideo360SDFtoGCSOperator(BaseOperator):
"bucket_name",
"object_name",
"impersonation_chain",
+ "unwrap_single",
Review Comment:
Same comments as before regarding removal from templated fields + docstring
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
@@ -82,6 +82,7 @@ class GoogleCalendarToGCSOperator(BaseOperator):
"destination_bucket",
"destination_path",
"impersonation_chain",
+ "unwrap_single",
Review Comment:
I prefer not to add `unwrap_single` to the `template_fields` at the moment, as I don't see a clear use case for that + we might decide to eventually deprecate the flag in the future after making behavior consistent.
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
Review Comment:
Please add a docstring for the `unwrap_single ` flag
##########
providers/google/tests/unit/google/marketing_platform/operators/test_display_video.py:
##########
@@ -144,4 +214,4 @@ def test_execute(self, mock_hook):
mock_hook.return_value.create_sdf_download_operation.assert_called_once_with(
body_request=body_request
)
- mock_context["task_instance"].xcom_push.assert_called_once_with(key="name", value=test_name)
+ mock_context["task_instance"].xcom_push.assert_called_once_with(key="name", value=test_name)
Review Comment:
Add a new line
##########
providers/google/src/airflow/providers/google/marketing_platform/operators/display_video.py:
##########
@@ -155,8 +157,9 @@ def __init__(
self.api_version = api_version
self.gcp_conn_id = gcp_conn_id
self.impersonation_chain = impersonation_chain
+ self.unwrap_single = unwrap_single
- def execute(self, context: Context) -> str:
+ def execute(self, context: Context) -> Union[str, list[str]]:
Review Comment:
```suggestion
def execute(self, context: Context) -> str | list[str]:
```
##########
providers/google/tests/unit/google/marketing_platform/operators/test_display_video.py:
##########
@@ -145,3 +215,5 @@ def test_execute(self, mock_hook):
body_request=body_request
)
mock_context["task_instance"].xcom_push.assert_called_once_with(key="name", value=test_name)
+
+
Review Comment:
Remove one empty line
##########
providers/google/src/airflow/providers/google/marketing_platform/operators/display_video.py:
##########
@@ -194,4 +197,8 @@ def execute(self, context: Context) -> str:
filename=os.path.join(tmp_dir, fname),
gzip=False,
)
- return f"{self.bucket_name}/{self.object_name}"
+ result = [f"gs://{self.bucket_name}/{self.object_name}"]
+
+ if self.unwrap_single:
+ return f"gs://{self.bucket_name}/{self.object_name}"
+ return result
Review Comment:
For a slightly better consistency:
```suggestion
result = [f"gs://{self.bucket_name}/{self.object_name}"]
if self.unwrap_single:
return result[0]
return result
```
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
@@ -108,6 +109,7 @@ def __init__(
destination_path: str | None = None,
gcp_conn_id: str = "google_cloud_default",
impersonation_chain: str | Sequence[str] | None = None,
+ unwrap_single: bool = True,
Review Comment:
In the future we plan to change the default to `False`, which will be a breaking change if you don't set it up explicitly.
For that reason, please let the value also accept `None` as well, and if it is `None` (meaning that it is unset) - set `unwrap_single` it to `True` + raise a `FutureWarning` about the expected change.
If it is explicitly set (either `True` or `False`), there's no need to raise this warning.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar and Display Video operators [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #61284:
URL: https://github.com/apache/airflow/pull/61284#discussion_r2749544554
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
@@ -19,7 +19,7 @@
import json
from collections.abc import Sequence
from tempfile import NamedTemporaryFile
-from typing import TYPE_CHECKING, Any
+from typing import TYPE_CHECKING, Any, Union
Review Comment:
```suggestion
from typing import TYPE_CHECKING, Any
```
##########
airflow-core/docs/administration-and-deployment/plugins.rst:
##########
@@ -351,3 +351,4 @@ For example, to print all routes, run:
.. code-block:: bash
FLASK_APP=airflow.www.app:create_app flask routes
+
Review Comment:
I assume it was added by mistake - please revert this change as it is unrelated to this PR
##########
providers/google/src/airflow/providers/google/cloud/transfers/calendar_to_gcs.py:
##########
@@ -159,9 +173,9 @@ def _upload_data(
object_name=dest_file_name,
filename=temp_file.name,
)
- return dest_file_name
+ return f"gs://{self.destination_bucket}/{dest_file_name}"
- def execute(self, context):
+ def execute(self, context) -> Union[str, list[str]]:
Review Comment:
```suggestion
def execute(self, context) -> str | list[str]:
```
##########
providers/google/src/airflow/providers/google/marketing_platform/operators/display_video.py:
##########
@@ -23,7 +23,7 @@
import tempfile
import zipfile
from collections.abc import Sequence
-from typing import TYPE_CHECKING, Any
+from typing import TYPE_CHECKING, Any, Union
Review Comment:
```suggestion
from typing import TYPE_CHECKING, Any
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return full GCS URIs from Google Calendar and Display Video operators [airflow]
Posted by "Ajay9704 (via GitHub)" <gi...@apache.org>.
Ajay9704 commented on PR #61284:
URL: https://github.com/apache/airflow/pull/61284#issuecomment-3828982653
> > @shahar1 i am unable to understand what went wrong can you please help me with this
> > > [Tests / Integration and System Tests / Integration: providers celery (pull_request)] (https://github.com/apache/airflow/actions/runs/21547359138/job/62091006890?pr=61284)Failing after 8m
>
> Sure! I tried to rerun it - it doesn't seem related to this PR specifically, so it's either a flaky integration test or something else that should be solved by merging from main. Please leave this PR as-is for now and feel free to work on other issues, I'll take care of merging it once it's solved.
Sure sir, and thank you for helping throughout
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org