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.

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

Update source-gnews manifest to use inline stream schemas #20405

Merged
merged 12 commits into from
Dec 19, 2022

Conversation

clnoll
Copy link
Contributor

@clnoll clnoll commented Dec 12, 2022

What

Updates the source-gnews low-code connector to include the stream schema inline in the manifest.
See #19499.

This is blocked by #20375.

TODO: update the airbyte-cdk version used by source-gnews once #20375 has been released.

How

  • Updates gnews.yaml to include a yaml version of the schema previously stored in the schemas directory.
  • Deletes the existing schema files.

Recommended reading order

  1. gnews.yaml
  2. Enable low-code CDK users to specify schema in the manifest #20375

🚨 User Impact 🚨

No breaking changes; the output of the discover method is the same with the inline stream schemas as it was with the on-disk schema files.

Pre-merge Checklist

  • Unit & integration tests passing.
  • Code reviews completed
  • Documentation updated
    • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • PR name follows PR naming conventions
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

@clnoll clnoll force-pushed the source-gnews-use-inline-schema branch from 2a994f6 to 1d073ae Compare December 12, 2022 23:59
@octavia-squidington-iv octavia-squidington-iv added the area/documentation Improvements or additions to documentation label Dec 13, 2022
@clnoll clnoll force-pushed the write-stream-schemas-to-manifest branch from f3d0a00 to 71782a8 Compare December 13, 2022 00:16
@clnoll clnoll requested a review from a team as a December 13, 2022 00:16
@octavia-squidington-iv octavia-squidington-iv added the CDK Connector Development Kit label Dec 13, 2022
@clnoll clnoll force-pushed the write-stream-schemas-to-manifest branch from 5bbfe37 to 625d612 Compare December 13, 2022 01:05
@clnoll clnoll force-pushed the source-gnews-use-inline-schema branch from 2624e82 to 14e30e9 Compare December 13, 2022 01:07
@octavia-squidington-iv octavia-squidington-iv removed the CDK Connector Development Kit label Dec 13, 2022
@@ -1,5 +1,109 @@
version: "0.1.0"

schemas:
search_stream_schema:
"$schema": http://json-schema.org/draft-07/schema#
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this line? I think it makes sense at the top level of a yaml file to specify the version of jsonschema to use to validate, but I doubt it does anything here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that can be removed. Just kept it in so I could diff the output of discover before and after the schema was included inline in the manifest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@girarda just pushed the update removing these lines. Did you want to review the change?

Base automatically changed from write-stream-schemas-to-manifest to master December 13, 2022 04:44
@clnoll clnoll force-pushed the source-gnews-use-inline-schema branch from df526c5 to 4e8a721 Compare December 13, 2022 21:15
@clnoll
Copy link
Contributor Author

clnoll commented Dec 13, 2022

/test connector=connectors/source-gnews

🕑 connectors/source-gnews https://github.com/airbytehq/airbyte/actions/runs/3689604894
❌ connectors/source-gnews https://github.com/airbytehq/airbyte/actions/runs/3689604894
🐛 https://gradle.com/s/t7ssi5iez6fem

Build Failed

Test summary info:

Could not find result summary

@clnoll clnoll force-pushed the source-gnews-use-inline-schema branch from 4e8a721 to f326718 Compare December 13, 2022 21:45
@@ -44,7 +146,12 @@ definitions:
nullable: "{{ ','.join(config['nullable']) }}"
from: "{{ stream_slice['start_time'] }}"
to: "{{ stream_slice['end_time'] }}"
schema_loader:
type: StaticSchemaLoader
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be InlineSchemaLoader correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😬 yep! Fixed.

@@ -6,7 +6,7 @@
from setuptools import find_packages, setup

MAIN_REQUIREMENTS = [
"airbyte-cdk~=0.1",
"airbyte-cdk~=0.13.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove the patch version from here. Right now, this will cause us to only pull the airbyte-cdk from 0.13.x onward as per PEP 440. Whereas ideally we probably want to continue to get minor bumps as the CDK continues to be changed in a non-breaking way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brianjlai this change does rely on CDK version 0.13.2 or greater. WDYT about changing it to airbyte-cdk>=0.13.2?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that we should definitely pin it to use 0.13 or above since it relies on a new feature, but the problem with explicitly marking it with the patch version 2 is that next time we build, we will only pull airbyte-cdk where minor version is 13. So in the next month if we progress to CDK 0.20.0, this connector will continue to be pinned at 0.13.x since it will only look for the latest patch. And I think based on PEP 440 ~= and >= are interchangeable. Basically I think we want to continue to get latest minors and specifying a patch in setup.py will inhibit that.

And actually given that this is a new non-breaking feature, we should probably also be bumping this to 0.14.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks for clarifying. Sounds like I should have bumped the CDK change to 0.14.0. Looks like that does exist now so I'll change this to use airbyte-cdk~=0.14.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah sorry my bad, I was getting some PRs mixed. Up this was more that in hindsight that we should've minor version bumped the cdk schema change to 14 since it was a feature add. But regardless 0.14.0 is still fine since that was released earlier today.

@@ -1,5 +1,107 @@
version: "0.1.0"

schemas:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, but can we move the schemas to the bottom of the file? i think the definitions and streams are likely to be the parts of the manifest that change most, so it's more intuitive to have them at the top.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah sorry you are correct and my mistake. I thought we did some preparsing to allow for forward references even if defined after, but doesn't seem like we do, so it's fine to have this at the top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries (sorry, deleted my comment because I thought maybe I'd spoken prematurely and was looking more closely at the issue). Going to write up an issue for updating the parser to handle forward references, since I do think that will make the yaml more readable.

@clnoll clnoll force-pushed the source-gnews-use-inline-schema branch from f326718 to 5d5e5a5 Compare December 14, 2022 00:25
search_stream:
schema_loader:
$ref: "*ref(definitions.schema_loader)"
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw I would find it more understandable if this said

  schema_loader:
    type: InlineSchemaLoader

instead of the ref

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@clnoll
Copy link
Contributor Author

clnoll commented Dec 16, 2022

@girarda @brianjlai @sherifnada did you want to rereview this? It needs an approval to merge, since we don't want to wait on the potential forward reference change.

@sherifnada
Copy link
Contributor

@clnoll no review needed from me! I just happened to see that one line where I made a comment :)

@clnoll
Copy link
Contributor Author

clnoll commented Dec 16, 2022

/test connector=connectors/source-gnews

@clnoll
Copy link
Contributor Author

clnoll commented Dec 16, 2022

/publish connector=connectors/source-gnews

🕑 Publishing the following connectors:
connectors/source-gnews
https://github.com/airbytehq/airbyte/actions/runs/3715878899


Connector Did it publish? Were definitions generated?
connectors/source-gnews

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets December 16, 2022 19:34 — with GitHub Actions Inactive
@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets December 16, 2022 19:35 — with GitHub Actions Inactive
@clnoll clnoll temporarily deployed to more-secrets December 16, 2022 19:46 — with GitHub Actions Inactive
@clnoll clnoll temporarily deployed to more-secrets December 16, 2022 19:46 — with GitHub Actions Inactive
@clnoll clnoll temporarily deployed to more-secrets December 16, 2022 22:44 — with GitHub Actions Inactive
@clnoll clnoll temporarily deployed to more-secrets December 16, 2022 22:45 — with GitHub Actions Inactive
@clnoll clnoll temporarily deployed to more-secrets December 19, 2022 14:39 — with GitHub Actions Inactive
@clnoll clnoll temporarily deployed to more-secrets December 19, 2022 14:40 — with GitHub Actions Inactive
@clnoll clnoll merged commit ddab3ce into master Dec 19, 2022
@clnoll clnoll deleted the source-gnews-use-inline-schema branch December 19, 2022 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/gnews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants