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

java CDK: hoist top-level gradle projects into CDK #31960

Merged
merged 22 commits into from
Oct 30, 2023

Conversation

postamar
Copy link
Contributor

@postamar postamar commented Oct 30, 2023

Fulfilling this TODO item that's been there since the inception of the Java CDK should help speed up building java connectors.

Not all projects have been hoisted, only those which connectors depend on, directly or transitively.

Fixes #31400

@vercel
Copy link

vercel bot commented Oct 30, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 30, 2023 6:31pm

@postamar postamar requested review from a team as code owners October 30, 2023 14:49
@postamar
Copy link
Contributor Author

postamar commented Oct 30, 2023

According to airbyte-ci connectors --support-level=certified --language=java list we have:

┃                                                       6 selected connectors                                                                                   
┃ ┏━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓                             
┃ ┃ Modified ┃ Connector             ┃ Language ┃ Release stage ┃ Version ┃ Folder                                                ┃                             
┃ ┡━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┩                             
┃ │ X        │ destination-bigquery  │ java     │ certified     │ 2.3.1   │ airbyte-integrations/connectors/destination-bigquery  │                             
┃ │ X        │ destination-s3        │ java     │ certified     │ 0.5.1   │ airbyte-integrations/connectors/destination-s3        │                             
┃ │ X        │ destination-snowflake │ java     │ certified     │ 3.4.2   │ airbyte-integrations/connectors/destination-snowflake │                             
┃ │ X        │ source-mongodb-v2     │ java     │ certified     │ 1.0.3   │ airbyte-integrations/connectors/source-mongodb-v2     │                             
┃ │ X        │ source-mysql          │ java     │ certified     │ 3.1.3   │ airbyte-integrations/connectors/source-mysql          │                             
┃ │ X        │ source-postgres       │ java     │ certified     │ 3.2.14  │ airbyte-integrations/connectors/source-postgres       │                             
┃ └──────────┴───────────────────────┴──────────┴───────────────┴─────────┴───────────────────────────────────────────────────────┘       

I will run the connectors test manually on each of there:

all ✅

Copy link
Contributor

@stephane-airbyte stephane-airbyte Oct 30, 2023

Choose a reason for hiding this comment

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

this is a dangerous file. won't even display the diff in github web UI... And even if it did, it would be unreadable since it's just a single line... Would you be open to remerging to main, if I reformat this file first?

Copy link
Contributor

@stephane-airbyte stephane-airbyte Oct 30, 2023

Choose a reason for hiding this comment

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

unless this is automatically generated by some test, in which case we should modify that to add proper formatting

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, this needs to be added to the .gitignore. Or rather replace the current entry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is even more dangerous than you think, in this project's gradle.build you'll find a downloadConnectorRegistry task that's a dependency of processResources and this task HTTP GETs this json file from one of our services. Yeah...

Still, this file shouldn't have been touched in this PR, only moved. I'll look into this. Thanks for noticing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I responded to your top-level comment without refreshing the page. You're right, this needs a gitignore update. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to be paranoid about very large changes. In a previous job, someone fixed spotbugs in a 3k+ files at once and at the same time buried a change to the javac command line. So thank you for not doing that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here. I try to have each change either change form or function but not both. This is a "form" change for sure.

@octavia-squidington-iv octavia-squidington-iv requested a review from a team October 30, 2023 16:41
@stephane-airbyte
Copy link
Contributor

looks like we're setting useLocalCdk to true in a lot of gradle files. Do we understand the ins and outs of doing that?

  1. does that mean that any changes to the CDK will always be pushed to the individual connectors, instead of each connector pinning their CDK version?
  2. are we OK with that change of behavior?
  3. if so, why wasn't it done to begin with?
  4. if everything becomes useLocalCdk, why not make that the top level default and remove all the override-to-default?

@postamar
Copy link
Contributor Author

postamar commented Oct 30, 2023

The useLocalCdk = true will be reverted once this PR is approved. The current workflow is to publish a new version of the CDK by writing a github comment containing publish-cdk ... and then update each connector's gradle file with the new CDK version and setting useLocalCdk back to false.

This workflow is veeeery much a work in progress and not something we intend to settle on. The CDK is fairly new as a concept, we haven't fleshed everything out.

@postamar
Copy link
Contributor Author

Thanks for the review! I'll wait for the github actions to complete and then I'll publish the CDK with version 0.2.0, and so forth, before merging.

@postamar
Copy link
Contributor Author

postamar commented Oct 30, 2023

/publish-java-cdk dry-run=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/6696909893
❌ Publish Java CDK version=0.2.0 failed!

@postamar
Copy link
Contributor Author

postamar commented Oct 30, 2023

/publish-java-cdk

🕑 https://github.com/airbytehq/airbyte/actions/runs/6697169665
✅ Successfully published Java CDK version=0.2.0!

@postamar
Copy link
Contributor Author

/approve-and-merge reason="bump CDK deps for all connectors"

@postamar
Copy link
Contributor Author

Approve-and-merge failed with client_payload is too large., merging manually instead.

@postamar postamar merged commit 7cd8020 into master Oct 30, 2023
17 of 92 checks passed
@postamar postamar deleted the postamar/hoist-into-cdk branch October 30, 2023 19:03
@edgao
Copy link
Contributor

edgao commented Oct 30, 2023

I missed this PR earlier - we intentionally excluded the typing-deduping modules from the initial CDK release because they're still under very active development and going through a lot of breaking changes. Is there a technical barrier to unhoisting those two specific projects?

@postamar
Copy link
Contributor Author

I didn't realize that. There's no obstacle to unhoisting these projects and they're pretty far down the dependency chain anyway. Alternatively, we can simply ignore the published jars for the time being and redefine the dependencies to typing-deduping as being from source, like previously. At that point, the only difference will be the gradle project name. I'll open a PR and add you as a reviewer.

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 CDK Connector Development Kit connectors/destination/azure-blob-storage connectors/destination/bigquery connectors/destination/cassandra connectors/destination/clickhouse connectors/destination/clickhouse-strict-encrypt connectors/destination/csv connectors/destination/databricks connectors/destination/dev-null connectors/destination/doris connectors/destination/dynamodb connectors/destination/e2e-test connectors/destination/elasticsearch connectors/destination/elasticsearch-strict-encryp connectors/destination/exasol connectors/destination/gcs connectors/destination/iceberg connectors/destination/kafka connectors/destination/keen connectors/destination/kinesis connectors/destination/local-json connectors/destination/mariadb-columnstore connectors/destination/mongodb connectors/destination/mongodb-strict-encrypt connectors/destination/mqtt connectors/destination/mssql connectors/destination/mssql-strict-encrypt connectors/destination/mysql connectors/destination/mysql-strict-encrypt connectors/destination/oracle connectors/destination/oracle-strict-encrypt connectors/destination/postgres connectors/destination/postgres-strict-encrypt connectors/destination/pubsub connectors/destination/pulsar connectors/destination/r2 connectors/destination/redis connectors/destination/redpanda connectors/destination/redshift connectors/destination/rockset connectors/destination/s3-glue connectors/destination/s3 connectors/destination/scylla connectors/destination/selectdb connectors/destination/snowflake connectors/destination/starburst-galaxy connectors/destination/teradata connectors/destination/tidb connectors/destination/vertica connectors/destination/yugabytedb connectors/source/bigquery connectors/source/clickhouse connectors/source/clickhouse-strict-encrypt connectors/source/cockroachdb connectors/source/db2 connectors/source/dynamodb connectors/source/e2e-test connectors/source/e2e-test-cloud connectors/source/elasticsearch connectors/source/kafka connectors/source/mongodb connectors/source/mongodb-strict-encrypt connectors/source/mongodb-v2 connectors/source/mssql connectors/source/mssql-strict-encrypt connectors/source/mysql connectors/source/mysql-strict-encrypt connectors/source/oracle connectors/source/oracle-strict-encrypt connectors/source/postgres connectors/source/python-http-tutorial connectors/source/redshift connectors/source/scaffold-java-jdbc connectors/source/sftp connectors/source/snowflake connectors/source/stock-ticker-api-tutorial connectors/source/teradata connectors/source/tidb normalization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

java cdk: hoist top-level gradle projects into CDK
4 participants