-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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): migrate to low-code cdk #53687
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
/format-fix
|
…b.com:airbytehq/airbyte into lazebnyi/source-stripe-move-basic-to-low-code
…b.com:airbytehq/airbyte into lazebnyi/source-stripe-move-basic-to-low-code
…b.com:airbytehq/airbyte into lazebnyi/source-stripe-move-basic-to-low-code
/format-fix
|
…om:airbytehq/airbyte into lazebnyi/source-stripe-migrate-to-low-code
/format-fix
|
…om:airbytehq/airbyte into lazebnyi/source-stripe-migrate-to-low-code
/format-fix
|
Regression tests are looking good. Only auto-generated fields are changed - https://github.com/airbytehq/airbyte/actions/runs/13858784655/job/38781757496 |
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! Should we do another round of regression test for incremental syncs? What is the rollout plan?
@@ -211,7 +212,7 @@ def test_given_no_state_when_read_then_use_external_accounts_endpoint(self, http | |||
output = self._read(_config().with_start_date(_A_START_DATE), _NO_STATE) | |||
most_recent_state = output.most_recent_state | |||
assert most_recent_state.stream_descriptor == StreamDescriptor(name=_STREAM_NAME) | |||
assert most_recent_state.stream_state == AirbyteStateBlob(updated=int(_NOW.timestamp())) | |||
assert int(most_recent_state.stream_state.updated) == int(_NOW.timestamp()) |
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 that means that the format of the state has changed from int
to str
? Does that means that we can't revert this change? We should at least make sure to be explicit on Can this PR be safely reverted and rolled back?
in the PR description
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 that means that the format of the state has changed from int to str?
Yes, the low-code CDK stores states as a string.
Does that means that we can't revert this change?
I don't think so, but I need to double-check to be 100% sure.
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.
Since you're 99% there, why not switch to manifest-only completely, leaving manifest.yaml at top level, and keeping unit tests?
What
To migrate source-stripe connector relying on custom python implementation to manifest based using low code cdk and latest CDK features.
Closes airbytehq/airbyte-internal-issues#11544
Closes airbytehq/airbyte-internal-issues#11683
Closes airbytehq/airbyte-internal-issues#11752
How
The connector supports 7 groups of streams, all migrated to a low-code architecture:
Base Incremental Streams
State Condition Streams
Strict State Condition Streams
Base Incremental Partition Streams
Partitioned State Condition Streams
Lazy Read State Condition Streams
Base Incremental Partition Streams with Lazy Read Logic
Implementation details:
StateDelegatingStream
, ensuring state management and event-based retrieval.LazySimpleRetriever
, enabling efficient data fetching when dealing with large payloads.User Impact
No
Can this PR be safely reverted and rolled back?