-
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
fix(source-amazon-ads): set is_resumable only for Fullrefresh streams #48343
base: master
Are you sure you want to change the base?
Conversation
[skip ci] Signed-off-by: Artem Inzhyyants <[email protected]>
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
UPD: |
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.
Should we add mock server tests to validate the behavior change?
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
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! Let's me this. Should we validate a couple of connection in prod once this is release to validate that this behave as expected not only for report streams but also for portfolios (or another non resumable stream)?
my main concern was only about incremental streams. The only incremental streams we have in this connector is *-reports, so I think its safe to merge this. I'll run regression test to check this. |
What
passing is_resumable=False for report stream disabled incremental option
How
set is_resumable only for Fullrefresh streams
Review guide
airbyte-integrations/connectors/source-amazon-ads/source_amazon_ads/streams/common.py
User Impact
Can this PR be safely reverted and rolled back?