Skip to content
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

helm: allow configuration of zap flags via helm values #385

Merged
merged 4 commits into from
Aug 12, 2024
Merged

Conversation

hown3d
Copy link
Contributor

@hown3d hown3d commented Aug 8, 2024

Motivation

Currently there is no way to configure the logging config of the yawol controllers.
This PR adds support to pass this configuration via helm values.

New helm values

Key Type Default Description
logging object {"encoding":"console","level":"info","stacktraceLevel":"error"} values are passed as zap-flags to the containers. See https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.18.4/pkg/log/zap#Options.BindFlags for more information
logging.encoding string "console" log encoding (one of 'json' or 'console')
logging.level string "info" Level to configure the verbosity of logging. Can be one of 'debug', 'info', 'error' or any integer value > 0 which corresponds to custom debug levels of increasing verbosity
logging.stacktraceLevel string "error" level at and above which stacktraces are captured (one of 'info', 'error' or 'panic')

Copy link
Member

@Kumm-Kai Kumm-Kai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! One small nit :)

charts/yawol-controller/values.yaml Outdated Show resolved Hide resolved
@Kumm-Kai Kumm-Kai self-assigned this Aug 9, 2024
@nschad
Copy link

nschad commented Aug 12, 2024

I would have added a generic block extraArgs where the user can specify what he needs. This seems unflexible and needs adoption everytime a flag changes or a new one is added

Example: https://github.com/cortexproject/cortex-helm-chart/blob/master/templates/alertmanager/alertmanager-dep.yaml#L106-L108

@hown3d
Copy link
Contributor Author

hown3d commented Aug 12, 2024

I would have added a generic block extraArgs where the user can specify what he needs. This seems unflexible and needs adoption everytime a flag changes or a new one is added

Example: cortexproject/cortex-helm-chart@master/templates/alertmanager/alertmanager-dep.yaml#L106-L108

I agree in general to add extraArgs as a value but having only extraArgs removes the abstraction of the values completely. I don't want to check if the application beneath uses zap as logging framework or however I can configure logging, I just want to configure it easily as a user.

Lukas Hoehl added 4 commits August 12, 2024 11:25
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.de>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.de>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.de>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.de>
@Kumm-Kai Kumm-Kai merged commit cc7bcfc into main Aug 12, 2024
2 checks passed
@Kumm-Kai Kumm-Kai deleted the log-flags branch August 12, 2024 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants