Skip to content

Switch Google Ads API version from v13 to v14 #32028

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
potiuk merged 1 commit into from
Jun 21, 2023
Merged

Switch Google Ads API version from v13 to v14 #32028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions airflow/providers/google/ads/hooks/ads.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@

from google.ads.googleads.client import GoogleAdsClient
from google.ads.googleads.errors import GoogleAdsException
from google.ads.googleads.v13.services.services.customer_service import CustomerServiceClient
from google.ads.googleads.v13.services.services.google_ads_service import GoogleAdsServiceClient
from google.ads.googleads.v13.services.types.google_ads_service import GoogleAdsRow
from google.ads.googleads.v14.services.services.customer_service import CustomerServiceClient
from google.ads.googleads.v14.services.services.google_ads_service import GoogleAdsServiceClient
from google.ads.googleads.v14.services.types.google_ads_service import GoogleAdsRow
from google.api_core.page_iterator import GRPCIterator
from google.auth.exceptions import GoogleAuthError

Expand Down Expand Up @@ -73,7 +73,7 @@ class GoogleAdsHook(BaseHook):
:return: list of Google Ads Row object(s)
"""

default_api_version = "v13"
default_api_version = "v14"
Copy link
Contributor

@eladkal eladkal Jun 20, 2023

Choose a reason for hiding this comment

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

This is a breaking change. does it not?

Copy link
Contributor Author

@ahidalgob ahidalgob Jun 20, 2023

Choose a reason for hiding this comment

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

This version change doesn't break anything in the methods we use in operators.

Copy link
Contributor

@eladkal eladkal Jun 20, 2023

Choose a reason for hiding this comment

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

I don't know GoogleAds but this is what I have in mind:

Users who use the hook may be relaying on the default value.

GoogleAdsHook(..).search(...)

Before this change it will use v13 after this change it will use v14.
Thus this is a breaking change.

Am I missing something?

Copy link
Contributor Author

@ahidalgob ahidalgob Jun 20, 2023

Choose a reason for hiding this comment

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

Hey, you are right but in this case we rely on only two methods from Google Ads API:

  • GoogleAdsServiceClient.search used by Hook.search and Hook.search_proto_plus

  • CustomerServiceClient.list_accessible_customers used in Hook.list_accessible_customers

And in this case behaviour is unchanged from v13 to v14 for those methods. search method wasn't changed and list_accessible_customers only added one output-only field to Customer (so not breaking), but this is not even visible from the user's side as we only return another field, not the whole Customer object.

Copy link
Member

@potiuk potiuk Jun 20, 2023

Choose a reason for hiding this comment

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

Yeah. Totally agree with @ahidalgob -> there are many libraries that have braking changes on their own but they are not breaking the API we are exposing to our users. Surely the users could use some stuff from the google ads client, but technically speaking this is completely outside of our control or influence.

We cannot break something that we are unaware of. If we take it to extreme, then every single change in every single dependency or line of code would be a breaking change (the usual reference to https://www.hyrumslaw.com/.

We will always break something with any change, this is clear, so it really depends on us what we see as "enough breaking" to make it a "Major" bump. What would qualify as a breaking change is changing a promise "our" code malks when publishing what user is supposed to use (i.e. Hooks, Operators, etc. etc. ). We cannot be responsible for "other's" code - as long as other code does not impact "our" code, the change is not really breaking.

Similarly to https://airflow.apache.org/docs/apache-airflow/stable/public-airflow-interface.html - we have https://airflow.apache.org/docs/apache-airflow-providers-google/stable/_api/airflow/providers/google/index.html# - and we do not break any of the API's there.

This one is technically equivalent to a chaning version of external dependency - which we previously agreed to that it is not a breaking chnage on its own.


def __init__(
self,
Expand Down
2 changes: 1 addition & 1 deletion airflow/providers/google/provider.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ dependencies:
- gcloud-aio-auth>=4.0.0,<5.0.0
- gcloud-aio-bigquery>=6.1.2
- gcloud-aio-storage
- google-ads>=20.0.0
- google-ads>=21.2.0
- google-api-core>=2.11.0
- google-api-python-client>=1.6.0
- google-auth>=1.0.0
Expand Down
2 changes: 1 addition & 1 deletion generated/provider_dependencies.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@
"gcloud-aio-auth>=4.0.0,<5.0.0",
"gcloud-aio-bigquery>=6.1.2",
"gcloud-aio-storage",
"google-ads>=20.0.0",
"google-ads>=21.2.0",
"google-api-core>=2.11.0",
"google-api-python-client>=1.6.0",
"google-auth-httplib2>=0.0.1",
Expand Down