Improve error output of kubectl update #9803
Conversation
satnam6502
force-pushed
the
execessive
branch
2 times, most recently
from
0638563
to
e0e4d03
Compare
|
GCE e2e build/test failed for commit e0e4d035f97a105d7e9bfe71b2dc2e027e3412d1. |
|
GCE e2e build/test passed for commit 14052c06f63a0e8a09edfad724a2e958311c220c. |
| // a sperate field ends up being concatentated into one string which contains | ||
| // the spec and the detail string. To avoid significant refactoring of the error | ||
| // data structures we just extract the required detail string by looking for it | ||
| // after "}': " which is horrible but expedient. |
There was a problem hiding this comment.
TODO: We should create an error struct with information embedded in fields, not in a string.
(or touch on how this should be fixed up after v1)
|
I'm okay with this if it's important to get a quick fix in for v1 (but please add a TODO to cleanup). What I don't like:
|
|
Indeed, the objective is to do a quick fix because right now because I think improving the user experience is fairly important and I think many users are likely to encounter this error message. I will add a TODO here but I am not sure if this is really where the TODO belongs since it's not this code that we mostly need to return later to fix up -- it's the error types that need to be worked and and that is a pretty big change (which we could do now -- but the effort/reward does not seem worth it right now). |
|
Sure. Don't worry about the TODO. Maybe track what we want in an issue? |
|
LGTM |
mikedanese
added
the
lgtm
|
@k8s-bot ok to test |
|
@satnam6502 can you rebase and push again, and see if that kicks k8s-bot |
|
GCE e2e build/test passed for commit e0e4d035f97a105d7e9bfe71b2dc2e027e3412d1. |
|
Rebased, pushed. |
|
GCE e2e build/test passed for commit 445f1113f37d048828f3adcb00999b4f9220a480. |
|
GCE e2e build/test failed for commit d69e0b5. |
|
@k8s-bot retest this please |
|
GCE e2e build/test passed for commit d69e0b5. |
Improve error output of kubectl update
This PR addresses issue #9721 to improve theerror output of
kubectl update.NewCmdUpdateis changed to used a modified version ofCheckErrcalledCheckCustomErrwhich can accept a custom error prefix message. Unfortunately, the text segment we want to report ("may not update fields other than container.image") which is originally stored in a separate field is unfortunately conflated with a larger text message that includes details of the spec being updated. To avoid making significant changes to the error types used in the code base I decided to do the horrible but expedient thing: extract out the required string by looking for text after "}': ". Now the error message is:which is exactly what @erictune requested.