Update Google Ads hook

[jacobhjkim]

1. Add parameter api_version to both GoogleAdsToGcsOperator & GoogleAdsListAccountsOperator.
2. Remove repeated line self.gcp_conn_id = gcp_conn_id.
3. Change the default api_version to v5 since v3 is deprecated.
4. Add api_version to GoogleAdsHook's docstring.

---

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

[jhtimmins]

@jacobhjkim Is this a breaking change for anyone using v3 (or v4, if it exists)?

[github-actions]

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

[jacobhjkim]

@jacobhjkim Is this a breaking change for anyone using v3 (or v4, if it exists)?

The current Airflow release's google Ads SDK will return a deprecation error (not warning) when you use api_version=v3. So I don't think this change will be a breaking change. People who are using v4 won't get affected since they are already specifying v4.

[dstandish]

what might be better is to make this Optional[str] = None (for all the operators)

then only supply api_version to hook if provided

this way, when we need to increment the default version again, we only need to do it in one place; the operators' default will always be whatever the hook default is

[turbaszek]

I agree with @dstandish. Using optional and hook's default value as fallback would be best - we will have only one place to change in future.

[jacobhjkim]

Somehow missed the notification. I agree, this way is more elegant.

[jacobhjkim]

Since passing in None is not the same as not passing in any parameter, I will have to change GoogleAdsHook to something like this. Does this look ok?

    def __init__(
        self,
        api_version: str,
        gcp_conn_id: str = "google_cloud_default",
        google_ads_conn_id: str = "google_ads_default",
    ) -> None:
        super().__init__()
        self.api_version = api_version if api_version else "v5"
        ...

This doesn't feel Pythonic, but it does allow us to only change one place in the future.

I wanted to see how other provider hooks deal with api_version

1. ComputeEngineHook takes api_version as an argument, but ComputeEngineSSHHook which calls ComputeEngineHook doesn't pass in api_version.
2. CloudSQLHook takes api_version but has no default value. Instead CloudSQLCreateInstanceOperator has a default value assigned.

It seems like there is no set method for this specific issue.

[dstandish]

i see, thinking about how to handle case if operator calls hook with version None

but i would make it so the operator doesn't pass that param to the hook unless it's not None. then in that case it uses hook default.

then in the hook you can do it in the normal way

    def __init__(
        self,
        api_version: str='v5',
        gcp_conn_id: str = "google_cloud_default",
        google_ads_conn_id: str = "google_ads_default",
    ) -> None:
        super().__init__()
        self.api_version = api_version
        ...

if for some reason it is not possible to do this with the operators (unlikely) i would do this:

    def __init__(
        self,
        api_version: Optional[str]=None,
        gcp_conn_id: str = "google_cloud_default",
        google_ads_conn_id: str = "google_ads_default",
    ) -> None:
        super().__init__()
        self.api_version = api_version or 'v5'
        ...

true it's a bit hidden, and that's why first suggestion is better, but it's still better than setting it in many places

if you wanted to make it more brightly visible, you could consider defining a constant in the module DEFAULT_API_VERSION and replace 'v5' with that

so,

    def __init__(
        self,
        api_version: Optional[str]=None,
        gcp_conn_id: str = "google_cloud_default",
        google_ads_conn_id: str = "google_ads_default",
    ) -> None:
        super().__init__()
        self.api_version = api_version or DEFAULT_API_VERSION
        ...

Lastly, with some clients, not supplying api version means will use latest. and if that were to be true here, i'd suggest doing that, so we don't need any version identifier in the code. But i think it may not work that way with gcp.

[jacobhjkim]

Sorry for keep asking questions about a seemingly simple issue. However, when you say

I would make it so the operator doesn't pass that param to the hook unless it's not None.

do you mean something like this?

        if self.api_version:
            ads_hook = GoogleAdsHook(
                gcp_conn_id=self.gcp_conn_id,
                google_ads_conn_id=self.google_ads_conn_id,
                api_version=self.api_version,
            )
        else:
            ads_hook = GoogleAdsHook(
                gcp_conn_id=self.gcp_conn_id,
                google_ads_conn_id=self.google_ads_conn_id,
            )

[dstandish]

no worries at all

so i think the standard appaoach is something like this:

            kwargs = {}
            if self.api_version:
                kwargs.update(api_version=self.api_version)
            ads_hook = GoogleAdsHook(
                gcp_conn_id=self.gcp_conn_id,
                google_ads_conn_id=self.google_ads_conn_id,
                **kwargs,
            )

[turbaszek]

@dstandish should we merge this PR? The version issue I think can be addressed in a followup, WDYT?

[jacobhjkim]

Sorry, I've been busy with my work lately, I will push an update first thing tomorrow. I don't think this PR needs a follow up.

[turbaszek]

Wouldn't it be simpler to do:

class GoogleAdsHook:
  default_version = "v5"
  
  def __init__(self, version):
    self.version = version or self.default_version

In that way we can simply pass None values in operators. WDYT?

[jacobhjkim]

@dstandish Suggested that way on this review comment.

and that's why first suggestion is better,

#15266 (comment)

What do you think @dstandish?

[dstandish]

i think either way is ok but i like @turbaszek's suggestion the best

it's all tradeoffs, but i think @turbaszek approach minimizes compleixity and noise overall, because as he points out then you don't need the kwargs logic.

[dstandish]

i think either way is ok but i like @turbaszek's suggestion the best

it's all tradeoffs, but i think @turbaszek approach minimizes compleixity and noise overall, because as he points out then you don't need the kwargs logic.

[dstandish]

approving pending fix

[turbaszek]

@jacobhjkim can you please rebase? I think there might have been an issue with some typo as multiple files failed on spell check

[jacobhjkim]

👍