-
Notifications
You must be signed in to change notification settings - Fork 22
feat: Add unprivileged and config-free discover for declarative static schemas #559
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
base: main
Are you sure you want to change the base?
Changes from all commits
cdd1ac9
7490ad1
9e84e1c
47bd67c
36d7f1f
b002218
acbab7e
d33dcdd
4253f28
64610b9
b228857
77772c3
6ca213c
dce4f8c
24a0919
f920f04
f01525f
769d361
c3cbad8
3cb8faf
08397ad
4066c75
943ea41
fac853c
9b37bc5
640c811
5894a1f
a79ad83
46170ad
c8dd910
2ce3b99
dd94e86
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 |
---|---|---|
|
@@ -105,7 +105,7 @@ def parse_args(args: List[str]) -> argparse.Namespace: | |
) | ||
required_discover_parser = discover_parser.add_argument_group("required named arguments") | ||
required_discover_parser.add_argument( | ||
"--config", type=str, required=True, help="path to the json configuration file" | ||
"--config", type=str, required=False, help="path to the json configuration file" | ||
) | ||
discover_parser.add_argument( | ||
"--manifest-path", | ||
|
@@ -177,19 +177,37 @@ def run(self, parsed_args: argparse.Namespace) -> Iterable[str]: | |
) | ||
if cmd == "spec": | ||
message = AirbyteMessage(type=Type.SPEC, spec=source_spec) | ||
yield from [ | ||
yield from ( | ||
self.airbyte_message_to_string(queued_message) | ||
for queued_message in self._emit_queued_messages(self.source) | ||
] | ||
) | ||
yield self.airbyte_message_to_string(message) | ||
elif ( | ||
cmd == "discover" | ||
and not parsed_args.config | ||
and not self.source.check_config_during_discover | ||
): | ||
# Connector supports unprivileged discover | ||
empty_config: dict[str, Any] = {} | ||
yield from ( | ||
self.airbyte_message_to_string(queued_message) | ||
for queued_message in self._emit_queued_messages(self.source) | ||
) | ||
yield from map( | ||
AirbyteEntrypoint.airbyte_message_to_string, | ||
self.discover(source_spec, empty_config), | ||
) | ||
elif parsed_args.config is None: | ||
# Raise a helpful error message if we reach here with no config. | ||
raise ValueError("The '--config' arg is required but was not provided.") | ||
else: | ||
raw_config = self.source.read_config(parsed_args.config) | ||
config = self.source.configure(raw_config, temp_dir) | ||
|
||
yield from [ | ||
yield from ( | ||
self.airbyte_message_to_string(queued_message) | ||
for queued_message in self._emit_queued_messages(self.source) | ||
] | ||
) | ||
if cmd == "check": | ||
yield from map( | ||
AirbyteEntrypoint.airbyte_message_to_string, | ||
|
@@ -261,7 +279,7 @@ def discover( | |
self, source_spec: ConnectorSpecification, config: TConfig | ||
) -> Iterable[AirbyteMessage]: | ||
self.set_up_secret_filter(config, source_spec.connectionSpecification) | ||
if self.source.check_config_against_spec: | ||
if self.source.check_config_during_discover: | ||
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. ❓ Am I reading this right that this is a manifest level flag? Should it be a spec level definition instead? 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. @bnchrch - I will think through this a bit more. As of now, it is a property of the connector base class as of now (defaulting to requiring check), overwritten by Declarative source to not require by default. I think the desired behavior is that for declarative sources, we'd not validate config unless dynamic schemas are needed. 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. Happy to approve if this is useful for this area. Certainly would be great to enable for all connectors via something in the spec though 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. Seems like it would take too much to add a proposal |
||
self.validate_connection(source_spec, config) | ||
catalog = self.source.discover(self.logger, config) | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.