Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(helm): Helm template for Celery beat (for reporting and alerting) #13116
feat(helm): Helm template for Celery beat (for reporting and alerting) #13116
Changes from 4 commits
603959c
a9e3962
9cd0d8c
2bf6548
ed4b43d
ed2805a
2d1f719
9750732
ff6653a
327573d
cb18a2d
29412d0
a764989
6febd34
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 initially named the pods the same way you did, but some colleague said that
beat
is kind of generic, and it would be better naming it{{ template "superset.fullname" . }}-celerybeat
.I let you judge 😉
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.
Yeah that makes sense...
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.
[K8S newbie] What's the point of those annotations?
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.
Since those values will change on any content updates of the source values, this will force a patch of the
Deployment
, and a restart of the pods whenever we publish an update to these (otherwise, updates to aConfigMap
does not automatically force the pods mounting them to restart).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.
There is no trace of flower in the
master
chart in fact 😮 To add it, we should do something really similar to this PR, plus a service dedicated to flower.I think this is out of the scope of this PR, my team will switch back to the
master
charts, and we will open PR to add what's missing, but not sure when this will be. 😕If you have some time to do it, I would gladly help!
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.
Agreed, different PR... and probably not high prio...
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.
Agree, but flower by default does not support auth, and it exposes dangerous functionality and sensitive info. May be a bit out of scope