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

Add CQLSH_PORT environment variable #1243

Merged
merged 1 commit into from
Oct 12, 2020

Conversation

Ashmita152
Copy link
Contributor

Signed-off-by: Ashmita Bohara [email protected]

Since jaegertracing/jaeger#2472 is merged, adding support for custom port here.

Partially Fixes: #1179

@codecov
Copy link

codecov bot commented Oct 11, 2020

Codecov Report

Merging #1243 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1243      +/-   ##
==========================================
+ Coverage   87.34%   87.36%   +0.01%     
==========================================
  Files          90       90              
  Lines        4900     4907       +7     
==========================================
+ Hits         4280     4287       +7     
  Misses        457      457              
  Partials      163      163              
Impacted Files Coverage Δ
pkg/storage/cassandra_dependencies.go 100.00% <100.00%> (ø)

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 729d8aa...389203b. Read the comment docs.

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Looks good to me! A small improvement could be done in the test, but if you disagree with the proposed change, I'll merge it as is.

foundValue := ""
for _, e := range b[0].Spec.Template.Spec.Containers[0].Env {
if e.Name == "CQLSH_PORT" {
foundValue = e.Value
Copy link
Contributor

Choose a reason for hiding this comment

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

Small improvement: you could make an assertion here: assert.Equal(t, "9042", ...) and then return. Where your current assert.Equal is (at the end of the test), you could have a assert.Fail(t, "value for CQLSH_PORT not found")

for _, e := range b[0].Spec.Template.Spec.Containers[0].Env {
if e.Name == "CQLSH_PORT" {
foundValue := e.Value
assert.Equal(t, "9042", foundValue, "unexpected CQLSH_PORT environment var value")
Copy link
Contributor

Choose a reason for hiding this comment

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

It's missing the return statement, so that the test is now failing :-) You might also not need the foundValue anymore: you can use e.Value directly in the assertion.

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Once the return statement is added, the test should pass and it'll be ready to be merged.

@jpkrohling
Copy link
Contributor

LGTM. Will be merged on green. Thank you for your contribution!

@Ashmita152
Copy link
Contributor Author

Thank you for your quick feedbacks @jpkrohling It was my pleasure contributing.

@mergify mergify bot merged commit c69e666 into jaegertracing:master Oct 12, 2020
@Ashmita152 Ashmita152 deleted the custom-port branch October 12, 2020 09:18
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.

Configure external Cassandra instance on cloud platform
2 participants