Switched the Cassandra Schema image to use the official image as base image #278
Conversation
jpkrohling
requested review from
black-adder,
oibe,
vprithvi and
yurishkuro
as code owners
|
This is the first step required to move the templates to use the official Cassandra image from DockerHub. |
|
I previously ran into an issue #244 where the image for schema creation did not work. Do you know if this change fixes it? Do we currently test that? |
|
Yes, I ran into the same issue when deploying a Cassandra 3.11 and running the existing image for the schema. |
|
As for testing: the image is tested as part of the (still manual?) tests on the deployment templates (Kubernetes/OpenShift). |
| @@ -3,7 +3,7 @@ | |||
| # This script is used in the Docker image jaegertracing/jaeger-cassandra-schema | |||
| # that allows installing Jaeger keyspace and schema without installing cqlsh. | |||
|
|
|||
| CQLSH=/opt/apache-cassandra-3.0.12/bin/cqlsh | |||
| CQLSH=/usr/bin/cqlsh | |||
There was a problem hiding this comment.
Could we make this configurable? And leave other env variables to be set to default values in create.sh?
There was a problem hiding this comment.
This is very specific to the base image that is being used. This script is not meant to be used anywhere else, so, I see little value in making this configurable.
There was a problem hiding this comment.
Could be this change avoided if it was configurable? I think yes.
There was a problem hiding this comment.
Maybe what @pavolloffay means is that the script could use an env var that would be set inside the Dockerfile, where it's more closely related to the actual image being used.
There was a problem hiding this comment.
Exactly, this script is just a wrapper for cqlsh. I don't know why it even sets env vars like DATACENCER, MODE, I think it should delegate most of the stuff to create.sh
There was a problem hiding this comment.
I would also propose renaming it. docker.sh is vague. What it does actually with docker? I could use this script with ccm like CQLSH="ccm node1 cqlsh" docker.sh
There was a problem hiding this comment.
I don't think making the env var would have been a great time saver for a change like this. But in any case, I'll make it use an env var, defaulting to /usr/bin/cqlsh
About the other proposed changes, they are not the scope of this change. Perhaps this could be done in a follow up PR, but the script was done to work strictly within a Docker image, and I have not tested it in any other situation.
|
k8s deployments use travis for both prod and all-in-one template. |
| @@ -1,5 +1,4 @@ | |||
| # TODO: replace this by the final Cassandra image | |||
| FROM jpkroehling/cassandra | |||
| FROM cassandra:3.11 | |||
There was a problem hiding this comment.
@yurishkuro is this version fine? Shouldn't we use the same version as xdock (3.9) ?
There was a problem hiding this comment.
Don't have a strong opinion either way. I think generally later versions of cqlsh work fine with older versions of Cassandra, but not vice versa.
|
PR updated to let CQLSH be set as env var. |
|
Something odd with the build / Travis, but I am going to merge this anyway. |
No description provided.