-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
OSDK->KB: Logging Flags #885
Comments
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle stale |
Description: It is basically to get [0] into controller-runtime. Haseeb has a PR that would make this simpler [1]. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle stale |
A pattern I really like about log levels is choosing a default log level (usually For example, if the default level is
I already have this pattern implemented in several projects so I can contribute it if you like it. |
Hi @Adirio, Sure. Feel free to contribute. It shows great 👍 . Just beware that we need to is as described #885 (comment) |
Do we need to use the flag names and behavior in that file? Or do we just need to provide all the functionality of those flags? For example, kubebuilder does a good job not showing which logging framework it uses, but the flag names mention |
Hi @Adirio, Note that we need to merge the PR #1116 first.
Good question. The reason this task is the integration with SDK. So, I would say that we should keep the same names. However, I think that I would also prefer the common usage as |
The following list describes the flags in Operator SDK and how they can be implemented with controller-runtime@0.5.0:
My suggestion would be:
|
https://github.com/kubernetes-sigs/controller-runtime/pull/767/files this PR has merged will the current flags being:
I believe that these names are important to have the I think we can add a follow-up's to:
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle stale |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle rotten |
Given that kubernetes-sigs/controller-runtime#767 is merged, what more is remaining on this? Is it just this?
|
Hi @StevenACoffman, We need to address the comment #885 (comment) in https://github.com/kubernetes-sigs/controller-runtime/blob/0e1344145ac34e036199f4d5abc8c71b59b7d7e5/pkg/log/zap/zap.go#L237 and the flags that you raise as well. Also, see that controller-runtime has the issue kubernetes-sigs/controller-runtime#1035. IHMO we ough to track this need in controller-runtime and work on it there and then close this issue with the PR #1721. WDYT @joelanford ? @StevenACoffman, Would you like to help with this one? |
So, I think we can close this one with the #1721 and then, if we need we can raise a new issue with a scope more defined to know what exactly still missing and should get done in KB. However, please feel free also to re-open if you think that it is required. |
Optional utilities for adding flags to customize log level.
/kind feature
The text was updated successfully, but these errors were encountered: