Add source in “error from server” message when using kubectl #10221
Conversation
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
| @@ -97,11 +97,11 @@ func RunCreate(f *cmdutil.Factory, out io.Writer, filenames util.StringList) err | |||
| err = r.Visit(func(info *resource.Info) error { | |||
| data, err := info.Mapping.Codec.Encode(info.Object) | |||
| if err != nil { | |||
| return err | |||
| return fmt.Errorf("error when creating %q: %v ", info.Source, err) | |||
There was a problem hiding this comment.
If I am not wrong, this should be an api/error (https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/api/errors/errors.go#L91).
There was a problem hiding this comment.
Shouldn't be necessary. The error returned here gets handled in CheckErr.
There was a problem hiding this comment.
It's not clear to me who calls this and whether or not it's important for these errors to be introspectable.
There was a problem hiding this comment.
Well-- this is a public method, so anyone could call it, no? CheckErr isn't guaranteed to be the thing receiving the error?
There was a problem hiding this comment.
Technically anyone could call it, but in practice it's only ever called in NewCmdCreate and in tests. Come to think of it, all the Run* functions should actually be private.
There was a problem hiding this comment.
Well, it's kinda handy to be able to call kubectl functions from e.g. tests, so let's think carefully before we do that... :)
|
@k8s-bot ok to test |
j3ffml
added
the
sig/api-machinery
|
LGTM. Seems like strict improvement to usability. |
| @@ -65,6 +65,7 @@ type Info struct { | |||
| Mapping *meta.RESTMapping | |||
| Namespace string | |||
| Name string | |||
| Source string | |||
There was a problem hiding this comment.
Please add a comment explaining what "Source" means. Is this the name of a .go file? a .json/.yaml file?
|
GCE e2e build/test passed for commit 201c2e7e7322122ffeabf82ba3d8c6e2da5cfe80. |
feihujiang
force-pushed
the
addSourceForErrorFromServerMessage
branch
2 times, most recently
from
a1d9447
to
41f8830
Compare
|
GCE e2e build/test failed for commit a1d94473561249d4310bd9433686c1c0b81a1643. |
|
GCE e2e build/test passed for commit 41f883011949e7e361daa9d058824bbf9c166daf. |
|
LGTM too |
| @@ -66,6 +66,9 @@ type Info struct { | |||
| Namespace string | |||
| Name string | |||
|
|
|||
| //Optional, Source is the filename or URL to template file (.json or .yaml), | |||
| //or stdin to use to handle the resource | |||
There was a problem hiding this comment.
Nit: A single space after //, please.
feihujiang
force-pushed
the
addSourceForErrorFromServerMessage
branch
from
41f8830
to
54fb8c6
Compare
feihujiang
force-pushed
the
addSourceForErrorFromServerMessage
branch
from
54fb8c6
to
b41b531
Compare
|
@lavalamp I updated according to your advice, Could you please review this PR again? |
|
LGTM |
|
@dchen1107 for ok-to-merge? |
|
LGTM |
…Message Add source in “error from server” message when using kubectl
#10212