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

feat: add confluent-hub to ksqlDB docker image #4729

Merged
merged 3 commits into from
Mar 9, 2020

Conversation

vcrfxia
Copy link
Contributor

@vcrfxia vcrfxia commented Mar 6, 2020

Description

Fixes #4558

This PR:

  • adds confluent-hub to the ksqlDB docker image
  • updates the embedded Connect tutorial to showcase the new usage
  • moves instructions for building ksqlDB docker images into ksql-docker/README.md
  • updates the instructions to use the publicly available base image, so community members can follow the instructions to build docker images as well
  • updates some copy-paste vestiges of "ksql-rest-app" to "ksql-docker" where appropriate

Testing done

Ran through the new version of the embedded Connect tutorial.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@vcrfxia vcrfxia requested review from JimGalasyn and a team as code owners March 6, 2020 21:35
@@ -136,15 +136,18 @@ services:
2. Get the JDBC connector
-------------------------

[Download the JDBC connector](https://www.confluent.io/hub/confluentinc/kafka-connect-jdbc)
to your local working directory. Next, unzip the downloaded archive:
The easiest way to download connectors for use in ksqlDB with embedded {{ site.kconnect }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JimGalasyn the changes in this file should not go live until the next released version of ksqlDB, as version 0.7.0 does not contain confluent-hub (but 0.8.0 will). Is targeting master appropriate, or should I hold off on merging until the next ksqlDB release?

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 set the local directory to be whatever Compose will mount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should set the local directory to be whatever Compose will mount.

Do you mean setting the value for /path/to/local/dir in both the docker-compose and in the confluent-hub command, in order to remove the need for the user to manually uncomment the volume mount and update the directory in both places?

That makes sense to me assuming there's a directory we're always guaranteed to be able to download to, but I couldn't think of one since only absolute paths are allowed in the volume mount.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can do ./local-path/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.

Doesn't work for me locally:

$ docker run -v ./plugins:/share/confluent-hub-components <test image> confluent-hub install --no-prompt confluentinc/kafka-connect-datagen:0.2.0
docker: Error response from daemon: create ./plugins: "./plugins" includes invalid characters for a local volume name, only "[a-zA-Z0-9][a-zA-Z0-9_.-]" are allowed. If you intended to pass a host directory, use absolute path.
See 'docker run --help'.

Am I doing something wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved offline. Compose has different behavior than Docker :( docker/cli#1203

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 with a workaround.

Copy link
Member

Choose a reason for hiding this comment

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

@vcrfxia iiuc, 0.8.0 goes out with CP 5.5, so if you retarget this PR to the 5.5.x branch, it will go live when CP 5.5 goes live..

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 hm. I wasn't planning to backport to 5.5.x since it's past feature freeze, so it sounds like I should just merge to master and let the change be picked up when the 0.8.0 docs branch is cut. Thanks for clarifying!

```bash
unzip confluentinc-kafka-connect-jdbc-*.zip
docker run -v /path/to/local/dir:/share/confluent-hub-components confluentinc/ksqldb-server:0.7.0 confluent-hub install --no-prompt confluentinc/kafka-connect-jdbc:5.4.1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

5.4.1 is the current latest version of the JDBC Connector, but how should we keep this version up to date as newer versions are released?

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 the version to {{ site.cprelease }} as discussed offline.

@vcrfxia vcrfxia requested a review from MichaelDrogalis March 6, 2020 21:39
Copy link
Member

@JimGalasyn JimGalasyn left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the update!

Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

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

Nice!

@vcrfxia vcrfxia merged commit b74867a into confluentinc:master Mar 9, 2020
@vcrfxia vcrfxia deleted the confluent-hub-docker branch March 9, 2020 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include confluent-hub in the Docker image
4 participants