-
Notifications
You must be signed in to change notification settings - Fork 382
Conversation
@carolynvs: GitHub didn't allow me to request PR reviews from the following users: jberkhahn. Note that only kubernetes-incubator members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
// Enabling all flags causes unwanted deprecation warnings from glog to always print in plugin mode | ||
pflag.CommandLine.AddGoFlag(flag.CommandLine.Lookup("v")) | ||
pflag.CommandLine.AddGoFlag(flag.CommandLine.Lookup("logtostderr")) | ||
pflag.CommandLine.Set("logtostderr", "true") |
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.
Uh, doesn't this force it to log to std err always? Do we not allow redirection of that?
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.
Without that flag, I don't see any verbose output. Redirection seems to work fine though
$ kubectl plugin svcat get brokers -v=7 2> debug.log
NAME URL STATUS
+------------+--------------------------------------------------------------+--------+
osba http://osba-open-service-broker-azure.osba.svc.cluster.local Ready
ups-broker http://ups-broker-ups-broker.ups-broker.svc.cluster.local Ready
$ cat debug.log
I0308 12:18:48.735569 27194 loader.go:357] Config loaded from file /Users/carolynvs/.kube/config
I0308 12:18:48.792845 27195 loader.go:357] Config loaded from file /Users/carolynvs/.kube/config
I0308 12:18:48.793937 27195 round_trippers.go:414] GET https://192.168.99.100:8443/apis/servicecatalog.k8s.io/v1beta1/clusterservicebrokers
I0308 12:18:48.793955 27195 round_trippers.go:421] Request Headers:
I0308 12:18:48.793965 27195 round_trippers.go:424] Accept: application/json, */*
I0308 12:18:48.793974 27195 round_trippers.go:424] User-Agent: svcat/v0.0.0 (darwin/amd64) kubernetes/$Format
I0308 12:18:48.806120 27195 round_trippers.go:439] Response Status: 200 OK in 12 milliseconds
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 this is a cli, and that flag says "log to the console instead of a file", it seems appropriate to have it always on.
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.
Huh, my confusion was that it seems like you're adding a flag to control it in line 6, but then always setting it to true on line 7. If it's always on why add the flag on line 6?
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.
Lines 6 adds the flag to the flag set so that it is defined, has helptext, etc. Line 7 sets it to true. Glog directly reads the flag value, logtostderr isn't something that I can directly control myself, so I have to muck with the flag...
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.
so does this line force glog to log warnings to stderr? I'm not familiar enough with pflag or glog to know this offhand, so I'm hoping you can save me the research 😄
2555dc0
to
4f6c291
Compare
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.
Just a question in here. Your output shows this works, so LGTM
// Enabling all flags causes unwanted deprecation warnings from glog to always print in plugin mode | ||
pflag.CommandLine.AddGoFlag(flag.CommandLine.Lookup("v")) | ||
pflag.CommandLine.AddGoFlag(flag.CommandLine.Lookup("logtostderr")) | ||
pflag.CommandLine.Set("logtostderr", "true") |
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.
so does this line force glog to log warnings to stderr? I'm not familiar enough with pflag or glog to know this offhand, so I'm hoping you can save me the research 😄
Github isn't letting me reply... 🤷♀️ @arschles
glog is a tirefire. By default it logs to files, not stdout/stderr. So for a cli |
LGTM after conflict merge |
4f6c291
to
11a2756
Compare
* master: Support provisioning by external id (kubernetes-retired#1756) Add namespaced ServiceBroker API (kubernetes-retired#1881) Enable verbose logging in svcat (kubernetes-retired#1822) Adding ServicePlan (namespaced plans) API (kubernetes-retired#1894) Svcat bind params now supports --params-json (kubernetes-retired#1889) words discussing the automated builds (kubernetes-retired#1885) v0.1.12 release changes don't overwrite generated files before verifying (kubernetes-retired#1891) Update registry code from broker to clusterservicebroker (kubernetes-retired#1880) Credentials remapping (kubernetes-retired#1868) Rename/move existing "ServicePlan" things (kubernetes-retired#1883) Start orphan mitigation after receiving a last operation status for async provisioning (kubernetes-retired#1886) Update of name in example ServiceClass (kubernetes-retired#1878) Cluster-id configmap (kubernetes-retired#1658) Add namespaced ServiceClass API (kubernetes-retired#1859) Fix common validations regression (kubernetes-retired#1882)
Support the
-v=LEVEL
flag in svcat, in both standalone and plugin mode./cc @jberkhahn