-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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 Pinterest: migrate to low-code #37698
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
# Conflicts: # airbyte-integrations/connectors/source-pinterest/metadata.yaml # airbyte-integrations/connectors/source-pinterest/poetry.lock # airbyte-integrations/connectors/source-pinterest/pyproject.toml # docs/integrations/sources/pinterest.md
airbyte-integrations/connectors/source-pinterest/source_pinterest/components/auth.py
Show resolved
Hide resolved
airbyte-integrations/connectors/source-pinterest/source_pinterest/components/record_filter.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-pinterest/source_pinterest/components/auth.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-pinterest/source_pinterest/source.py
Show resolved
Hide resolved
airbyte-integrations/connectors/source-pinterest/source_pinterest/manifest.yaml
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-pinterest/source_pinterest/manifest.yaml
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-pinterest/source_pinterest/components/record_filter.py
Outdated
Show resolved
Hide resolved
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.
a few small things and tweaks but overall looks good w/ the adjustments to the error handling.
Have you done any live test run reports to verify that there are no regressions in the behavior? If not, can you can perform at least one or two to verify the changes, I'll ✅ once we have confirmed that
airbyte-integrations/connectors/source-pinterest/source_pinterest/manifest.yaml
Show resolved
Hide resolved
airbyte-integrations/connectors/source-pinterest/source_pinterest/manifest.yaml
Outdated
Show resolved
Hide resolved
@brianjlai here is one of runs: https://github.com/airbytehq/airbyte/actions/runs/9211964690 - the only failed test is related to discover, however, I don't see any issues when comparing original and new schemas |
I know this was a breaking change, but was this expected? When you say you don't see any issues, do you mean that the diffs on the control vs. yours do not differ at all? Or that the deviations don't change the behavior? I think I want to understand why the catalog changed and we should be sure that was intended. If you run |
I double-checked the catalogs manually - the schemas are completely the same except the order of streams in the catalogs. This is because I used different order in the |
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 double-checked the catalogs manually - the schemas are completely the same except the order of streams in the catalogs. This is because I used different order in the manifest file, however, I don't think this should cause any issues.
Additionally, I've created a connection using our test account since there are more streams with data than I could find in customers' connections and ran the tests with this connection - please see the result.
Very interesting, thank you for verifying and double checking this. If ordering of streams in the catalog is the only issue, then that should not be a problem. Approved.
My one ask is reach out to @alafanechere @clnoll about this. I'm curious if this is the desired behavior of the live test framework? In the end checking for existence is what we really care about
Hey @brianjlai & @askarpets, we definitely don't intend to be checking the stream order. I actually thought we'd fixed this. Will look into it, and sorry for the confusion! |
Thank you @clnoll! |
What
Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/7435
How
Migrated streams to low-code
Review guide
manifest.yaml
source.py
User Impact
Format of state has changed for incremental streams
Can this PR be safely reverted and rolled back?