feat(source-tiktok-marketing): Migrate to manifest-only #61580
feat(source-tiktok-marketing): Migrate to manifest-only #61580
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
👋 Greetings, Contributor!Here are some helpful tips and reminders for your convenience. Helpful Resources
PR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR:
|
|
|
@brianjlai I'm happy to sit down with you tomorrow to review if we have time but I don't know what I should be looking for in here. |
pnilan
left a comment
pnilan
left a comment
There was a problem hiding this comment.
Looks good. Have one non-blocking comment assuming tests run fine and all.
| import pytest | ||
| from source_tiktok_marketing import SourceTiktokMarketing | ||
| from source_tiktok_marketing.components.advertiser_ids_partition_router import ( | ||
| from components import ( |
There was a problem hiding this comment.
@brianjlai I usually import the manifest_only_fixtures to access the components -- but now I'm wondering if that is even necessary.
i.e. in conftest.py
pytest_plugins = ["airbyte_cdk.test.utils.manifest_only_fixtures"]@ChristoGrab Can you remind me the benefit of importing the fixtures?
There was a problem hiding this comment.
yeah, i forgot to add that. That is the command I also used in conftest.py for zendesk, but forgot to add. Might be why my CI checks are failing, but think we're all on the same page. Thanks for the catch and will add
| - $ref: "#/definitions/creative_assets_portfolios_stream" | ||
| - $ref: "#/definitions/creative_assets_videos_stream" | ||
| # The following streams are only included if the config is not using a sandbox account for authentication | ||
| - type: ConditionalStreams |
|
Regression test results: Auto run w/ various connections: 
Specific test run for additional validation: 
Local TestingOne final note on testing, because of the impact to how we make available connections based on the config, I've also tested this as a dev version on Cloud and confirmed, there are 44 streams on OAuth creds and 28 streams on sandbox creds which is expected. Prod Discover (see
|
|
/approve-regression-tests see above message
|
|
one note on the CI checks, I'm not sure why we're all of a sudden having some weird things related to rate limiting or request errors, but given that the manifest-only migration did not change any part of the implementation beyond which streams are made available (not any change to the stream's sync behavior), this might be a test creds issue since regression and local testing passed. |
4 participants
Resolves: https://github.com/airbytehq/airbyte-internal-issues/issues/13282
What
Migrates
source-tiktok-marketingto manifest-only so that it can be edited in builderHow
Standard process for removing unneeded files, fixing unit/integration test suite, and modifying the manifest
The main change to note is that this is the first use of the new
ConditionalStreamscomponent which reads in the config to determine whether certain streams should be available to users. There are 28 streams that should always be shown regardless of environment and 16 streams that are only available to prod account environmentsTesting Plan:
I built a dev image and ran our test accounts for both
discoverandreadto validate existing behaviorI will run regression tests and identify 3 high volume candidates to test and validate behavior
Will be included in a progressive rollout
Review guide
The main focus is the
ConditionalStreamsblock of the manifest to make sure there is parity with the existing implementation.User Impact
n/a
Can this PR be safely reverted and rolled back?