Return list of destination URIs from HttpToGCSOperator

[Srabasti]

Changes per #11323 for http_to_gcs_operator

Was generative AI tooling used to co-author this PR?

Yes for tests - ChatGPT

[uranusjr]

Why do we need to do deprecation? From what I can tell, the operator does not return a value at all before the change, so we don’t need to consider compatibility? We can just add the argument and set the default to True.

[shahar1]

+1 one to the comment made by @uranusjr, and I'll take it a bit further - I'd probably avoid adding the unwrap_single parameter at all - as after aligning behavior of all relevant operators at some point in the future, we could think of deprecating the unwrap_single and return only list[str] (accessing a single value would be consistent and simple as lst[0]). To make such alignment easier, it would be the best to avoid this flag at all if it is not required for backward-compatbility.

[Srabasti]

Thanks for your review comments @uranusjr and @shahar1 !
Had made changes, assuming both True and False cases should be handled for unwrap_single field, for current state, to ensure failsafe for future. Since we are planning to remove it in long run, does not make sense to do that anymore.

Since am not aware of the past context, I was curious to learn the reason behind "unwrap_single" name has been chosen for this interim parameter name.

[shahar1]

Thanks for your review comments @uranusjr and @shahar1 ! Had made changes, assuming both True and False cases should be handled for unwrap_single field, for current state, to ensure failsafe for future. Since we are planning to remove it in long run, does not make sense to do that anymore.

Since am not aware of the past context, I was curious to learn the reason behind "unwrap_single" name has been chosen for this interim parameter name.

If you've made any changes, you might have forgotten to push them :)

Basically it was my idea for the naming - I took inspiration from KubernetesPodOperator that has the same parameter. As the name hints, it "unwraps" lists of single entities (e.g, ['gs://bucket/file']) into a string ('gs://bucket/file'). It's good for backward-compatibility for the cases that already return the single entity as default, but I think that for the long term it's better that all operators will return list[str], and the user will take care of the unwrapping if necessary (i.e., lst[0] for single entities).

In the case of HttpToGCSOperator, it doesn't return anything at the moment - so it's just a matter of return [f"gs://{self.bucket_name}/{self.object_name}"], testing this behavior, and that's all. No real need for unwrap_single.

[Srabasti]

Thanks for the clarification @shahar1 as this helps me understand better the thought process behind choosing variable names!

Apologies for delay in response as it has been busy with items related to Airflow:)

[potiuk]

Needs to update some tests though

[Srabasti]

Yes am working on the tests..

[Srabasti]

Breeze testing before changes implemented for test_http_to_gcs.py

Breeze testing after changes implemented for test_http_to_gcs.py

Used AI for generating tests.

[shahar1]

Excellent job Srabasti!
Before merging we need to run the system tests for this operator to ensure that nothing breaks.
You may try doing it on your own, or wait until someone else does - if necessary, I will help with it during this week before cutting the release the new Google provider version, so this feature will be included.

@yuseok89 - maybe you could lend a hand? :)

CC: @VladaZakharova @MaksYermak

[yuseok89]

@shahar1
I ran the system test for HttpToGCSOperator.

Test Screenshot

[potiuk]

Nice!