-
Notifications
You must be signed in to change notification settings - Fork 344
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 OTEL config to Jaeger CR #1056
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1056 +/- ##
==========================================
- Coverage 64.61% 64.51% -0.10%
==========================================
Files 84 85 +1
Lines 6721 6860 +139
==========================================
+ Hits 4343 4426 +83
- Misses 2236 2277 +41
- Partials 142 157 +15
Continue to review full report at Codecov.
|
9a3364a
to
d8e1b55
Compare
expected: false, | ||
opts: v1.NewOptions(map[string]interface{}{"config": "/etc/config.yaml"}), | ||
}, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another like this, but with config
- and expected: false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what you mean. I have added some case, I think there are all the combinations now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@objectiser could you please have a look again.
I have added the volumes and volume mounts to the agent from the common spec. Alternatively, we can only add the volumes and mounts for the otelconfig.
pkg/inject/sidecar.go
Outdated
// ensure we have a consistent order of the arguments | ||
// see https://github.com/jaegertracing/jaeger-operator/issues/334 | ||
sort.Strings(args) | ||
|
||
dep.Spec.Template.Spec.Volumes = append(dep.Spec.Template.Spec.Volumes, commonSpec.Volumes...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to add volumes to the pod spec and mounts to agent sidecar container
}}, | ||
Volumes: commonSpec.Volumes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as for sidecar, I had to add volumes and mounts to the agent container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - although e2e test failure needs checking.
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
f27f88e
to
e4755b0
Compare
All tests passed, the coverage is red, however when I dig in into codecov UI I cannot find the uncovered lines. |
@pavolloffay |
It's OTEL config you can find it in https://github.com/open-telemetry/opentelemetry-collector |
Resolves #1004
Notes:
A simple CR to test this PR. The collector should successfully deploy and it should use 14269 as HC port.