New `connector_builder` module for handling requests from the Connector Builder #23888

clnoll · 2023-03-08T22:37:09Z

What

Initial implementation of Connector Builder handlers in the CDK.
This PR just implements resolve_manifest handler, but provides the structure we'll use for implementing list_streams and stream_read.

How

Modifies the source_declarative_manifest to handle Connector Builder requests. These are identified by the presence of a __command key in the config.

If a __command key is not present in the config, we create the ManifestDeclarativeSource and then call launch on the source as usual.

Otherwise, we instantiate a ConnectorBuilderSource to handle the command. The ConnectorBuilderSource is instantiated with a ManifestDeclarativeSource. To handle the resolve_manifest request, the ConnectorBuilderSource calls resolved_manifest on the injected ManifestDeclarativeSource, and returns the result as an AirbyteRecordMessage whose structure matches the Connector Builder server’s ResolveManifest object.

airbyte-cdk/python/connector_builder/connector_builder_source.py

airbyte-cdk/python/connector_builder/README.md

airbyte-cdk/python/connector_builder/connector_builder_source.py

… Builder. Also implements `resolve_manifest` handler

maxi297

LGTM! I like the clear and concise documentation that comes with the added valud

airbyte-cdk/python/connector_builder/connector_builder_handler.py

girarda

small nits but looks good otherwise

airbyte-cdk/python/source_declarative_manifest/README.md

airbyte-cdk/python/connector_builder/connector_builder_handler.py

brianjlai

question about the return types on the handler, but lgtm!

brianjlai · 2023-03-09T18:57:37Z

airbyte-cdk/python/connector_builder/connector_builder_handler.py

+    raise NotImplementedError
+
+
+def resolve_manifest(source) -> Union[AirbyteMessage, AirbyteRecordMessage]:


It feels a little inconsistent that we return either an AirbyteMessage or an AirbyteRecordMessage. The "parent" AirbyteMessage object has fields for each of the relevant message unless the protocol has changed. Shouldn't we be returning an AirbyteMessage w/ the record or trace field set and the relevant type? Or returning a Union[AirbyteRecordMessage, AirbyteTraceMessage]?

Link to protocol: https://github.com/airbytehq/airbyte-protocol/blob/main/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml#L43-L51

Thanks for pointing that out, I think always returning AirbyteMessage makes sense.

airbyte-cdk/python/source_declarative_manifest/main.py

girarda · 2023-03-10T03:05:17Z

airbyte-cdk/python/source_declarative_manifest/main.py

+def preparse(args: List[str]) -> Tuple[str, str]:
+    parser = argparse.ArgumentParser()
+    parser.add_argument("command", type=str, help="Airbyte Protocol command")
+    parser.add_argument("--config", type=str, required=True, help="path to the json configuration file")


this must accept a --catalog parameter to respect the airbyte protocol

maxi297 · 2023-03-13T19:50:22Z

airbyte-cdk/python/source_declarative_manifest/main.py

+def handle_request(args: List[str]):
+    config = get_config_from_args(args)
+    source = create_source(config)
+    if "__command" in config:


We had our internal tech spec explaining why we use __command to pass commands from the platform to the CDK. Should we make part of it public/documented here in order to provide context to a dev reading this code so that it makes sense for him?

girarda · 2023-03-13T21:39:57Z

airbyte-cdk/python/connector_builder/main.py

+    return config
+
+
+def preparse(args: List[str]) -> Tuple[str, str]:


the interface should be the same as other connectors so this should also accept --catalog and and optional --state

It might make sense to always use AirbyteEntrypoint.parse_args

@clnoll is there a reason to use a preparse method instead of directly calling AirbyteEntrypoint.parse_args? Using the same method ensures we're using the same interface

Good call, updated to use AirbyteEntrypoint.parse_args.

girarda · 2023-03-13T21:41:00Z

airbyte-cdk/python/source_declarative_manifest/README.md

@@ -7,6 +7,8 @@ This entrypoint is used for connectors created by the connector builder. These c

 The spec operation is not supported because the config is not known when running a spec.

+This entrypoint is also the entrypoint for requests from the [Connector Builder](https://docs.airbyte.com/connector-development/config-based/connector-builder-ui/) Server. In addition to the `__injected_declarative_manifest`, the [Connector Builder backend](https://github.com/airbytehq/airbyte/blob/master/airbyte-cdk/python/connector_builder/README.md) config requires the `__command` key, whose value is one of the commands handled by the ConnectorBuilderHandler (`stream_read`, `list_streams`, or `resolve_manifest`).


it would make sense to provide complete config examples (maybe without the manifest) to clarify how this is meant to be used

Added to the connector_builder README.

girarda

question about using preparse vs AirbyteEntrypoint.parse_args but looks good otherwise

…or Builder (airbytehq#23888) Also implements `resolve_manifest` handler

…or Builder (#23888) Also implements `resolve_manifest` handler

clnoll requested a review from a team as a code owner March 8, 2023 22:37

octavia-squidington-iii added the CDK Connector Development Kit label Mar 8, 2023

clnoll requested review from girarda and maxi297 March 8, 2023 22:46

girarda reviewed Mar 8, 2023

View reviewed changes

airbyte-cdk/python/connector_builder/connector_builder_source.py Outdated Show resolved Hide resolved

airbyte-cdk/python/connector_builder/README.md Show resolved Hide resolved

airbyte-cdk/python/connector_builder/connector_builder_source.py Outdated Show resolved Hide resolved

clnoll requested a review from girarda March 9, 2023 13:23

clnoll and others added 4 commits March 9, 2023 13:57

New connector_builder module for handling requests from the Connector…

f829ac5

… Builder. Also implements `resolve_manifest` handler

Automated Commit - Formatting Changes

13a9a14

Rename ConnectorBuilderSource to ConnectorBuilderHandler

1c85330

Update source_declarative_manifest README

0dccb4e

clnoll force-pushed the cdk-connector-builder-module-resolve-manifest branch from 13faa1e to 0dccb4e Compare March 9, 2023 13:58

maxi297 approved these changes Mar 9, 2023

View reviewed changes

airbyte-cdk/python/connector_builder/connector_builder_handler.py Outdated Show resolved Hide resolved

airbyte-cdk/python/connector_builder/connector_builder_handler.py Outdated Show resolved Hide resolved

Reorganize

f7a475a

girarda approved these changes Mar 9, 2023

View reviewed changes

airbyte-cdk/python/source_declarative_manifest/README.md Outdated Show resolved Hide resolved

airbyte-cdk/python/connector_builder/connector_builder_handler.py Outdated Show resolved Hide resolved

brianjlai approved these changes Mar 9, 2023

View reviewed changes

girarda reviewed Mar 9, 2023

View reviewed changes

airbyte-cdk/python/source_declarative_manifest/main.py Outdated Show resolved Hide resolved

girarda requested changes Mar 10, 2023

View reviewed changes

CR improvements

a7911f5

maxi297 reviewed Mar 13, 2023

View reviewed changes

Give connector_builder its own main.py

aea625e

clnoll force-pushed the cdk-connector-builder-module-resolve-manifest branch from c8aee98 to aea625e Compare March 13, 2023 21:25

girarda reviewed Mar 13, 2023

View reviewed changes

Add --catalog argument

d38a760

girarda approved these changes Mar 14, 2023

View reviewed changes

clnoll added 2 commits March 14, 2023 11:14

Remove unneeded preparse

d64cf5d

Update README

8732639

clnoll merged commit 8ee32b1 into master Mar 14, 2023

clnoll deleted the cdk-connector-builder-module-resolve-manifest branch March 14, 2023 17:51

adriennevermorel pushed a commit to adriennevermorel/airbyte that referenced this pull request Mar 17, 2023

New connector_builder module for handling requests from the Connect…

e7cd23c

…or Builder (airbytehq#23888) Also implements `resolve_manifest` handler

erohmensing pushed a commit that referenced this pull request Mar 22, 2023

New connector_builder module for handling requests from the Connect…

3f83173

…or Builder (#23888) Also implements `resolve_manifest` handler

erohmensing pushed a commit that referenced this pull request Mar 22, 2023

New connector_builder module for handling requests from the Connect…

c940f1d

…or Builder (#23888) Also implements `resolve_manifest` handler

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New `connector_builder` module for handling requests from the Connector Builder #23888

New `connector_builder` module for handling requests from the Connector Builder #23888

clnoll commented Mar 8, 2023

maxi297 left a comment

girarda left a comment

brianjlai left a comment

brianjlai Mar 9, 2023 •

edited

Loading

clnoll Mar 9, 2023

clnoll Mar 9, 2023

girarda Mar 10, 2023

maxi297 Mar 13, 2023

girarda Mar 13, 2023

girarda Mar 14, 2023

clnoll Mar 14, 2023

girarda Mar 13, 2023

clnoll Mar 14, 2023

girarda left a comment

		raise NotImplementedError


		def resolve_manifest(source) -> Union[AirbyteMessage, AirbyteRecordMessage]:

		return config


		def preparse(args: List[str]) -> Tuple[str, str]:

		@@ -7,6 +7,8 @@ This entrypoint is used for connectors created by the connector builder. These c

		The spec operation is not supported because the config is not known when running a spec.

		This entrypoint is also the entrypoint for requests from the [Connector Builder](https://docs.airbyte.com/connector-development/config-based/connector-builder-ui/) Server. In addition to the `__injected_declarative_manifest`, the [Connector Builder backend](https://github.com/airbytehq/airbyte/blob/master/airbyte-cdk/python/connector_builder/README.md) config requires the `__command` key, whose value is one of the commands handled by the ConnectorBuilderHandler (`stream_read`, `list_streams`, or `resolve_manifest`).

New connector_builder module for handling requests from the Connector Builder #23888

New connector_builder module for handling requests from the Connector Builder #23888

Conversation

clnoll commented Mar 8, 2023

What

How

maxi297 left a comment

Choose a reason for hiding this comment

girarda left a comment

Choose a reason for hiding this comment

brianjlai left a comment

Choose a reason for hiding this comment

brianjlai Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girarda left a comment

Choose a reason for hiding this comment

New `connector_builder` module for handling requests from the Connector Builder #23888

New `connector_builder` module for handling requests from the Connector Builder #23888

brianjlai Mar 9, 2023 •

edited

Loading