Skip to content
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

Allow for custom requesters to be defined in low-code manifests #21001

Merged
merged 5 commits into from
Jan 7, 2023

Conversation

brianjlai
Copy link
Contributor

@brianjlai brianjlai commented Jan 4, 2023

What

In the handwritten manifest, we're not currently allowing for developers to override the default HttpRequester. This wasn't a problem to this point because there were no cases of overriding HttpRequester. However in source-monday, it is currently implementing a custom InterpolatedRequestOptionsProvider which is a component we are deprecating, so I'm advising that they instead use a CustomRequester which is why we need this now.

More context as to the why:
https://airbytehq-team.slack.com/archives/C02U9R3AF37/p1672763295299529

How

Adds CustomRequester to the YAML manifest schema and regenerated the latest models.

@brianjlai brianjlai requested a review from a team as a code owner January 4, 2023 02:08
@octavia-squidington-iv octavia-squidington-iv added the CDK Connector Development Kit label Jan 4, 2023
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

do we need any tests around this?

@brianjlai
Copy link
Contributor Author

brianjlai commented Jan 5, 2023

added test for this

@maxi297
Copy link
Contributor

maxi297 commented Jan 6, 2023

It think it is interesting to allow the custom implementations of RecordExtractor.

I also remember us discussion that it could be interesting to reduce the number of custom implementations we have. I'm not sure I understand why DpathStringExtractor from source-monday is specific to source-monday and therefore I'm wondering if it could be extracted in the DpathExtractor we already have. Would we also like to explore how we could remove DpathStringExtractor from source-monday?

@brianjlai
Copy link
Contributor Author

I'm not sure I understand why DpathStringExtractor from source-monday is specific to source-monday and therefore I'm wondering if it could be extracted in the DpathExtractor we already have.

We do sometimes ask to "promote" components that are generalizable. But we don't always have a set timeline for that. It very well could be, but as to not impact GL's velocity or if we haven't come to an exact decision on if we want to support it long term we tend to err on the side of caution. Once we allow it we'll always have to support it.

Would we also like to explore how we could remove DpathStringExtractor from source-monday?

As mentioned above, probably worth a discussion, but also falls a little outside the scope of this change. I think there is potential use case for many of the more general components (retrievers, extractors, selectors, requester, error handlers) and extractors fit into that. But with caveat that we should be only going as far as the interface level. CustomRetrievers and not CustomSimpleRetriever. The less custom components the better, but in this case, it also does demonstrate why we need them on extractors because there are use cases we don't current cover and there might be more in the future.

And also noting since it's relevant, source-monday was updated to use type: CustomRecordExtractor in a recent PR as per the new schema.

@brianjlai
Copy link
Contributor Author

brianjlai commented Jan 7, 2023

/publish-cdk dry-run=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/3859684956
https://github.com/airbytehq/airbyte/actions/runs/3859684956

@brianjlai
Copy link
Contributor Author

brianjlai commented Jan 7, 2023

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/3859723108
https://github.com/airbytehq/airbyte/actions/runs/3859723108

@brianjlai brianjlai merged commit 8f21d0d into master Jan 7, 2023
@brianjlai brianjlai deleted the brian/support_custom_requester_component branch January 7, 2023 01:17
jbfbell pushed a commit that referenced this pull request Jan 13, 2023
* Allow for custom requesters to be defined in low-code manifests

* add test for custom requester component

* bump versions and changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants