-
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 Recharge: migrate to Low-Code #35450
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
…/migrate-to-low-code
…/migrate-to-low-code
…sponse VS Content-Length
…/migrate-to-low-code
…/migrate-to-low-code
…/migrate-to-low-code
…/migrate-to-low-code
…/migrate-to-low-code
…/migrate-to-low-code
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
…/migrate-to-low-code
I was also able to remove the |
…/migrate-to-low-code
…/migrate-to-low-code
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 had less time than I would have liked to review this again. Here are some comments:
-
I could not tests the following streams as there is no test data in our sandbox environment
- collections
- discounts
- onetimes
-
I get 3 records for the manifest version instead of 45 in the current one for the stream products. It seems like it is duplication because the API does not consider the
updated_at_min
andupdated_at_max
. Can you confirm that? I could now find if I had raised this issue or not in the previous round of review
@maxi297 Regarding your comment above:
These are not populated in for the Sandbox, and are covered with Integration Tests.
I confirm these are the duplicates. There are 3 records for the |
…/migrate-to-low-code
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.
Cool! With the confirmation that it is duplication, LGTM!
@@ -2,49 +2,49 @@ | |||
{ | |||
"type": "STREAM", | |||
"stream": { | |||
"stream_state": { "updated_at": "2050-05-18T00:00:00" }, | |||
"stream_state": { "updated_at": "2050-05-18T00:00:00Z" }, |
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.
It is look like breaking change as you change state format
The STATE output from the [
{
"subscriptions": {
"updated_at": "2023-05-13T04:07:32+00:00"
},
"addresses": {
"updated_at": "2023-05-13T04:07:34+00:00"
},
"charges": {
"updated_at": "2023-05-13T04:16:51+00:00"
},
"collections": {},
"customers": {
"updated_at": "2023-05-13T04:16:36+00:00"
},
"onetimes": {},
"discounts": {
"updated_at": "2021-05-13T12:40:17+00:00"
},
"orders": {
"updated_at": "2023-05-13T04:16:51+00:00"
},
"products": {},
"shop": {},
"metafields": {},
}
] The STATE output from the [
{
"subscriptions": {
"updated_at": "2023-05-13T04:07:32+00:00"
},
"addresses": {
"updated_at": "2023-05-13T04:07:34+00:00"
},
"charges": {
"updated_at": "2023-05-13T04:16:51+00:00"
},
"collections": {},
"customers": {
"updated_at": "2023-05-13T04:16:36+00:00"
},
"onetimes": {},
"discounts": {
"updated_at": "2021-05-13T12:40:17+00:00"
},
"orders": {
"updated_at": "2023-05-13T04:16:51+00:00"
},
"products": {},
"shop": {},
"metafields": {}
}
] I could be wrong, but I don't observe the difference you've found @lazebnyi. Could you please be more specific about where the breaking change takes place? |
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
as we have two cursor_datetime_formats
What
Resolving: https://github.com/airbytehq/airbyte-internal-issues/issues/2917
How
airbyte-cdk.declarative
(Low-code)orders
to cover it's implementationintegration tests
to cover theempty streams
:collections
discounts
onetimes
🚨 User Impact 🚨
No breaking changes for this Migration.