-
Notifications
You must be signed in to change notification settings - Fork 707
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
Remove default for daemonset/deployment in eck-beats & eck-agent Helm Charts #6621
Remove default for daemonset/deployment in eck-beats & eck-agent Helm Charts #6621
Conversation
… Helm Chart. Signed-off-by: Michael Montgomery <mmontg1@gmail.com>
deploy/eck-beats/lint-values.yaml
Outdated
# until https://github.com/helm/helm/pull/11440 | ||
# is merged | ||
spec: | ||
daemonSet: {} |
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.
Do we need this or change how we run helm lint
?
Because running the linter without it doesn't fail there is just an extra INFO level log.
> helm lint --strict .; echo $?
engine.go:176: [INFO] Missing required value: A Beat type is required
engine.go:189: [INFO] Fail: At least one of daemonSet or deployment is required for a functional Beat
==> Linting .
1 chart(s) linted, 0 chart(s) failed
0
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.
@thbkrkr I swore mine was failing without this in place.. I'll verify/
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 the Fail
confused me, I think, and I didn't check the return value. I've removed this.
deploy/eck-beats/values.yaml
Outdated
# ref: https://www.elastic.co/guide/en/cloud-on-k8s/current/k8s-beat-configuration.html#k8s-beat-chose-the-deployment-model | ||
# | ||
daemonSet: {} | ||
# daemonSet: {} | ||
# 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.
It's not from your PR, but we are not consistent with the daemonSet/deployment defaults for Beats and Agent.
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 believe I made them look consistent in the values @thbkrkr lmk.
Make daemonset/deployment values consistent. Signed-off-by: Michael Montgomery <mmontg1@gmail.com>
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.
👍
@elasticmachine run elasticsearch-ci/docs |
resolves #6330
Since helm/helm#11440 is still open/unresolved, we really need to not set a default deployment/daemonset for both eck-agent and eck-beats helm charts as they simply cannot be removed when using a "parent" helm chart pattern, such as in
eck-stack
.open question, should we do the same for eck-fleet-server, since that deployment mechanism is typically a deployment?