Skip to content

✨Source Outreach: Migrate Python CDK to Low-code CDK #36602

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 15 commits into from
Closed

✨Source Outreach: Migrate Python CDK to Low-code CDK #36602

wants to merge 15 commits into from

Conversation

btkcodedev
Copy link
Collaborator

What

Migrating Source Outreach to Low-Code CDK
Closes airbytehq/airbyte-internal-issues#6916

How

Developed using (Configuration Based Source) low-code CDK

Recommended reading order

  1. spec.yaml
  2. manifest.yaml
  3. schemas/*

Tests

Integration & Acceptance
  • Test successful with incremental syncs

image

🚨 User Impact 🚨

  • Migration to low-code

@vercel Vercel
Copy link

vercel bot commented Mar 28, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 7, 2024 7:07am

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/outreach labels Mar 28, 2024
@btkcodedev
Copy link
Collaborator Author

airbyte-ci test - Full pass state ✅

  • Pagination ✅
  • Incremental sync ✅
  • CustomComponent - For record extraction and Incremental sync ✅

Result:
image

@btkcodedev
Copy link
Collaborator Author

Awaiting review 😄

girarda
girarda reviewed Mar 28, 2024
View reviewed changes
Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leaving surface-level comments for now. Will report again after regression testing

airbyte-integrations/connectors/source-outreach/source_outreach/components.py
) -> MutableMapping[str, Any]:
params = {}
if self.cursor_field in stream_state:
params[f"filter[{self.cursor_field}]"] = stream_state[self.cursor_field] + "..inf"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we use a standard DatetimeBasedCursor and send requests with filter {{ stream_interval.start_time}}..{{ stream_interval.end_time }}?

This might lead to submitting more requests in the case where there are very few records, but it would allow us to checkpoint between every interval which I see as a win here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please elaborate on the change, I am a little new to the stream_interval variable, Where could we retrieve that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the query parameter is passed, the API is not responding as it should,
image

Showing error as Invalid DateTime given (2020-11-16T00:00:00.000000Z). DateTime filter parameters must be in the format \"YYYY-MM-DD\", \"YYYY-MM-DDTHH:MMZ\", \"YYYY-MM-DDTHH:MM:SSZ\", or \"YYYY-MM-DDTHH:MM:SS.UUUZ\"

Airbyte automatically converts stream_interval.start_time and stream_interval.end_time to cursor format, but backend is not recognizing it, so better to leave as CustomIncremental Sync as current is good I guess 😄

airbyte-integrations/connectors/source-outreach/source_outreach/schemas/sequence_steps.json Outdated
@@ -81,7 +81,7 @@
"creator": {
"type": ["null", "array"],
"items": {
"type": "string"
"type": ["string", "integer"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's flag this in the breaking change. I think this is important information because it means the field type in the destination and in downstream systems need to support both string and integer

Copy link
Collaborator Author

@btkcodedev btkcodedev Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Edited in metadata breaking change message

@btkcodedev btkcodedev requested a review from girarda March 29, 2024 13:49
@alafanechere alafanechere added the low-code-migration This connector has been migrated to the low-code CDK label Apr 3, 2024
@btkcodedev
Copy link
Collaborator Author

All comments resolved :)

@btkcodedev
Copy link
Collaborator Author

Inline schema ✅
Poetry lock file ✅
Breaking change referred in metadata ✅
Add migration.md docs file ✅
Docs updated ✅

airbyte-ci test - All pass
image

@btkcodedev
Copy link
Collaborator Author

Marking PR as ready to merge

@btkcodedev
Copy link
Collaborator Author

Continued in #36954

@btkcodedev btkcodedev closed this Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@girarda girarda Awaiting requested review from girarda

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/outreach low-code-migration This connector has been migrated to the low-code CDK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@btkcodedev @girarda @alafanechere @octavia-squidington-iii