-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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.
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
🐛 Source Google-Ads: add "login-customer-id" setting #5159
🐛 Source Google-Ads: add "login-customer-id" setting #5159
Conversation
/test connector=connectors/source-google-ads
|
@@ -46,6 +46,8 @@ class GoogleAds: | |||
DEFAULT_PAGE_SIZE = 1000 | |||
|
|||
def __init__(self, credentials: Mapping[str, Any], customer_id: str): | |||
if "login_customer_id" in credentials and not credentials["login_customer_id"].strip(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if login_customer_id filed is empty then remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
return True, None | ||
except Exception as error: | ||
return False, f"Unable to connect to Google Ads API with the provided credentials - {repr(error)}" | ||
except GoogleAdsException as error: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove try/except block, any exception would be catch on AbstractSource check method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but the main goal of the try/except is the correct formulation of the error message.
@@ -51,6 +51,7 @@ Google Ads Account with an approved Developer Token \(note: In order to get API | |||
* refresh_token | |||
* start_date | |||
* customer_id | |||
* login_customer_id (more information about this field you can find in [Google Ads docs](https://developers.google.com/google-ads/api/docs/concepts/call-structure#cid)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* login_customer_id (more information about this field you can find in [Google Ads docs](https://developers.google.com/google-ads/api/docs/concepts/call-structure#cid)) | |
* login_customer_id (you can find more information about this field in [Google Ads docs](https://developers.google.com/google-ads/api/docs/concepts/call-structure#cid)) |
@@ -46,6 +46,8 @@ class GoogleAds: | |||
DEFAULT_PAGE_SIZE = 1000 | |||
|
|||
def __init__(self, credentials: Mapping[str, Any], customer_id: str): | |||
if "login_customer_id" in credentials and not credentials["login_customer_id"].strip(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if "login_customer_id" in credentials and not credentials["login_customer_id"].strip(): | |
# https://developers.google.com/google-ads/api/docs/concepts/call-structure#cid | |
if "login_customer_id" in credentials and not credentials["login_customer_id"].strip(): |
@@ -39,13 +39,18 @@ | |||
"title": "Refresh Token", | |||
"description": "Refresh token generated using developer_token, oauth_client_id, and oauth_client_secret. More instruction on how to find this value in our <a href=\"https://docs.airbyte.io/integrations/sources/google-adwords#setup-guide\">docs</a>", | |||
"airbyte_secret": true | |||
}, | |||
"login_customer_id": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be a separate field outside of credentials
since that is an autogenerated json which will likely not contain this information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
/test connector=connectors/source-google-ads
|
/publish connector=connectors/source-google-ads
|
What
closes #5150.
How
Describe the solution
Recommended reading order
x.java
y.python
Pre-merge Checklist
Expand the checklist which is relevant for this PR.
Connector checklist
airbyte_secret
in the connector's spec./gradlew :airbyte-integrations:connectors:<name>:integrationTest
./test connector=connectors/<name>
command as documented here is passing.README.md
docs/SUMMARY.md
if it's a new connectordocs/integrations/<source or destination>/<name>
.docs/integrations/...
. See changelog exampledocs/integrations/README.md
contains a reference to the new connector/publish
command described hereConnector Generator checklist
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changes