-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
[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
Changes from all commits
82ea25e
0c69179
9ce4b48
5aa17e2
5351429
a8f33a3
2be92c7
48f2595
8e15f32
582ccb0
360db7f
17c38a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -246,7 +246,11 @@ def is_object_definition_with_class_name(definition): | |
|
||
@staticmethod | ||
def is_object_definition_with_type(definition): | ||
return isinstance(definition, dict) and "type" in definition | ||
# The `type` field is an overloaded term in the context of the low-code manifest. As part of the language, `type` is shorthand | ||
# for convenience to avoid defining the entire classpath. For the connector specification, `type` is a part of the spec schema. | ||
# For spec parsing, as part of this check, when the type is set to object, we want it to remain a mapping. But when type is | ||
# defined any other way, then it should be parsed as a declarative component in the manifest. | ||
return isinstance(definition, dict) and "type" in definition and definition["type"] != "object" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the name of the method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem with how are connector specifications are defined, the It's a little bit of a hack, but it allows us to make the manifest defined and external defined specs interchangable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @brianjlai should we rename our There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add this context as a comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup will add! |
||
|
||
@staticmethod | ||
def get_default_type(parameter_name, parent_class): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# | ||
# Copyright (c) 2022 Airbyte, Inc., all rights reserved. | ||
# | ||
|
||
from airbyte_cdk.sources.declarative.spec.spec import Spec | ||
|
||
__all__ = ["Spec"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
# | ||
# Copyright (c) 2022 Airbyte, Inc., all rights reserved. | ||
# | ||
|
||
from dataclasses import InitVar, dataclass | ||
from typing import Any, Mapping | ||
|
||
from airbyte_cdk.models.airbyte_protocol import ConnectorSpecification | ||
from dataclasses_jsonschema import JsonSchemaMixin | ||
|
||
|
||
@dataclass | ||
class Spec(JsonSchemaMixin): | ||
""" | ||
Returns a connection specification made up of information about the connector and how it can be configured | ||
|
||
Attributes: | ||
documentation_url (str): The link the Airbyte documentation about this connector | ||
connection_specification (Mapping[str, Any]): information related to how a connector can be configured | ||
""" | ||
|
||
documentation_url: str | ||
connection_specification: Mapping[str, Any] | ||
options: InitVar[Mapping[str, Any]] | ||
|
||
def generate_spec(self) -> ConnectorSpecification: | ||
""" | ||
Returns the connector specification according the spec block defined in the low code connector manifest. | ||
""" | ||
|
||
# We remap these keys to camel case because that's the existing format expected by the rest of the platform | ||
return ConnectorSpecification.parse_obj( | ||
{"documentationUrl": self.documentation_url, "connectionSpecification": self.connection_specification} | ||
Comment on lines
+32
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @brianjlai how this will affect existing low-code connectors using the previous format? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should not, in the event that a |
||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
from enum import Enum, EnumMeta | ||
from typing import Any, List, Mapping, Union | ||
|
||
from airbyte_cdk.models import ConnectorSpecification | ||
from airbyte_cdk.sources.declarative.checks import CheckStream | ||
from airbyte_cdk.sources.declarative.checks.connection_checker import ConnectionChecker | ||
from airbyte_cdk.sources.declarative.declarative_source import DeclarativeSource | ||
|
@@ -33,7 +34,7 @@ class ConcreteDeclarativeSource(JsonSchemaMixin): | |
class YamlDeclarativeSource(DeclarativeSource): | ||
"""Declarative source defined by a yaml file""" | ||
|
||
VALID_TOP_LEVEL_FIELDS = {"definitions", "streams", "check", "version"} | ||
VALID_TOP_LEVEL_FIELDS = {"check", "definitions", "spec", "streams", "version"} | ||
|
||
def __init__(self, path_to_yaml): | ||
""" | ||
|
@@ -69,6 +70,28 @@ 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 connector specification (spec) as defined in the Airbyte Protocol. The spec is an object describing the possible | ||
configurations (e.g: username and password) which can be configured when running this connector. For low-code connectors, this | ||
will first attempt to load the spec from the manifest's spec block, otherwise it will load it from "spec.yaml" or "spec.json" | ||
in the project root. | ||
""" | ||
|
||
self.logger.debug( | ||
"parsed YAML into declarative source", | ||
extra={"path_to_yaml_file": self._path_to_yaml, "source_name": self.name, "parsed_config": json.dumps(self._source_config)}, | ||
) | ||
|
||
spec = self._source_config.get("spec") | ||
if spec: | ||
if "class_name" not in spec: | ||
spec["class_name"] = "airbyte_cdk.sources.declarative.spec.Spec" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if the developer uses a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All three formats should work. If they define the |
||
spec_component = self._factory.create_component(spec, dict())() | ||
return spec_component.generate_spec() | ||
else: | ||
return super().spec(logger) | ||
|
||
def _read_and_parse_yaml_file(self, path_to_yaml_file): | ||
package = self.__class__.__module__.split(".")[0] | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 andtype
:object
gets added the component. Then, when because it has atype
we attempt to resolve it in theCLASS_TYPES_REGISTRY
which will correctly fail becausetype
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting. I assume the request options provider's field work because they're
RequestInput
, not rawMapping
?