You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/04/08 03:32:55 UTC
[GitHub] [airflow] jacobhjkim opened a new pull request #15266: Update Google Ads hook
jacobhjkim opened a new pull request #15266:
URL: https://github.com/apache/airflow/pull/15266
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
1. Remove repeated line `self.gcp_conn_id = gcp_conn_id`.
2. Change the default `api_version` to `v5` since `v3` is deprecated.
3. Add `api_version` to GoogleAdsHook's docstring.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r619697979
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
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
```python
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:
```python
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,
```python
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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jacobhjkim commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
jacobhjkim commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r619665345
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
Somehow missed the notification. I agree, this way is more elegant.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#issuecomment-818383483
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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jacobhjkim commented on pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
jacobhjkim commented on pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#issuecomment-845702674
👍
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jacobhjkim commented on pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
jacobhjkim commented on pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#issuecomment-818450741
> @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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#issuecomment-838924805
@dstandish should we merge this PR? The version issue I think can be addressed in a followup, WDYT?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jacobhjkim commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
jacobhjkim commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r631590371
##########
File path: airflow/providers/google/ads/operators/ads.py
##########
@@ -89,11 +92,19 @@ def __init__(
self.google_ads_conn_id = google_ads_conn_id
self.gzip = gzip
self.impersonation_chain = impersonation_chain
+ self.api_version = api_version
def execute(self, context: dict) -> str:
uri = f"gs://{self.bucket}/{self.object_name}"
+ 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)
+ ads_hook = GoogleAdsHook(
+ gcp_conn_id=self.gcp_conn_id,
+ google_ads_conn_id=self.google_ads_conn_id,
+ **kwargs,
+ )
Review comment:
@dstandish Suggested that way on this review comment. https://github.com/apache/airflow/pull/15266#discussion_r619697979
What do you think @dstandish?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r631825481
##########
File path: airflow/providers/google/ads/hooks/ads.py
##########
@@ -69,22 +69,25 @@ class GoogleAdsHook(BaseHook):
:type gcp_conn_id: str
:param google_ads_conn_id: The connection ID with the details of Google Ads config.yaml file.
:type google_ads_conn_id: str
+ :param api_version: The Google Ads API version to use.
+ :type api_version: str
:return: list of Google Ads Row object(s)
:rtype: list[GoogleAdsRow]
"""
+ default_version = "v5"
+
def __init__(
self,
+ api_version: str,
Review comment:
this needs to be optional
##########
File path: airflow/providers/google/ads/hooks/ads.py
##########
@@ -69,22 +69,25 @@ class GoogleAdsHook(BaseHook):
:type gcp_conn_id: str
:param google_ads_conn_id: The connection ID with the details of Google Ads config.yaml file.
:type google_ads_conn_id: str
+ :param api_version: The Google Ads API version to use.
+ :type api_version: str
:return: list of Google Ads Row object(s)
:rtype: list[GoogleAdsRow]
"""
+ default_version = "v5"
Review comment:
while you're at it, i might suggest `default_api_version`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jacobhjkim commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
jacobhjkim commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r619819693
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
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?
```python
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,
)
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r612954376
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
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 version again, we only need to do it in one place.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r619697979
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
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
```python
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:
```python
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
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.
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
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
```python
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:
```python
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,
```python
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.
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
no worries at all
so i think the standard appaoach is something like this:
```python
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,
)
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jacobhjkim commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
jacobhjkim commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r619671516
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
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?
```python
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](https://github.com/apache/airflow/blob/master/airflow/providers/google/cloud/hooks/compute.py#L52) takes `api_version` as an argument, but [ComputeEngineSSHHook](https://github.com/apache/airflow/blob/master/airflow/providers/google/cloud/hooks/compute_ssh.py#L144) which calls ComputeEngineHook doesn't pass in `api_version`.
2. [CloudSQLHook](https://github.com/apache/airflow/blob/master/airflow/providers/google/cloud/hooks/cloud_sql.py#L96) takes `api_version` but has no default value. Instead [CloudSQLCreateInstanceOperator](https://github.com/apache/airflow/blob/master/airflow/providers/google/cloud/operators/cloud_sql.py#L320) has a default value assigned.
It seems like there is no set method for this specific issue.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r612954376
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
what might be better is to make this `Optional[str] = None` (for all the operators)
then only supply to hook if provided
this way, when we need to increment the version again, we only need to do it in one place.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#issuecomment-843559672
@jacobhjkim can you please rebase? I think there might have been an issue with some typo as multiple files failed on spell check
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jacobhjkim commented on pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
jacobhjkim commented on pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#issuecomment-838928272
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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r619940764
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
no worries at all
so i think the standard appaoach is something like this:
```python
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,
)
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r631597040
##########
File path: airflow/providers/google/ads/operators/ads.py
##########
@@ -89,11 +92,19 @@ def __init__(
self.google_ads_conn_id = google_ads_conn_id
self.gzip = gzip
self.impersonation_chain = impersonation_chain
+ self.api_version = api_version
def execute(self, context: dict) -> str:
uri = f"gs://{self.bucket}/{self.object_name}"
+ 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)
+ ads_hook = GoogleAdsHook(
+ gcp_conn_id=self.gcp_conn_id,
+ google_ads_conn_id=self.google_ads_conn_id,
+ **kwargs,
+ )
Review comment:
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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r619604044
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jacobhjkim commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
jacobhjkim commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r631590371
##########
File path: airflow/providers/google/ads/operators/ads.py
##########
@@ -89,11 +92,19 @@ def __init__(
self.google_ads_conn_id = google_ads_conn_id
self.gzip = gzip
self.impersonation_chain = impersonation_chain
+ self.api_version = api_version
def execute(self, context: dict) -> str:
uri = f"gs://{self.bucket}/{self.object_name}"
+ 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)
+ ads_hook = GoogleAdsHook(
+ gcp_conn_id=self.gcp_conn_id,
+ google_ads_conn_id=self.google_ads_conn_id,
+ **kwargs,
+ )
Review comment:
@dstandish Suggested that way on this review comment.
> and that's why first suggestion is better,
https://github.com/apache/airflow/pull/15266#discussion_r619697979
What do you think @dstandish?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#issuecomment-818383219
@jacobhjkim Is this a breaking change for anyone using v3 (or v4, if it exists)?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jacobhjkim commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
jacobhjkim commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r619665345
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
Somehow missed the notification. I agree, this way is more elegant.
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
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?
```python
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](https://github.com/apache/airflow/blob/master/airflow/providers/google/cloud/hooks/compute.py#L52) takes `api_version` as an argument, but [ComputeEngineSSHHook](https://github.com/apache/airflow/blob/master/airflow/providers/google/cloud/hooks/compute_ssh.py#L144) which calls ComputeEngineHook doesn't pass in `api_version`.
2. [CloudSQLHook](https://github.com/apache/airflow/blob/master/airflow/providers/google/cloud/hooks/cloud_sql.py#L96) takes `api_version` but has no default value. Instead [CloudSQLCreateInstanceOperator](https://github.com/apache/airflow/blob/master/airflow/providers/google/cloud/operators/cloud_sql.py#L320) has a default value assigned.
It seems like there is no set method for this specific issue.
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
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?
```python
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,
)
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r619604044
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r631589657
##########
File path: airflow/providers/google/ads/operators/ads.py
##########
@@ -89,11 +92,19 @@ def __init__(
self.google_ads_conn_id = google_ads_conn_id
self.gzip = gzip
self.impersonation_chain = impersonation_chain
+ self.api_version = api_version
def execute(self, context: dict) -> str:
uri = f"gs://{self.bucket}/{self.object_name}"
+ 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)
+ ads_hook = GoogleAdsHook(
+ gcp_conn_id=self.gcp_conn_id,
+ google_ads_conn_id=self.google_ads_conn_id,
+ **kwargs,
+ )
Review comment:
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?
##########
File path: airflow/providers/google/ads/operators/ads.py
##########
@@ -89,11 +92,19 @@ def __init__(
self.google_ads_conn_id = google_ads_conn_id
self.gzip = gzip
self.impersonation_chain = impersonation_chain
+ self.api_version = api_version
def execute(self, context: dict) -> str:
uri = f"gs://{self.bucket}/{self.object_name}"
+ 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)
+ ads_hook = GoogleAdsHook(
+ gcp_conn_id=self.gcp_conn_id,
+ google_ads_conn_id=self.google_ads_conn_id,
+ **kwargs,
+ )
Review comment:
Wouldn't it be simpler to do:
```py
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?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r612954376
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
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
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jacobhjkim edited a comment on pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
jacobhjkim edited a comment on pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#issuecomment-818450741
> @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`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r619697979
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
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
```python
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:
```python
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
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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek merged pull request #15266: Update Google Ads hook
Posted by GitBox <gi...@apache.org>.
turbaszek merged pull request #15266:
URL: https://github.com/apache/airflow/pull/15266
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org