[low-code cdk] Allow for spec file to be defined in the yaml manifest instead of an external file #18411

brianjlai · 2022-10-25T01:01:40Z

What

Allows for developers to define the connector specification used in the spec command within the manifest file instead of a separate spec.json or spec.yaml

How

Added a new spec component so we can get additional schema validation and make some of our casing more consistent (it still outputs the same format)
Update the generator to define spec in the yaml
Also fixed all of our commented out tests that weren't really working due to issues w/ how we read temp files and added better testing of edge cases involving how spec gets defined

Validation

Once we have this all reviewed I'll migrate either a connector from hacktoberfest or greenhouse to define it in the manifest to verify publishing still works. I think it should since specs in the various mask files are generated from the spec command run on the container and not the literal spec.yaml file itself.

…n external file

…d rework the code for better testing

brianjlai · 2022-10-25T01:06:02Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/parsers/factory.py

@@ -243,7 +243,7 @@ def is_object_definition_with_class_name(definition):

    @staticmethod
    def is_object_definition_with_type(definition):
-        return isinstance(definition, dict) and "type" in definition
+        return isinstance(definition, dict) and "type" in definition and definition["type"] != "object"


this makes me slightly nervous, but the connection_specification is effectively just a mapping/dict/object so when we are generating the spec component, it gets read in as an object and type: object gets added the component. Then, when because it has a type we attempt to resolve it in the CLASS_TYPES_REGISTRY which will correctly fail because type isn't in that map.

I'm not sure why we never saw this before, but I spot checked some components and didn't see any fields that were of mapping type so that might be why.

interesting. I assume the request options provider's field work because they're RequestInput, not raw Mapping?

brianjlai · 2022-10-25T01:10:48Z

...r-templates/source-configuration-based/source_{{snakeCase name}}/{{snakeCase name}}.yaml.hbs

+
+spec:
+  documentation_url: https://docsurl.com
+  connection_specification:


It's worth noting that this went from camel case to snake case. I like better consistency with our component definitions across the file and the response of a spec call will still be camel case to retain compatibility which I mention in another comment.

However, it does make it a small gotcha for migrating existing connectors that it needs to be switched, but it will be very clear and obvious since the YAML will fail validation and report that the fields are missing. I not married to either format tho.

brianjlai · 2022-10-25T01:16:56Z

...r-templates/source-configuration-based/source_{{snakeCase name}}/{{snakeCase name}}.yaml.hbs

+spec:
+  documentation_url: https://docsurl.com
+  connection_specification:
+    $schema: http://json-schema.org/draft-07/schema#


left it here as a reminder, but we can actually remove the $schema from the generator because it will get inferred to the latest version when its not present. I kind of like removing it because right now it exposes an underlying implementation detail of how we validate schemas.

Con is that it becomes a small gotcha for a developer that there could be a mismatch in schema drafts if there is some kind of incompatibility between draft versions when we always use the latest

sherifnada · 2022-10-27T00:18:50Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/yaml_declarative_source.py

@@ -69,6 +70,27 @@ def streams(self, config: Mapping[str, Any]) -> List[Stream]:
            self._apply_log_level_to_stream_logger(self.logger, stream)
        return source_streams

+    def spec(self, logger: logging.Logger) -> ConnectorSpecification:
+        """
+        Returns the spec for this integration. The spec is a JSON-Schema object describing the required configurations (e.g: username


Suggested change

Returns the spec for this integration. The spec is a JSON-Schema object describing the required configurations (e.g: username

Returns the connector specification (spec) as defined in the Airbyte Protocol. The spec is an object describing the possible configurations (e.g: username

sherifnada · 2022-10-27T00:21:52Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/yaml_declarative_source.py

+    def spec(self, logger: logging.Logger) -> ConnectorSpecification:
+        """
+        Returns the spec for this integration. The spec is a JSON-Schema object describing the required configurations (e.g: username
+        and password) required to run this integration. For low-code connectors, this will first attempt to load the spec from the


Suggested change

and password) required to run this integration. For low-code connectors, this will first attempt to load the spec from the

and password) which can be configured when running this connector. For low-code connectors, this will first attempt to load the spec from the

sherifnada · 2022-10-27T02:28:59Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/parsers/factory.py

@@ -243,7 +243,7 @@ def is_object_definition_with_class_name(definition):

    @staticmethod
    def is_object_definition_with_type(definition):
-        return isinstance(definition, dict) and "type" in definition
+        return isinstance(definition, dict) and "type" in definition and definition["type"] != "object"


the name of the method is_object... should this check be !=?

The problem with how are connector specifications are defined, the type field is an overloaded term. In the manifest, type is just shorthand so we don't have to include the full classpath. But for the connector specification object, type: object is actually part of the spec schema. So the factory gets confused and thinks we need to parse the spec as another declarative component, but we really rant this to remain a Mapping. Without this line, it looks for object in the CLASS_TYPES_REGISTRY

It's a little bit of a hack, but it allows us to make the manifest defined and external defined specs interchangable

@brianjlai should we rename our type field to something less overloaded as part of lowcode to beta?

can you add this context as a comment?

yup will add!

girarda · 2022-10-25T18:46:17Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/spec/spec.py

+
+    Attributes:
+        documentation_url (str): The link the Airbyte documentation about this connector
+        connection_specification (str): information related to how a connector can be configured


the type should be Mapping[str, Any]

oops yeah, commented to quickly

girarda · 2022-10-25T18:47:14Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/spec/spec.py

+    options: InitVar[Mapping[str, Any]]
+
+    def __post_init__(self, options: Mapping[str, Any]):
+        self._options = options


should we explicitly not store the options to ensure they are not used as part of the spec?

yeah that's a good point if we want to treat spec exactly how it is today. will update this to not persist or use options anywhere

girarda · 2022-11-01T18:15:27Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/yaml_declarative_source.py

+        spec = self._source_config.get("spec")
+        if spec:
+            if "class_name" not in spec:
+                spec["class_name"] = "airbyte_cdk.sources.declarative.spec.Spec"


what if the developer uses a type field instead of class_name?

All three formats should work. If they define the class_name then we'll just use that. We define Spec in the class_types_registry so it should work with type, and if it's omitted here then we'll insert class name here.

girarda · 2022-11-04T01:02:16Z

airbyte-cdk/python/unit_tests/sources/declarative/test_factory.py

+    documentation_url = spec.documentation_url
+    connection_specification = spec.connection_specification
+    assert documentation_url == "https://airbyte.com/#yaml-from-manifest"
+    print(connection_specification)


nit: remove the debug print

brianjlai · 2022-11-07T19:20:43Z

/publish-cdk dry-run=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/3413359869
✅ https://github.com/airbytehq/airbyte/actions/runs/3413359869

brianjlai · 2022-11-07T19:30:44Z

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/3413427564
✅ https://github.com/airbytehq/airbyte/actions/runs/3413427564

…nent * master: 🪟 🎉 Enable frontend validation for <1hr syncs (cloud) #19028 Stream returns AirbyteMessages (#18572) 🎉 New Source - Recruitee [low-code SDK] (#18671) 🎉 New source: Breezometer [low-code cdk] (#18650) Check disabled connections after protocol update (#18990) Simple default replication worker refactor (#19002) 🎉 New Source: Visma e-conomic (#18595) 🎉 New Source: Fastbill (#18593) Bmoric/extract state api (#18980) 🎉 New Source: Zapier Supported Storage (#18442) 🎉 New source: Klarna (#18385) `AirbyteEstimateTraceMessage` (#18875) Extract source definition api (#18977) [low-code cdk] Allow for spec file to be defined in the yaml manifest instead of an external file (#18411) 🐛 Source HubSpot: fix property scopes (#18624) Ensure that only 6-character hex values are passed to monaco editor (#18943)

marcosmarxm · 2022-12-07T16:43:34Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/spec/spec.py

+        return ConnectorSpecification.parse_obj(
+            {"documentationUrl": self.documentation_url, "connectionSpecification": self.connection_specification}


@brianjlai how this will affect existing low-code connectors using the previous format?

It should not, in the event that a spec is not defined in the manifest itself, it will default to using the original way of retrieving and generating the spec from the spec.yaml or spec.json file.

brianjlai added 2 commits October 24, 2022 17:55

allow for spec to be defined in the source.yaml manifest instead of a…

82ea25e

…n external file

make spec a component within the language to get schema validation an…

0c69179

…d rework the code for better testing

github-actions bot added the CDK Connector Development Kit label Oct 25, 2022

fix formatting and extra method

9ce4b48

brianjlai commented Oct 25, 2022

View reviewed changes

brianjlai marked this pull request as ready for review October 25, 2022 02:49

brianjlai requested a review from a team as a code owner October 25, 2022 02:49

brianjlai requested a review from girarda October 25, 2022 02:50

sherifnada reviewed Oct 27, 2022

View reviewed changes

girarda reviewed Nov 1, 2022

View reviewed changes

brianjlai added 2 commits November 2, 2022 17:37

pr feedback and add some more test

5aa17e2

Merge branch 'master' into brian/low_code_spec_in_manifest

5351429

girarda approved these changes Nov 4, 2022

View reviewed changes

sherifnada approved these changes Nov 4, 2022

View reviewed changes

brianjlai added 5 commits November 6, 2022 18:23

Merge branch 'master' into brian/low_code_spec_in_manifest

a8f33a3

pr feedback

2be92c7

bump airbyte-cdk version

48f2595

Merge branch 'master' into brian/low_code_spec_in_manifest

8e15f32

bump version

582ccb0

brianjlai mentioned this pull request Nov 7, 2022

fix formatting in CDK #19083

Closed

37 tasks

gradle format

360db7f

remove from manifest spec

17c38a4

brianjlai merged commit f9863d6 into master Nov 7, 2022

brianjlai deleted the brian/low_code_spec_in_manifest branch November 7, 2022 19:44

github-actions bot mentioned this pull request Nov 15, 2022

Bump helm chart version reference to 0.40.43 #19453

Closed

octavia-squidington-iii mentioned this pull request Nov 18, 2022

Bump Airbyte version from 0.40.18 to 0.40.19 #19579

Merged

marcosmarxm reviewed Dec 7, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[low-code cdk] Allow for spec file to be defined in the yaml manifest instead of an external file #18411

[low-code cdk] Allow for spec file to be defined in the yaml manifest instead of an external file #18411

brianjlai commented Oct 25, 2022 •

edited

Loading

brianjlai Oct 25, 2022 •

edited

Loading

girarda Nov 4, 2022

brianjlai Oct 25, 2022 •

edited

Loading

brianjlai Oct 25, 2022

sherifnada Oct 27, 2022

sherifnada Oct 27, 2022

sherifnada Oct 27, 2022

brianjlai Nov 3, 2022

girarda Nov 4, 2022

sherifnada Nov 4, 2022

brianjlai Nov 7, 2022

girarda Oct 25, 2022

brianjlai Nov 3, 2022

girarda Oct 25, 2022

brianjlai Nov 3, 2022

girarda Nov 1, 2022

brianjlai Nov 3, 2022

girarda Nov 4, 2022

brianjlai commented Nov 7, 2022 •

edited by github-actions bot

Loading

brianjlai commented Nov 7, 2022 •

edited by github-actions bot

Loading

marcosmarxm Dec 7, 2022

brianjlai Dec 7, 2022

	Returns the spec for this integration. The spec is a JSON-Schema object describing the required configurations (e.g: username
	Returns the connector specification (spec) as defined in the Airbyte Protocol. The spec is an object describing the possible configurations (e.g: username

	and password) required to run this integration. For low-code connectors, this will first attempt to load the spec from the
	and password) which can be configured when running this connector. For low-code connectors, this will first attempt to load the spec from the

		return ConnectorSpecification.parse_obj(
		{"documentationUrl": self.documentation_url, "connectionSpecification": self.connection_specification}

[low-code cdk] Allow for spec file to be defined in the yaml manifest instead of an external file #18411

[low-code cdk] Allow for spec file to be defined in the yaml manifest instead of an external file #18411

Conversation

brianjlai commented Oct 25, 2022 • edited Loading

What

How

Recommended reading order

Validation

brianjlai Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianjlai Oct 25, 2022 • 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

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

brianjlai commented Nov 7, 2022 • edited by github-actions bot Loading

brianjlai commented Nov 7, 2022 • edited by github-actions bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianjlai commented Oct 25, 2022 •

edited

Loading

brianjlai Oct 25, 2022 •

edited

Loading

brianjlai Oct 25, 2022 •

edited

Loading

brianjlai commented Nov 7, 2022 •

edited by github-actions bot

Loading

brianjlai commented Nov 7, 2022 •

edited by github-actions bot

Loading