Skip to content
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.

Sign up for GitHub

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

Jump to bottom

🎉 CDK: support loading spec from yaml file #12104

Merged
merged 7 commits into from
Apr 20, 2022
Merged

🎉 CDK: support loading spec from yaml file #12104

merged 7 commits into from
Apr 20, 2022

Conversation

pedroslopez
Copy link
Contributor

@pedroslopez pedroslopez commented Apr 18, 2022 •
edited
Loading

What

Solves #11888 by adding support for loading the connector spec from a spec.yaml file in addition to the existing spec.json.

How

  • Look for spec.yaml and use that if present. Existing support for json files remains unchanged.
  • If both a spec.yaml and spec.json exist, an exception will be raised.
  • Added tests around the connector spec method.

Tests

Unit

391 passed, 537 warnings in 4.14s

@pedroslopez pedroslopez marked this pull request as draft April 18, 2022 15:35
@github-actions github-actions bot added the CDK Connector Development Kit label Apr 18, 2022
@pedroslopez pedroslopez linked an issue Apr 18, 2022 that may be closed by this pull request
Support reading YAML files for specs #11888
Closed
@pedroslopez pedroslopez marked this pull request as ready for review April 19, 2022 15:03
@pedroslopez pedroslopez changed the title support loading spec from yaml file 🎉 CDK: support loading spec from yaml file Apr 19, 2022
@pedroslopez pedroslopez requested a review from Phlair April 19, 2022 15:10
Phlair
Phlair suggested changes Apr 20, 2022
View reviewed changes
Copy link
Contributor

@Phlair Phlair left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff, functionality looks spot on, just some minor comments about raising correct Exceptions.

New unit tests are also great! Requesting the refactor into TestAirbyteSpec, as per my comments, for approval 👍

airbyte-cdk/python/airbyte_cdk/connector.py Outdated Show resolved Hide resolved
airbyte-cdk/python/airbyte_cdk/connector.py Outdated Show resolved Hide resolved
airbyte-cdk/python/airbyte_cdk/connector.py Show resolved Hide resolved
airbyte-cdk/python/unit_tests/test_connector.py Show resolved Hide resolved
airbyte-cdk/python/unit_tests/test_connector.py Show resolved Hide resolved
airbyte-cdk/python/unit_tests/test_connector.py Show resolved Hide resolved
airbyte-cdk/python/unit_tests/test_connector.py Outdated Show resolved Hide resolved
airbyte-cdk/python/unit_tests/test_connector.py Outdated Show resolved Hide resolved
@Phlair
Copy link
Contributor

Phlair commented Apr 20, 2022 •
edited
Loading

@pedroslopez I'm assuming you're going to implement an example spec.yaml and update docs to explain yaml specs in another PR?

@pedroslopez
Copy link
Contributor Author

@pedroslopez I'm assuming you're going to implement an example spec.yaml and update docs to explain yaml specs in another PR?

Yep! I tried it out locally by using editable pip dependencies (-e) but I believe the new version of the CDK needs to be published before I can start updating some connectors. Docs changes will be pushed up to a new PR today.

@pedroslopez pedroslopez requested a review from Phlair April 20, 2022 12:39
Phlair
Phlair approved these changes Apr 20, 2022
View reviewed changes
Copy link
Contributor

@Phlair Phlair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

@pedroslopez
Copy link
Contributor Author

pedroslopez commented Apr 20, 2022 •
edited by github-actions bot
Loading

/publish-cdk dry-run=true

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

@pedroslopez
Copy link
Contributor Author

pedroslopez commented Apr 20, 2022 •
edited by github-actions bot
Loading

/publish-cdk dry-run=false

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

@pedroslopez pedroslopez merged commit 53799cb into master Apr 20, 2022
@pedroslopez pedroslopez deleted the pedroslopez/cdk-yaml-specs branch April 20, 2022 20:18
@pedroslopez pedroslopez mentioned this pull request Apr 20, 2022
Merged
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Apr 21, 2022
Merged
suhomud pushed a commit that referenced this pull request May 23, 2022
@pedroslopez @suhomud
🎉 CDK: support loading spec from yaml file (#12104)
a3c1d0c 
* support loading spec from yaml file

* formatting

* remove commented code

* update comment

* remove unused file

* raise correct exception types

* bump version, update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Phlair Phlair Phlair approved these changes

Assignees
No one assigned
Labels
CDK Connector Development Kit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support reading YAML files for specs
2 participants
@pedroslopez @Phlair