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

[Connector Builder] upgrade orval and use connector manifest schema directly #20166

Merged
merged 4 commits into from
Dec 7, 2022

Conversation

lmossman
Copy link
Contributor

@lmossman lmossman commented Dec 6, 2022

What

Resolves #18258

As described in the above issue, until now orval (the openAPI -> typescript generator library we use) had issues with pulling in external JSON schema files.

After several rounds of back-and-forth on this orval issue, as of version 6.11.0-alpha.10 all of the issues I have found have now been resolved!

How

This PR therefore upgrades to that version of orval, and updates the connector builder OpenAPI spec to pull in the connector manifest schema directly into the API and use it where relevant. This is important for upcoming changes to the connector builder UI which will utilize these types.

This means that a ConnectorManifest typescript type is now generated in the ConnectorBuilderClient.ts file, so it can be used for type-checking of manifest objects in the connector builder, which makes up the other changes in this PR. This is important for upcoming changes to the connector builder UI which will utilize these types.

Recommended reading order

  1. x.java
  2. y.python

🚨 User Impact 🚨

There should be no visible user impact as a result of this change.

@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Dec 6, 2022
Comment on lines -191 to -192
# AirbyteProtocol:
# $ref: ../../../../airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing AirbyteProtocol from this spec, because it still has issues being pulled in which aren't the fault of orval, but more an incompatibility between JSON schema features that the AirbyteProtocol is using and the version of OpenAPI that most generator tools still support.

This means we are in solution number 2 as described here which is a totally satisfactory position to be in.

@lmossman lmossman marked this pull request as ready for review December 6, 2022 23:06
@lmossman lmossman requested a review from a team as a code owner December 6, 2022 23:06
@lmossman lmossman temporarily deployed to more-secrets December 6, 2022 23:15 — with GitHub Actions Inactive
@lmossman lmossman temporarily deployed to more-secrets December 6, 2022 23:15 — with GitHub Actions Inactive
@lmossman lmossman temporarily deployed to more-secrets December 6, 2022 23:21 — with GitHub Actions Inactive
@lmossman lmossman temporarily deployed to more-secrets December 6, 2022 23:21 — with GitHub Actions Inactive
from connector_builder.generated.models.declarative_stream import DeclarativeStream


class ConnectorManifest(BaseModel):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI @girarda @brianjlai since this PR is having the connector builder server OpenAPI spec reference the connector manifest schema directly, it is now generating a lot more models.

Is this setup okay to move forward with for now? I'm assuming we will either start to consume these models or remove them as part of #20044

Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

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

:shipit:

@lmossman lmossman merged commit e55757b into master Dec 7, 2022
@lmossman lmossman deleted the lmossman/use-connector-manifest-schema-directly branch December 7, 2022 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix connector builder openAPI spec codegen problems
3 participants