-
Notifications
You must be signed in to change notification settings - Fork 327
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
feat(cli): kumactl install tracing #655
Conversation
return strings.HasSuffix(file.Name, ".yaml") | ||
}) | ||
|
||
renderedFiles, err := renderFiles(yamlTemplateFiles, templateArgs, simpleTemplateRenderer) |
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.
You should be able to just pass templateFiles
instead of yamlTemplateFiles
because all files for tracing are YAML files
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 used the existing code in install_metrics.go
that also uses yamlTemplateFiles
. Should it change in both?
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.
It's needed for metrics because we also have .json files, so we have to filter those.
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.
Done
- env: | ||
- name: COLLECTOR_ZIPKIN_HTTP_PORT | ||
value: "9411" | ||
image: jaegertracing/all-in-one |
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.
Is it a good idea to leave here the latest version? What if Jaeger releases a new version and this YAML is not compatible with it? I think it's more reliable to lock the version here. Of course this comes at the cost of updating the version every now and then, but then we are sure that we are providing the command that works.
We locked the version in metrics Prometheus + Grafana deployment.
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.
Done.
@@ -0,0 +1,163 @@ | |||
# |
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.
Can you maybe also add a comment with a link on which this file is based?
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.
Done.
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.
Please add a CHANGELOG.md entry and it's ready to merge
Adding
kumactl install tracing
to automatically provision Jaeger with Zipkin compatibility in akuma-tracing
namespace.Goes in hand with kumahq/kuma-website#167