-
Notifications
You must be signed in to change notification settings - Fork 482
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
Copy tempo-query binary to /tempo-query #2453
Conversation
- previous implementation forces tempo to run with root privileges in Kubernetes because it is also writing to files in /tmp directory - this solution is inline to how other tempo components are built and ensures /tmp dir exists with mkdir -p
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 👍🏼
btw, tempo-query is an optional component and we sent a PR to disable it by default in helm chars
Co-authored-by: Suraj Nath <9503187+electron0zero@users.noreply.github.com>
Thanks for feedback. I updated comment according to suggestion |
|
||
# This is silly, but it's important that tempo-query gets copied into /tmp |
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 didn't like my cheeky comment ;) ?
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.
oh, @electron0zero suggested that!
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.
Haha I personally liked it and that’s why I initially kept that part in there 😄
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 was okay with either but wanted to sound like we know what we are doing 😆
What this PR does:
Updates the tempo-query Dockerfile such that it doesn’t copy
tempo-query
binary to /tmp dir but instead to root dir, i.e /tempo-query.Context:
Which issue(s) this PR fixes:
Fixes grafana/helm-charts#2024
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]