Skip to content

Comments

GoogleCalendarToGCSOperator: add unwrap_single flag and return full GCS URIs#61284

Merged
shahar1 merged 20 commits intoapache:mainfrom
Ajay9704:GCS_URIs_#11323
Feb 11, 2026
Merged

GoogleCalendarToGCSOperator: add unwrap_single flag and return full GCS URIs#61284
shahar1 merged 20 commits intoapache:mainfrom
Ajay9704:GCS_URIs_#11323

Conversation

@Ajay9704
Copy link
Contributor

@Ajay9704 Ajay9704 commented Jan 31, 2026

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

Used AI for resolving conflicts

@Ajay9704 Ajay9704 requested a review from shahar1 as a code owner January 31, 2026 10:09
@boring-cyborg boring-cyborg bot added area:providers provider:google Google (including GCP) related issues labels Jan 31, 2026
Copy link
Contributor

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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).

@Ajay9704
Copy link
Contributor Author

  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

Copy link
Contributor

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better, a couple of comments to resolve before we're ready to go

Copy link
Contributor

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please handle these last changes and I'll aprove

@shahar1 shahar1 self-requested a review January 31, 2026 13:07
Copy link
Contributor

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work Ajay!

cc: @VladaZakharova @MaksYermak

@shahar1
Copy link
Contributor

shahar1 commented Jan 31, 2026

@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 and note for your next PRs.

@Ajay9704
Copy link
Contributor Author

Ajay9704 commented Jan 31, 2026

@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 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

I have updated PR description accordingly

@Ajay9704
Copy link
Contributor Author

@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

@shahar1
Copy link
Contributor

shahar1 commented Jan 31, 2026

@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.

@Ajay9704
Copy link
Contributor Author

@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

@shahar1
Copy link
Contributor

shahar1 commented Jan 31, 2026

@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.

Abhishekmishra2808 added a commit to Abhishekmishra2808/airflow that referenced this pull request Feb 1, 2026
…erator

- Add unwrap_single keyword-only parameter to __init__ (default=False)
- Update execute() return type to str | list[str]
- Implement conditional return logic:
  - Returns str when unwrap_single=True
  - Returns list[str] by default (backward compatible)
- Add comprehensive docstring updates
- Add test_execute_with_unwrap_single() to validate new behavior
- Maintain backward compatibility with existing code

Follows the pattern used in PR apache#61284
@shahar1
Copy link
Contributor

shahar1 commented Feb 6, 2026

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).

shahar1 pushed a commit to Abhishekmishra2808/airflow that referenced this pull request Feb 6, 2026
…erator

- Add unwrap_single keyword-only parameter to __init__ (default=False)
- Update execute() return type to str | list[str]
- Implement conditional return logic:
  - Returns str when unwrap_single=True
  - Returns list[str] by default (backward compatible)
- Add comprehensive docstring updates
- Add test_execute_with_unwrap_single() to validate new behavior
- Maintain backward compatibility with existing code

Follows the pattern used in PR apache#61284
shahar1 pushed a commit to Abhishekmishra2808/airflow that referenced this pull request Feb 7, 2026
…erator

- Add unwrap_single keyword-only parameter to __init__ (default=False)
- Update execute() return type to str | list[str]
- Implement conditional return logic:
  - Returns str when unwrap_single=True
  - Returns list[str] by default (backward compatible)
- Add comprehensive docstring updates
- Add test_execute_with_unwrap_single() to validate new behavior
- Maintain backward compatibility with existing code

Follows the pattern used in PR apache#61284
@yuseok89
Copy link
Contributor

yuseok89 commented Feb 9, 2026

@shahar1
I ran the system tests for GoogleCalendarToGCSOperator.

Test Screenshots

unrwap_single = True

image

unrwap_single = False

image

As for GoogleDisplayVideo360SDFtoGCSOperator, I'm not sure an individual account can run the system tests, as they likely require DV360 enterprise access.

@shahar1
Copy link
Contributor

shahar1 commented Feb 10, 2026

@shahar1 I ran the system tests for GoogleCalendarToGCSOperator.

Test Screenshots

unrwap_single = True

image ### unrwap_single = False image 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?

@Ajay9704 Ajay9704 changed the title Return full GCS URIs from Google Calendar and Display Video operators Return full GCS URIs from Google Calendar Feb 10, 2026
Copy link
Contributor

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

  1. I've accidentally misled you in this comment - I meant that return_gcs_uri cannot be set to False altogether with unwrap_single=False (I flipped it the other way around when writing it).
  2. Please add a deprecation warning to return_gcs_uri parameter as well. Be careful - when checking the condition of both flags, they should be bool and not None (otherwise you might compare with None that has a falsy beahvior).
  3. Adjusts tests according to the above.

Afterwards I think that we're good to go.

Copy link
Contributor

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very last small fix and we're good to go :)

@Ajay9704
Copy link
Contributor Author

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.

@shahar1 shahar1 changed the title Return full GCS URIs from Google Calendar GoogleCalendarToGCSOperator: add unwrap_single flag and return full GCS URIs Feb 11, 2026
@shahar1 shahar1 merged commit 4ca1da7 into apache:main Feb 11, 2026
90 checks passed
Ratasa143 pushed a commit to Ratasa143/airflow that referenced this pull request Feb 15, 2026
choo121600 pushed a commit to choo121600/airflow that referenced this pull request Feb 22, 2026
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