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

Bulk Core/Load CDK: Support for connector-type-specific non-config sp… #45463

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

johnny-schmidt
Copy link
Contributor

What

Adds general support for extending the generated specification to include values specific to the destination type but not available on the metadata or the configuration json.

This is specifically to meet destination needs for expressing feature-specific things like support for refreshes.

Example usage is here

How

Core: Adds an injection point for a specification extender and a default identity implementation
Load: Adds a destination-specific implementation of the extender that applies fields from a (required) injected destination specification.

@johnny-schmidt johnny-schmidt requested review from a team as code owners September 13, 2024 17:13
Copy link

vercel bot commented Sep 13, 2024

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

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Sep 13, 2024 6:04pm

@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Sep 13, 2024
@johnny-schmidt johnny-schmidt force-pushed the issue-9365/destination-spec-extension branch from d569b01 to 66ee962 Compare September 13, 2024 17:21
Copy link
Contributor

@edgao edgao left a comment

Choose a reason for hiding this comment

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

seems like a reasonable implementation. nonblocking question (feel free to merge as-is): do we want to keep this in-code, or do we eventually want to parse these out of some resource file?

(it's convenient that I can currently look at <path-to-connector>/spec.json for any connector, but I don't really care that much. Maybe we're better off having the code be source of truth, but autogen some spec.json equivalent for human reference?)

}
}

interface SpecificationExtender : (ConnectorSpecification) -> ConnectorSpecification
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL you can do this. that's cool!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes @postamar suggested that (in a different context). It's cleaner than using functions as arguments and allows you to extend/inject as needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this may or should be a fun interface. I don't know. I'm not sure it matters either.

@@ -15,13 +16,24 @@ import java.net.URI
class SpecOperation(
@Value("\${airbyte.connector.metadata.documentation-url}") val documentationUrl: String,
val configJsonObjectSupplier: ConfigurationJsonObjectSupplier<*>,
val specificationExtender: SpecificationExtender,
Copy link
Contributor

Choose a reason for hiding this comment

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

naming nit: given that we're calling this as a function, can we rename to a verb? extendSpecification or something

@johnny-schmidt johnny-schmidt force-pushed the issue-9365/destination-spec-extension branch from 66ee962 to 817cc46 Compare September 13, 2024 18:04
@johnny-schmidt johnny-schmidt enabled auto-merge (squash) September 13, 2024 18:04
@johnny-schmidt johnny-schmidt merged commit 91013e1 into master Sep 16, 2024
29 checks passed
@johnny-schmidt johnny-schmidt deleted the issue-9365/destination-spec-extension branch September 16, 2024 16:58
@postamar
Copy link
Contributor

I forgot to thank you for this! Thank you, this is neat.

@postamar
Copy link
Contributor

We'll need this to transform the JSON schema AST too. It's not something that I look forward to but it's necessary to deal with groups and other platform UI idiosyncracies.

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