4776: Python CDK: Validate input config.py against spec #5457

avida · 2021-08-17T08:35:26Z

What

Resolves #4776

How

Describe the solution

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions
Connector version bumped like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed.

avida · 2021-08-17T08:43:03Z

/publish-cdk dry-run=true

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

yevhenii-ldv · 2021-08-17T12:11:45Z

airbyte-cdk/python/CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog

+## 0.1.10
+Add checking specified config againt spec for read, write, check and ddiscover commands


Suggested change

Add checking specified config againt spec for read, write, check and ddiscover commands

Add checking specified config againt spec for read, write, check and discover commands

yevhenii-ldv · 2021-08-17T12:12:12Z

airbyte-cdk/python/CHANGELOG.md

+Add checking specified config againt spec for read, write, check and ddiscover commands
+
+##0.1.9
+remove this line after rebase


No, its cause Artur has his MR with 0.1.9 version published so my is 0.1.10 https://pypi.org/project/airbyte-cdk/

I'm waiting when he will be ready, rebase my changes onto his and update this file

yevhenii-ldv · 2021-08-17T12:18:17Z

airbyte-cdk/python/airbyte_cdk/sources/utils/schema_helpers.py

+    spec_schema = spec.connectionSpecification
+    try:
+        validate(instance=config, schema=spec_schema)
+        return None


Suggested change

return None

return

yevhenii-ldv

Small changes

sherifnada

couple of changes needed:

exit code should be > 0 if config is invalid
we should have unit tests for checkin catalogs #1

sherifnada · 2021-08-17T21:31:31Z

airbyte-cdk/python/airbyte_cdk/destinations/destination.py


    @abstractmethod
    def write(
        self, config: Mapping[str, Any], configured_catalog: ConfiguredAirbyteCatalog, input_messages: Iterable[AirbyteMessage]
    ) -> Iterable[AirbyteMessage]:
        """Implement to define how the connector writes data to the destination"""

-    def _run_spec(self) -> AirbyteMessage:
-        return AirbyteMessage(type=Type.SPEC, spec=self.spec(self.logger))
+    def _run_spec(self, spec: ConnectorSpecification) -> AirbyteMessage:


we should delete this method, calling it _run_spec when it just wraps an object is misleading

sherifnada · 2021-08-17T21:35:44Z

airbyte-cdk/python/setup.py

@@ -35,7 +35,7 @@

 setup(
    name="airbyte-cdk",
-    version="0.1.8",
+    version="0.1.10",


you'll need to either merge this with arthur's changes or release separately

Ive merged Arthur's branch into mine so now this PR looks dirty before his branch will be merged on master.

sherifnada · 2021-08-17T21:38:51Z

airbyte-cdk/python/airbyte_cdk/destinations/destination.py

+            return
+        config = self.read_config(config_path=parsed_args.config)
+        check_error_msg = check_config_against_spec(config, spec)
+        if check_error_msg:


an invalid config should cause the exit code to be >0, it's invalid input. An exit code of 0 means the connector succeeded

…h' into drezchykov/4776-cdk-validate-spec

avida · 2021-08-18T08:06:52Z

@sherifnada

we should have unit tests for checkin catalogs #1

Didnt get about this. Looks like an old PR with json files and bash scripts. How can I add unit tests on it?

sherifnada

Couple of comments, feel free to merge once they are addressed and the other PR is merged

sherifnada · 2021-08-18T16:08:43Z

airbyte-cdk/python/unit_tests/destinations/test_destination.py

@@ -180,6 +180,7 @@ def test_run_check(self, mocker, destination: Destination, tmp_path):
        parsed_args = argparse.Namespace(**args)
        destination.run_cmd(parsed_args)

+        mocker.patch.object(destination, "spec", return_value=ConnectorSpecification(connectionSpecification={}))


we should add a test case here too to verify the config is validated.

It may help to look at test coverage by running ./type_check_and_test.sh

There is already detailed test case that covers validation function here: https://github.com/airbytehq/airbyte/pull/5457/files#diff-0d9087e19f5bdabe137529bb861aaaa261b39d5344e79041602a2a94c7530301R150 So Ive just added checks if this validation function is called with correct args for destination.

Thanks, done

sherifnada · 2021-08-18T16:09:39Z

airbyte-cdk/python/airbyte_cdk/sources/utils/schema_helpers.py

+
+def check_config_against_spec_or_exit(config: Mapping[str, Any], spec: ConnectorSpecification, logger: Logger):
+    """
+    Check config object against spec. In case of spec is invalid, throws


nice docstring

…-validate-spec

avida · 2021-08-19T09:59:56Z

/publish-cdk dry-run=false

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

gaart added 5 commits August 16, 2021 12:49

Add MultipleTokenAuthenticator

d8a7437

Upd docs

db9609c

Refactor

278e0af

Upd docs

a57965d

Upd multiple token support: switch to list of tokens

035f665

avida requested review from htrueman, Zirochkaa and yevhenii-ldv August 17, 2021 08:35

github-actions bot added the CDK Connector Development Kit label Aug 17, 2021

avida force-pushed the drezchykov/4776-cdk-validate-spec branch from bcc4dc6 to 257c9bb Compare August 17, 2021 08:38

4776: Python CDK: Validate input config.py against spec

c019bce

avida force-pushed the drezchykov/4776-cdk-validate-spec branch from 257c9bb to c019bce Compare August 17, 2021 08:46

Zirochkaa approved these changes Aug 17, 2021

View reviewed changes

avida requested a review from sherifnada August 17, 2021 12:04

yevhenii-ldv reviewed Aug 17, 2021

View reviewed changes

yevhenii-ldv approved these changes Aug 17, 2021

View reviewed changes

Fix typo

e3d5279

avida force-pushed the drezchykov/4776-cdk-validate-spec branch from e4bdf9a to e3d5279 Compare August 17, 2021 12:30

sherifnada suggested changes Aug 17, 2021

View reviewed changes

Dmytro Rezchykov added 3 commits August 18, 2021 10:48

fix review comments

c97588b

fix review comments

2896816

Merge remote-tracking branch 'origin/gaart/python-cdk-multi-token-aut…

aa93cea

…h' into drezchykov/4776-cdk-validate-spec

github-actions bot added the area/documentation Improvements or additions to documentation label Aug 18, 2021

avida requested a review from sherifnada August 18, 2021 08:07

sherifnada approved these changes Aug 18, 2021

View reviewed changes

github-actions bot removed the area/documentation Improvements or additions to documentation label Aug 19, 2021

Dmytro Rezchykov added 2 commits August 19, 2021 11:54

Merge remote-tracking branch 'origin/master' into drezchykov/4776-cdk…

86e7457

…-validate-spec

fix review comment

e29552e

avida force-pushed the drezchykov/4776-cdk-validate-spec branch from ebaa6aa to e29552e Compare August 19, 2021 09:58

avida merged commit b1f2bf5 into master Aug 19, 2021

avida deleted the drezchykov/4776-cdk-validate-spec branch August 19, 2021 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4776: Python CDK: Validate input config.py against spec #5457

4776: Python CDK: Validate input config.py against spec #5457

avida commented Aug 17, 2021

avida commented Aug 17, 2021 •

edited by github-actions bot

Loading

yevhenii-ldv Aug 17, 2021

avida Aug 17, 2021

yevhenii-ldv Aug 17, 2021

avida Aug 17, 2021

avida Aug 17, 2021

yevhenii-ldv Aug 17, 2021

avida Aug 17, 2021

yevhenii-ldv left a comment

sherifnada left a comment

sherifnada Aug 17, 2021

avida Aug 18, 2021

sherifnada Aug 17, 2021

avida Aug 18, 2021

sherifnada Aug 17, 2021

avida Aug 18, 2021

avida commented Aug 18, 2021

sherifnada left a comment

sherifnada Aug 18, 2021

avida Aug 19, 2021

avida Aug 19, 2021

sherifnada Aug 18, 2021

avida commented Aug 19, 2021 •

edited by github-actions bot

Loading

	Add checking specified config againt spec for read, write, check and ddiscover commands
	Add checking specified config againt spec for read, write, check and discover commands

4776: Python CDK: Validate input config.py against spec #5457

4776: Python CDK: Validate input config.py against spec #5457

Conversation

avida commented Aug 17, 2021

What

How

Recommended reading order

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

avida commented Aug 17, 2021 • edited by github-actions bot 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

yevhenii-ldv left a comment

Choose a reason for hiding this comment

sherifnada left a 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

avida commented Aug 18, 2021

sherifnada left a 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

avida commented Aug 19, 2021 • edited by github-actions bot Loading

avida commented Aug 17, 2021 •

edited by github-actions bot

Loading

avida commented Aug 19, 2021 •

edited by github-actions bot

Loading