Differentiate between server error messages and client error messages in kubectl

[brendandburns]

Closes #3937

[brendandburns]

ok, fixed the test, so much for refactoring on the fly while I'm in there...

[erictune]

@ghodss I can't assign this to you, but are you a good person to review this?

[j3ffml]

Reviewing.

[j3ffml]

I recall a few people (@smarterclayton and others) saying this pattern is bad and we should discontinue it. I'm personally fine with the pattern if it actually does error parsing work instead of just wrapping logfatal.

That being said, there is a duplicate declaration of checkErr in pkg/cmd/util/helpers.go (mea culpa; I was lazy and didn't want to sed across all occurrences). IMO, it should just be declared in pkg/cmd/util. Please add a TODO to move it there, and I will do so after merging this.

[smarterclayton]

We're really trying to nuke this because it makes commands untestable in units. Making it public is an implicit endorsement...

On Mar 4, 2015, at 2:15 PM, Jeff Lowdermilk notifications@github.com wrote:

In pkg/kubectl/cmd/cmd.go:

@@ -331,7 +332,16 @@ func DefaultClientConfig(flags *pflag.FlagSet) clientcmd.ClientConfig {

func checkErr(err error) {
I recall a few people, (@smarterclayton and others) saying this pattern is bad and we should discontinue it. I'm personally fine with the pattern if it actually does error parsing work instead of just wrapping logfatal.

That being said, there is a duplicate declaration of checkErr in pkg/cmd/util/helpers.go (mea culpa; I was lazy and didn't want to sed across all occurrences). IMO, it should just be declared in pkg/cmd/util. Please add a TODO to move it there, and I will do so after merging this.

—
Reply to this email directly or view it on GitHub.

[ghodss]

The abstraction of functionality to handle an error is not a problem - the problem is calling this functionality from all the various functions from which we call it. I added some comments on #2146 (the issue that I think you're referring to) to clarify. In this case we're abstracting out the program printing a specific kind of string and then fataling on error. As long as that's the same functionality that the util function provides, then it's okay to merge them, but I don't think having two of these functions is intrinsically an issue. Probably the naming could be improved... fatalOnError would be better.

[ghodss]

Another reason it may not make sense to merge them is that the messages added here are specifically designed to be a kubectl enhancement - see #3937. They wouldn't necessarily make sense outside kubectl context.

[smarterclayton]

Go has a pattern for handling errors. We should be using it... The command execution functions not returning an error is a quirk of commander, not a behavior we should be mimicking.

----- Original Message -----

@@ -331,7 +332,16 @@ func DefaultClientConfig(flags *pflag.FlagSet)
clientcmd.ClientConfig {

func checkErr(err error) {

The abstraction of functionality to handle an error is not a problem - the
problem is calling this functionality from all the various functions from
which we call it. I added some comments on #2146 (the issue that I think
you're referring to) to clarify. In this case we're abstracting out the
program printing a specific kind of string and then fataling on error. As
long as that's the same functionality that the util function provides, then
it's okay to merge them, but I don't think having two of these functions is
intrinsically an issue. Probably the naming could be improved...
fatalOnError would be better.

---

Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/5016/files#r25804575

[j3ffml]

@smarterclayton, apologies if I'm being dense, but what is the pattern you are suggesting we use here? Log error and then return? Or are you suggesting that we fork/upstream commander's Run function to return an error and push error handling into the command framework?

[smarterclayton]

Example:

func (f *Factory) NewCmdGet(out io.Writer) *cobra.Command {
    cmd := &cobra.Command{
        Use:     "get [(-o|--output=)json|yaml|template|...] <resource> [<id>]",
        Short:   "Display one or many resources",
        Long:    get_long,
        Example: get_example,
        Run: func(cmd *cobra.Command, args []string) {
            RunGet(f, out, cmd, args)
        },
    }
        // ...
    return cmd
}

Should probably be

func (f *Factory) NewCmdGet(out io.Writer) *cobra.Command {
    cmd := &cobra.Command{
        Use:     "get [(-o|--output=)json|yaml|template|...] <resource> [<id>]",
        Short:   "Display one or many resources",
        Long:    get_long,
        Example: get_example,
        Run: func(cmd *cobra.Command, args []string) {
            err := RunGet(f, out, cmd, args)
            checkErr(err)
        },
    }
        // ...
    return cmd
}

There's no reason our code has to run inside of the Run() method. That would then allow us to a) unit test RunGet (which we need to be able to do) and b) centralize all exit handling.

----- Original Message -----

@@ -331,7 +332,16 @@ func DefaultClientConfig(flags *pflag.FlagSet)
clientcmd.ClientConfig {

func checkErr(err error) {

@smarterclayton, apologies if I'm being dense, but what is the pattern you
are suggesting we use here? Log error and then return? Or are you suggesting
that we fork/upstream commander's Run function to return an error and push
error handling into the command framework?

---

Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/5016/files#r25807240

[j3ffml]

sgtm. The scope of this PR is addressing the sever-vs-client error origin (#3937). Are you ok with merging this as is and opening a separate issue for the (mostly mindless) task of converting all commands to use the above error handling pattern?

[smarterclayton]

Its already covered in #2146

----- Original Message -----

@@ -331,7 +332,16 @@ func DefaultClientConfig(flags *pflag.FlagSet)
clientcmd.ClientConfig {

func checkErr(err error) {

sgtm. The scope of this PR is addressing the sever-vs-client error origin
(#3937). Are you ok with merging this as is and opening a separate issue for
the (mostly mindless) task of converting all commands to use the above error
handling pattern?

---

Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/5016/files#r25819463

[j3ffml]

lgtm. I'm happy to merge this as is and pick up the follow up task to fix error returning for all commands (#2146, comment). Will wait on lgtm from @smarterclayton before merging.

[smarterclayton]

LGTM