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

Support injection of JAEGER_SERVICE_NAME based on app or k8s recommen… #362

Merged
merged 2 commits into from
Apr 5, 2019
Merged

Conversation

objectiser
Copy link
Contributor

…ded labels

Order of precedence is app.kubernetes.io/instance, if not found then, app.kubernetes.io/name and finally app (as now).

Fixes #352

Signed-off-by: Gary Brown gary@brownuk.com

…ded labels

Signed-off-by: Gary Brown <gary@brownuk.com>
@codecov
Copy link

codecov bot commented Apr 4, 2019

Codecov Report

Merging #362 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #362      +/-   ##
==========================================
+ Coverage   89.94%   89.96%   +0.01%     
==========================================
  Files          64       64              
  Lines        3054     3059       +5     
==========================================
+ Hits         2747     2752       +5     
  Misses        207      207              
  Partials      100      100
Impacted Files Coverage Δ
pkg/inject/sidecar.go 100% <100%> (ø) ⬆️

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 7f314b5...126aefd. Read the comment docs.

@@ -58,6 +58,39 @@ func TestInjectSidecarWithEnvVars(t *testing.T) {
assert.Equal(t, "jaeger,b3", dep.Spec.Template.Spec.Containers[0].Env[1].Value)
}

func TestInjectSidecarWithEnvVarsK8sAppName(t *testing.T) {
jaeger := v1.NewJaeger("TestInjectSidecarWithEnvVars")
Copy link
Contributor

Choose a reason for hiding this comment

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

For the future, we could replace the instance name in tests with a random value, as this is something we always forget to change after a copy/paste... Or just have them all have the same value. The purpose of having a unique name per test is to avoid clashes in parallel tests, but we don't have such a scenario today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm usually the one that falls into this trap :(

Copy link
Contributor

Choose a reason for hiding this comment

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

I do it all the time as well.

assert.Equal(t, envVarServiceName, dep.Spec.Template.Spec.Containers[0].Env[0].Name)
assert.Equal(t, "testapp.default", dep.Spec.Template.Spec.Containers[0].Env[0].Value)
assert.Equal(t, envVarPropagation, dep.Spec.Template.Spec.Containers[0].Env[1].Name)
assert.Equal(t, "jaeger,b3", dep.Spec.Template.Spec.Containers[0].Env[1].Value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove the assertions that are not relevant for the test?

Signed-off-by: Gary Brown <gary@brownuk.com>
@jpkrohling jpkrohling merged commit 922d562 into jaegertracing:master Apr 5, 2019
@objectiser objectiser deleted the applabel branch April 5, 2019 09:36
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.

2 participants