-
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 Adwords: support selecting the lookback window for determining conversions #2918
Conversation
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.
@harshithmullapudi Thanks for the contribution!! we will also need to pass this input to the singer tap in source.py
. Would you like to make that change?
@@ -10,7 +10,8 @@ | |||
"oauth_client_secret", | |||
"refresh_token", | |||
"start_date", | |||
"customer_ids" | |||
"customer_ids", | |||
"conversion_window_days" |
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 not be a required parameter for backwards compatibility
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.
Backward compatibility apart, should it be required?
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.
nope. It should be optional. It basically decides the lookback window for getting reports. By default this is 30 days, but it should be configurable by the user
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.
Yeah I will remove from required
...-integrations/connectors/source-google-adwords-singer/source_google_adwords_singer/spec.json
Show resolved
Hide resolved
Yeah will work on that |
I think this is already taken care. As this comes from config.json. I did test this. |
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.
@harshithmullapudi looks great, and you are right about source.py
-- my mistake for the misleading comment. Thanks!
@harshithmullapudi FYI this is blocked on #2929 -- we're working on that this week and you should be able to access the new connector version once that issue is blocked. |
Thank you @harshithmullapudi !!! |
What
Describe what the change is solving
It helps the user to define the historical replication start date
How
Describe the solution
As the singer already supports it adding it in the schema will let airbyte api configure it