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

Use CQLSH_HOST in final call to cqlsh as well #1372

Merged
merged 1 commit into from
Feb 22, 2019

Conversation

funny-falcon
Copy link
Contributor

Which problem is this PR solving?

  • docker script doesn't use CQLSH_HOST variable for final call to cqlsh
    I suppose, it makes impossible to use docker image on other host that is not part of cassandra cluster.

Short description of the changes

  • add CQLSH_HOST variable in place of host argument

@codecov
Copy link

codecov bot commented Feb 22, 2019

Codecov Report

Merging #1372 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1372   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         164     164           
  Lines        7376    7376           
======================================
  Hits         7376    7376

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afa5432...3241486. Read the comment docs.

@black-adder black-adder merged commit 2d6b35e into jaegertracing:master Feb 22, 2019
@funny-falcon
Copy link
Contributor Author

Oh, I was just writting that it was wrong change :-(
I've missed that cqlsh accepts environment variable CQLSH_HOST.

My thoughts:

  • default CQLSH_HOST is "cassandra"
  • default CQLSH_HOST were not exported
  • previously default CQLSH_HOST were not passed to final command, but it were passed to check command.

Therefore:

  • if CQLSH_HOST were defined from outside (as a docker -e CQLSH_HOST argument), it worked correctly in both places (because it were passed as an argument to check command, and passed as a environment variable to final command)
  • if CQLSH_HOST were not defined from outside, check command accepted default value "cassandra" as a host argument, and, probably, it failed, making whole script fail.

Therefore, correct change should be removing both usage of CQLSH_HOST as arguments.

I sorry that I sent this pull request without enough investigation.

@jpkrohling
Copy link
Contributor

I kinda wondered whether cqlsh shouldn't read CQLSH_HOST, but given that we have a similar usage a few lines above (line 18), I think this is appropriate.

The default value cassandra is consistent with the docs we have, but it's really intended to be set by users, as there's no right default value we could use.

konradgaluszka pushed a commit to SabreOSS/jaeger that referenced this pull request Feb 26, 2019
annanay25 pushed a commit to annanay25/jaeger that referenced this pull request Mar 1, 2019
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