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: Do not rely on local cdk anymore #22391

Merged
merged 7 commits into from
Feb 7, 2023

Conversation

flash1293
Copy link
Contributor

What

Related to #21830

Downloads the CDK in the specified version as part of the frontend build to build typescript types instead of relying on CDK files in a folder as part of the same repo.

This is not fixing the linked issue as it's not ensuring connector builder server and webapp use the same version of the CDK, it's done to unblock the repo split in the short term.

How

Do a curl request to fetch the tar ball of the cdk, then extract the declarative manifest schema from it. Also add a way to overwrite the used version and location of the manifest schema via environment variables to allow local development.

@octavia-squidington-iii octavia-squidington-iii added the area/frontend Related to the Airbyte webapp label Feb 5, 2023
@flash1293 flash1293 marked this pull request as ready for review February 5, 2023 23:19
@flash1293 flash1293 requested review from timroes and lmossman February 5, 2023 23:19
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Feb 6, 2023
Comment on lines +4 to +5
# Make sure this is aligned with the CDK version of the connector builder server
DEFAULT_CDK_VERSION="0.25.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirming, is the plan to address keeping these aligned automatically in a follow-up PR, since in the description you said this PR does not fully close that issue?

Do you have an idea of how to solve that?

Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

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

One small nit about adding a comment but up to you on if you think it is necessary to add.

Otherwise this LGTM, tested locally and it works as expected

docs/contributing-to-airbyte/developing-locally.md Outdated Show resolved Hide resolved
@flash1293 flash1293 enabled auto-merge (squash) February 7, 2023 16:29
@flash1293 flash1293 merged commit aace7ae into master Feb 7, 2023
@flash1293 flash1293 deleted the flash1293/pin-cdk-manifest branch February 7, 2023 16:53
danidelvalle pushed a commit to danidelvalle/airbyte that referenced this pull request Feb 9, 2023
* do not rely on local cdk anymore

* cache versioned schemas

* fix used version

* review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation area/frontend Related to the Airbyte webapp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants