-
Notifications
You must be signed in to change notification settings - Fork 217
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
pkg/assets/create: Replace Verbose and StdErr with Logger #225
Conversation
To allow consumers to plug in their preferred logger instead of requring an output stream. I'm using go-log/log [1] as an unopinionated interface, which callers can feed with the stdlib's log package, logrus, etc. depending on their logging preferences. [1]: https://github.com/go-log/log
Generated with: $ alpha-build-machinery/scripts/update-deps.sh Also bring in stretchr/testify/assert for convenient test assertion. The the bumps to the other libraries are because Glide.yaml is floating them at master or their indirect dependencies.
/retest |
We should not import new package here, instead we can create the interface here and plug whatever we need there (the interfaces are exchangeable). I would just copy https://github.com/go-log/log/blob/master/log.go#L5 at the top of the create.go |
@@ -53,8 +48,8 @@ func EnsureManifestsCreated(ctx context.Context, manifestDir string, restConfig | |||
return err | |||
} | |||
|
|||
if options.Verbose && options.StdErr == nil { | |||
options.StdErr = os.Stderr | |||
if options.Logger == nil { |
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.
If the options.Logger is not set, just don't log anything or create some dummy implementation of Logger interface?
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.
If the options.Logger is not set, just don't log anything or create some dummy implementation of Logger interface?
The default I'm plugging in in this block is a no-op logger unless the user set that package-level global.
/hold The whole org uses glog for very clear reason, because the whole k8s apimachinery and client-go is tightly coupled to it. Using more then 1 logger is a lost of time and will not work given the hardcoded glog in k8s. You just don't use 2 loggers in 1 project. |
/cc @deads2k |
Decoupling the code from glog is fine, but not by pulling in another package. Let's just abstract with a local interface and every consumer can plug a wrapper in for his/her favourite logger. |
Making more loggers configurable is work that could have been better spent on important things, same goes for reviews. In the end the same statement applies for logging in the end component, if it doesn't use glog. (note: I personally don't like glog at all, but I understand the necessity of using it implied by being hardcoded in k8s.) |
The installer inherited logrus. I don't see it consuming this code directly, but I also don't see a need for opinionated logging in libraries. However, I can reroll to take glog (or klog?) types if folks don't like the interface approach.
Why copy/paste this (and the no-op implementation), when they're already written and can be vendored? Seems easier to maintain that way, and if your vendor tooling pruned unused packages it would be about the same amount of code either way. |
Also, lots of Kubernetes library log handling discussion in kubernetes/kubernetes#61006, where interface-based approaches were raised but not accepted (although I'm still not clear on why not). |
Klog will be brought up with 1.13 kubernetes, so I wouldn't bother with that, yet.
That's one of the reasons klog happened, it might be a road towards just that, we'll see. |
"sort" | ||
"strings" | ||
"time" | ||
|
||
"github.com/ghodss/yaml" | ||
"github.com/go-log/log" |
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.
no new logging dep. Project standard is glog and we'll switch to klog with kube.
if options.Verbose { | ||
fmt.Fprintf(options.StdErr, "[#%d] failed to fetch discovery: %s\n", retryCount, err) | ||
} | ||
options.Logger.Logf("[#%d] failed to fetch discovery: %s", retryCount, err) |
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.
project standard upstream and downstream is fmt.printf for CLI and glog for everything else. The previous code is in keeping.
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.
project standard upstream and downstream is fmt.printf for CLI and glog for everything else. The previous code is in keeping.
But this is library code, so I'd expect glog over fmt, right?
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.
@wking depends on the caller of this code. So far it is cluster-bootstrap which is a command we run during the bootstrap (so it is CLI ;-). I'm fine switching this to glog, was trying to avoid importing glog because I know that some components/cli have problem with parsing glog flags (so wanted to avoid that if not necessary).
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.
depends on the caller of this code. So far it is cluster-bootstrap which is a command we run during the bootstrap (so it is CLI ;-).
I don't think code in library-go should have API choices hinge on "we know all of our consumers, and they're not libraries themselves". Is there a reason that approach is not as brittle as it seems?
I'm fine switching this to glog, was trying to avoid importing glog because I know that some components/cli have problem with parsing glog flags (so wanted to avoid that if not necessary).
Like openshift/installer#1181. This is exactly why I prefer logging interfaces, but it sounds like that's a no-go?
/hold |
Hrm. I went to reroll this to use glog as requested by @tnozicka here @deads2k here. But I'm stuck with collecting the logs for unit tests. Should I drop those tests? Should I have |
@wking: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@wking: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
@openshift-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
To allow consumers to plug in their preferred logger instead of requring an output stream. I'm using go-log/log as an unopinionated interface, which callers can feed with the stdlib's log package, logrus, etc. depending on their logging preferences.
CC @mfojtik, @sttts