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

docs: fix docker image names + remove monitoring interceptor docs #4076

Merged
merged 4 commits into from
Dec 10, 2019

Conversation

vcrfxia
Copy link
Contributor

@vcrfxia vcrfxia commented Dec 6, 2019

Description

Two docs changes:

Testing done

Docs-only change.

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 a review from a team as a code owner December 6, 2019 18:23
@vcrfxia vcrfxia requested a review from JimGalasyn December 6, 2019 18:24
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!

@@ -114,7 +114,7 @@ Start the ksqlDB CLI:

```bash
docker run --network tutorials_default --rm --interactive --tty \
confluentinc/cp-ksql-cli:{{ site.release }} \
confluentinc/ksqldb-cli:{{ site.release }} \
Copy link

Choose a reason for hiding this comment

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

Is the ksql command also needed here before the server endpoint? For example:

docker run --network tutorials_default --rm --interactive --tty \
    confluentinc/ksqldb-cli:0.6.0 ksql http://ksqldb-server:8088

@JimGalasyn
Copy link
Member

JimGalasyn commented Dec 9, 2019

@mikebin @vcrfxia I added the ksql command to the docker run examples, please let me know if they're correct!

Copy link

@mikebin mikebin left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise LGTM

@@ -166,7 +166,7 @@ Load the Streaming Data to KSQL
1. Launch the KSQL CLI:

```bash
docker-compose exec ksql-cli ksql http://ksql-server:8088
docker-compose exec ksql-cli ksql http://ksqldb-server:8088
Copy link

Choose a reason for hiding this comment

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

Should this be ksqldb-cli?

Copy link
Member

@JimGalasyn JimGalasyn Dec 9, 2019

Choose a reason for hiding this comment

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

I think it's correct, that's the server endpoint the CLI connects to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jim is correct, the service is named ksql-cli in the docker-compose: https://github.com/confluentinc/examples/blob/5.3.1-post/clickstream/docker-compose.yml#L97 so that part is fine. However, ksqldb-server:8088 should be ksql-server:8088 so Mike has still found a bug! Will update accordingly.

Copy link

Choose a reason for hiding this comment

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

Ah, my bad. This example is using the CP Docker images, and the containers have the ksql name instead of ksqldb. Sorry for the confusion.

@vcrfxia vcrfxia merged commit 4c6caa6 into confluentinc:master Dec 10, 2019
@vcrfxia vcrfxia deleted the ksqldb-interceptor-docs branch December 10, 2019 00:32
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.

3 participants