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: add functional-test dependencies to Docker module #4586

Merged
merged 1 commit into from
Feb 19, 2020

Conversation

colinhicks
Copy link
Contributor

Description

Dependencies required by ksql-test-runner are not present in the artifacts copied to Docker images. cf. #4584

Testing done

I built the Docker image locally via
mvn -Pdocker package -pl ksql-docker -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/'

Then I ran the repro steps in #4584, using the placeholder/confluentinc/ksql-docker:local.build image built above.

The reported exception was not thrown.

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 #")

@colinhicks colinhicks requested a review from a team as a code owner February 18, 2020 20:08
@ghost
Copy link

ghost commented Feb 18, 2020

@confluentinc It looks like @colinhicks just signed our Contributor License Agreement. 👍

Always at your service,

clabot

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.

I'm wondering if instead of depending directly on hamcrest, why not depend on the ksql-package module? That way anything else will get copied over if we ever add it.

@colinhicks
Copy link
Contributor Author

@agavra fair point. I guess we have this line in the package pom because it needs to be built after all source modules https://github.com/confluentinc/ksql/blob/master/ksql-package/pom.xml#L32.

    <!-- Dependencies are required only to ensure this module is built last. -->

But I'm not familiar enough with the packaging details to say that ksql-package does not strictly need to go last.

A second consideration is that ksql-package brings in other net-new deps. I suppose it's not a goal at this point to aggressively limit the image size?

@agavra
Copy link
Contributor

agavra commented Feb 18, 2020

@colinhicks - that actually makes sense - let's go down this approach for now

@apurvam
Copy link
Contributor

apurvam commented Feb 19, 2020

Thanks for taking care of this @colinhicks

cc @vpapavas

@colinhicks colinhicks merged commit b023cd0 into confluentinc:5.5.x Feb 19, 2020
@colinhicks colinhicks deleted the GH-4584-fun-test-deps branch February 19, 2020 14:22
@colinhicks
Copy link
Contributor Author

Btw, I couldn't sync 5.5.x to master after merging this PR, as I don't have push permissions.

Thanks for the review!

colinhicks added a commit that referenced this pull request Feb 26, 2020
Dependencies required by ksql-test-runner were not present in the artifacts copied to Docker images. This adds them to the ksql-docker pom. cf. #4584
colinhicks added a commit that referenced this pull request Feb 27, 2020
Dependencies required by ksql-test-runner were not present in the artifacts copied to Docker images. This adds them to the ksql-docker pom. cf. #4584
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