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 ksql-functional-tests to the ksql package #3111

Merged

Conversation

@apurvam
Copy link
Contributor

commented Jul 22, 2019

Description

The KSQL testing tool wasn't added to the ksql-package module, which meant that it will not be in the tarball, or rpms, or debs. This patch adds it.

Testing done

ksql-functional-tests.jar now exists in the built package, where previously it did not.

amehta-macbook-pro-3:ksql-package-5.4.0-SNAPSHOT-package apurva$ find . -name "*functional*"
./share/java/ksql/ksql-functional-tests-5.4.0-SNAPSHOT.jar
amehta-macbook-pro-3:ksql-package-5.4.0-SNAPSHOT-package apurva$ pwd
/Users/apurva/workspace/confluent/ksql/ksql-package/target/ksql-package-5.4.0-SNAPSHOT-package
amehta-macbook-pro-3:ksql-package-5.4.0-SNAPSHOT-package apurva$

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

@apurvam apurvam requested a review from confluentinc/ksql Jul 22, 2019

@agavra
agavra approved these changes Jul 22, 2019
Copy link
Contributor

left a comment

Thanks @apurvam!

@agavra agavra requested a review from confluentinc/ksql Jul 22, 2019

@apurvam apurvam added the bug label Jul 22, 2019

@big-andy-coates
Copy link
Member

left a comment

Until #2804 is resolved I'm not sure we should be releasing the testing tool into the wild. IMHO it looks very unprofessional to release a tool that requires a load of test jars to be pulled in. We don't really want to be packaging test jars with our main release.

@agavra

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

@big-andy-coates #2804 is the cause for #3094 - I suppose that gives your point validation

@apurvam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

This is module is already in our public docker images, so the code is already out there and there is no getting it back. This PR brings the rest of the distributions in line.

Also, however you slice it, the testing tool will have to be part of the main release in order to be used. So I think this particular PR is still going to be relevant regardless of the rest.

#2804 is about cleaning up ksql-functional-tests to not depend on test jars, which I agree is tech debt that we me must pay.

@apurvam apurvam requested a review from confluentinc/ksql Jul 24, 2019

@big-andy-coates

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

We should then be fixing #2804 with much urgency and including it in a point release of KSQL's last release!

@apurvam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

I agree that #2804 should be fixed with more urgency than it has so far been shown. And I agree that it shouldn't wait for 5.4. We can pick up the discussion there.

But as it is, our current distribution vehicles are inconsistent with each other. And most likely, the change in this PR will remain after #2804. So I am not convinced that it make sense to block this on #2804 .

@apurvam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

ping @big-andy-coates . Did you see my last comment?

@big-andy-coates

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

I did. I guess I'm just trying to block this to put pressure on getting #2804, as I personally think its really bad we've shipped test jars in a release. Like, REALLY bad.

But you're right, this is orthogonal.

@big-andy-coates
Copy link
Member

left a comment

Grudgingly approved :p

@apurvam apurvam merged commit 9548135 into confluentinc:5.3.x Aug 1, 2019

2 checks passed

Semantic Pull Request ready to be squashed
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@apurvam apurvam deleted the apurvam:add-functional-tests-to-packaging branch Aug 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.