-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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-linkedin-ads): move custom streams to manifest only #48863
base: master
Are you sure you want to change the base?
feat(source-linkedin-ads): move custom streams to manifest only #48863
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…treams-to-manifest-only
…treams-to-manifest-only
…treams-to-manifest-only
…ifest-only' of github.com:airbytehq/airbyte into lazebnyi/source-linkedin-ads-move-custom-streams-to-manifest-only
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.
Overall looks good to me. I'm eager to remove the custom code. Just a couple of concerns/questions
airbyte-integrations/connectors/source-linkedin-ads/source_linkedin_ads/source.py
Show resolved
Hide resolved
airbyte-integrations/connectors/source-linkedin-ads/source_linkedin_ads/manifest.yaml
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-linkedin-ads/unit_tests/test_source.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-linkedin-ads/unit_tests/test_source.py
Outdated
Show resolved
Hide resolved
…treams-to-manifest-only
/format-fix
|
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! One last thing would be to run/analyze a relevant regression test output. I'll still preemptively approve this
What
With new version of CDK streams can be created dynamically via config deduction. So we can move from custom implementation to cdk based.
Fixed: https://github.com/airbytehq/airbyte-internal-issues/issues/10790
How
Deleted all custom method from source and add dynamic streams definition to the manifest. Stream name moved to a higher level, as the record selector now also requires the stream name for instance creation, like the retriever.
Review guide
manifest.yaml
source.py
NOTE: COULD BE REVIEW/TESTED WITH CI AFTER THIS - airbytehq/airbyte-python-cdk#149
User Impact
No
Can this PR be safely reverted and rolled back?