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

fix: Include functional tests jar in docker images #4274

Merged
merged 4 commits into from
Jan 29, 2020

Conversation

vpapavas
Copy link
Member

@vpapavas vpapavas commented Jan 10, 2020

Description

Fixes #3989

We need to include the functional tests jar in the docker image. Instead of creating two new modules that will build the two images for server and cli respectively, we decided to build a single image for both server and cli that will be called ksqldb.

This PR adds a new module that is used only for building the ksqldb docker image. (The actual name of the image (ksqldb) will be assigned by the jenkins tool.)

Testing done

Tried the docker image and am able to run server, cli and testing tool.

  1. Build local image:
mvn -Pdocker package -DskipTests -Dspotbugs.skip -Dcheckstyle.skip  -Ddockerfile.skip=false -Dskip.docker.build=false -Ddocker.upstream-tag=master-latest -Ddocker.tag=local.build -Ddocker.upstream-registry='368821881613.dkr.ecr.us-west-2.amazonaws.com/' -pl :ksql-docker -am -T 4
  1. Try server
docker exec -it ksqldb-server bash
> ksql
  1. Try cli
docker exec -it ksqldb-server ksql http://ksqldb-server:8088
  1. Steps for testing tool (followed with modifications from here: https://docs.confluent.io/current/ksql/docs/developer-guide/ksql-testing-tool.html)

a. Open terminal into container:

docker exec -it ksqldb-server bash

b. Create files input.json, output.json, statement.json and copy into container

docker cp input.json ksqldb-server:.

c. Copy test jars into `/usr/share/java/ksql-rest-app/

wget https://repo1.maven.org/maven2/junit/junit/4.12/junit-4.12.jar
wget https://repo1.maven.org/maven2/org/hamcrest/hamcrest-all/1.3/hamcrest-all-1.3.jar

d. Run testing tool:

ksql-test-runner -s statements.sql -i input.json -o output.json

e. Output is:

	>>>>> Test failed: Invalid key value for topic test_topic.
Expected KeyType: STRING
Actual KeyType: INTEGER, key: 0.

Tried same steps with image created from jenkins script.

Tried quickstart (https://ksqldb.io/quickstart.html), currently fails due to : #4400 .

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
    Docs for using the testing tool with docker image need to get created/updated!
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@vpapavas vpapavas requested a review from a team as a code owner January 10, 2020 22:04
@vpapavas vpapavas requested a review from agavra January 10, 2020 22:04
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

Overall, this LGTM! I think it's much cleaner to have this be a single module at the end than what we currently have.

But.... hate to do this to you 😂 can you please merge #4260 with this? Also, open up a ticket to delete the old dockerfiles etc.. after we've migrated the Jenkins job to build from these ones.

ksql-docker/docker-compose.yml Outdated Show resolved Hide resolved
ksql-docker/pom.xml Outdated Show resolved Hide resolved
ksql-docker/pom.xml Outdated Show resolved Hide resolved
@agavra agavra self-assigned this Jan 11, 2020
@agavra agavra requested a review from a team January 11, 2020 00:55
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

Thanks @vpapavas for taking a stab at this! One last thing if you don't mind, I think it might make sense to include a package-info.java (see this) explaining what this package does (and why we need it)

removed not needed files

rename module

remove docker compose file
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.

Docker images do not include testing-tool JARs
2 participants