-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feat(source-stripe): ref stream Payment Methods
#44891
feat(source-stripe): ref stream Payment Methods
#44891
Conversation
[no ci] Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
{ | ||
"type": "STREAM", | ||
"stream": { | ||
"stream_state": { "customer_updated": 10000000000 }, |
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 the state be per partition here? It seems like we are at risk of losing records if it's not
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 the state be per partition here?
I don't think so because ParentIncrementalStipeSubStream
class is used here. Thus, Customers
are being synced incrementally and we'll get payment_methods
only for the updated ones.
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.
Oh! So we have validated that when a payment method is updated, the client updated field is also updated. Can we document this somewhere?
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.
Also for the incremental part, shouldn't we rely on payment_method.*
events instead? https://docs.stripe.com/api/events/types#event_types-payment_method.attached
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 we did validate the assumption, can it be documented in the code so we have a reference? else we'll assume we're at risk of losing records
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.
So we have validated that when a payment method is updated, the client updated field is also updated.
Tested this, customer is NOT being updated on payment_method
update.
Also for the incremental part, shouldn't we rely on payment_method.* events instead?
Agreed, we should use this.
Moving PR to draft, need to change the implementation:
1st sync/Full refresh -- read from customers/{customer_id}/payment_methods
2nd sync/Incremental -- read from events stream with type
= payment_method.*
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
…ripe-customer-payment-methods
{ | ||
"type": "STREAM", | ||
"stream": { | ||
"stream_state": { "customer_updated": 10000000000 }, |
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 we did validate the assumption, can it be documented in the code so we have a reference? else we'll assume we're at risk of losing records
@@ -483,6 +483,18 @@ def streams(self, config: MutableMapping[str, Any]) -> List[Stream]: | |||
sub_items_attr="refunds", | |||
**args, | |||
), | |||
ParentIncrementalStipeSubStream( | |||
name="customer_payment_methods", |
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.
maybe a hot take, but I think we should just replace the existing payment_methods
stream. It's not useful as far as I could tell
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
…ripe-customer-payment-methods # Conflicts: # airbyte-integrations/connectors/source-stripe/metadata.yaml # airbyte-integrations/connectors/source-stripe/pyproject.toml # docs/integrations/sources/stripe.md
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
…ripe-customer-payment-methods
Customer Payment Methods
Payment Methods
…ripe-customer-payment-methods
@@ -43,6 +43,12 @@ data: | |||
scopedImpact: | |||
- scopeType: stream | |||
impactedScopes: ["refunds"] | |||
6.0.0: |
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.
does this need to be a major version bump? this feels like something we should notify users in the docs and migration guide, but I don't think it's worth stopping their syncs
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.
customer
was set as object, and now it is changed to string
.
This is the main reason to mark changes as breaking.
endpoint change can also affect customer incremental sync.
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.
shouldn't this be part of the migration guide then?
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.
🌶️ I'd still advocate for not flagging as breaking change since only one Cloud user actually has data for this stream
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.
ok, reverted.
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.
shouldn't this be part of the migration guide then?
Since i removed this version from breakingChanges
in metadata, CI fails on QA checks in migration guide.
Is there any other place I can place the release notes?
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
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.
I'm fine with the PR in the current state. If possible, I think we should add the payment method for another customer in our CATs but I'm not blocking the PR for that
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
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.
, assuming regression tests pass for the impacted streams (Persons, Events
…ripe-customer-payment-methods
…ripe-customer-payment-methods # Conflicts: # airbyte-integrations/connectors/source-stripe/metadata.yaml # airbyte-integrations/connectors/source-stripe/poetry.lock # airbyte-integrations/connectors/source-stripe/pyproject.toml # airbyte-integrations/connectors/source-stripe/unit_tests/test_source.py # docs/integrations/sources/stripe.md
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
…205/source-stripe-customer-payment-methods
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
UPD: |
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
/approve-and-merge reason="There is a migration guide, but no breaking change" |
@@ -109,7 +108,7 @@ def check_connection(self, logger: logging.Logger, config: MutableMapping[str, A | |||
args = self._get_stream_base_args(config) | |||
account_stream = StripeStream(name="accounts", path="accounts", use_cache=USE_CACHE, **args) | |||
try: | |||
next(account_stream.read_records(sync_mode=SyncMode.full_refresh)) | |||
next(account_stream.read_records(sync_mode=SyncMode.full_refresh), None) |
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.
What
Resolve https://github.com/airbytehq/product-request-backlog/issues/234
Payment Methods
to use customer Payment Methods endpointHow
Payment Methods
Review guide
airbyte-integrations/connectors/source-stripe/source_stripe/source.py
airbyte-integrations/connectors/source-stripe/source_stripe/schemas/payment_methods.json
User Impact
Breaking change due to schema changes in customer field: "object" -> "string"
Can this PR be safely reverted and rolled back?