Merge pull request #46468 from alexandercampbell/cleanup-in-kubectl

Automatic merge from submit-queue

Cleanup pkg/kubectl

I was reading through `pkg/kubectl` in preparation for completing https://github.com/kubernetes/kubectl/issues/11 and noticed several opportunities for improvement. This should be easy to review since it's mostly mechanical changes. The only complicated changes are in `addFromEnvFile`, which I refactored into two functions and wrote tests for.

**Release note**:

```release-note
NONE
```

pkg/kubectl/BUILD

@@ -14,6 +14,7 @@ go_test(
         "cluster_test.go",
         "configmap_test.go",
         "deployment_test.go",
+        "env_file_test.go",
         "generate_test.go",
         "kubectl_test.go",
         "namespace_test.go",

pkg/kubectl/apply.go

@@ -61,11 +61,7 @@ func SetOriginalConfiguration(info *resource.Info, original []byte) error {
 	}
 
 	annots[api.LastAppliedConfigAnnotation] = string(original)
-	if err := info.Mapping.MetadataAccessor.SetAnnotations(info.Object, annots); err != nil {
+	return info.Mapping.MetadataAccessor.SetAnnotations(info.Object, annots)
-		return err
-	}
-
-	return nil
 }
 
 // GetModifiedConfiguration retrieves the modified configuration of the object.

pkg/kubectl/cmd/apply_edit_last_applied.go

@@ -18,7 +18,7 @@ package cmd
 
 import (
 	"io"
-	gruntime "runtime"
+	"runtime"
 
 	"github.com/spf13/cobra"
 
@@ -96,7 +96,8 @@ func NewCmdApplyEditLastApplied(f cmdutil.Factory, out, errOut io.Writer) *cobra
 	usage := "to use to edit the resource"
 	cmdutil.AddFilenameOptionFlags(cmd, &options.FilenameOptions, usage)
 	cmd.Flags().StringVarP(&options.Output, "output", "o", "yaml", "Output format. One of: yaml|json.")
-	cmd.Flags().BoolVar(&options.WindowsLineEndings, "windows-line-endings", gruntime.GOOS == "windows", "Use Windows line-endings (default Unix line-endings)")
+	cmd.Flags().BoolVar(&options.WindowsLineEndings, "windows-line-endings", runtime.GOOS == "windows",
+		"Defaults to the line ending native to your platform.")
 	cmdutil.AddRecordVarFlag(cmd, &options.Record)
 
 	return cmd

pkg/kubectl/cmd/apply_set_last_applied.go

@@ -118,11 +118,7 @@ func (o *SetLastAppliedOptions) Complete(f cmdutil.Factory, cmd *cobra.Command)
 	}
 
 	o.Namespace, o.EnforceNamespace, err = f.DefaultNamespace()
-	if err != nil {
+	return err
-		return err
-	}
-
-	return nil
 }
 
 func (o *SetLastAppliedOptions) Validate(f cmdutil.Factory, cmd *cobra.Command) error {
@@ -179,10 +175,7 @@ func (o *SetLastAppliedOptions) Validate(f cmdutil.Factory, cmd *cobra.Command)
 
 		return nil
 	})
-	if err != nil {
+	return err
-		return err
-	}
-	return nil
 }
 
 func (o *SetLastAppliedOptions) RunSetLastApplied(f cmdutil.Factory, cmd *cobra.Command) error {

pkg/kubectl/cmd/convert.go

@@ -158,11 +158,7 @@ func (o *ConvertOptions) Complete(f cmdutil.Factory, out io.Writer, cmd *cobra.C
 	}
 	o.encoder = f.JSONEncoder()
 	o.printer, err = f.PrinterForCommand(cmd, o.local, nil, printers.PrintOptions{})
-	if err != nil {
+	return err
-		return err
-	}
-
-	return nil
 }
 
 // RunConvert implements the generic Convert command

pkg/kubectl/cmd/cp.go

@@ -34,15 +34,15 @@ import (
 )
 
 var (
-	cp_example = templates.Examples(i18n.T(`
+	cpExample = templates.Examples(i18n.T(`
-	    # !!!Important Note!!!
+		# !!!Important Note!!!
-	    # Requires that the 'tar' binary is present in your container
+		# Requires that the 'tar' binary is present in your container
-	    # image.  If 'tar' is not present, 'kubectl cp' will fail.
+		# image.  If 'tar' is not present, 'kubectl cp' will fail.
 
-	    # Copy /tmp/foo_dir local directory to /tmp/bar_dir in a remote pod in the default namespace
+		# Copy /tmp/foo_dir local directory to /tmp/bar_dir in a remote pod in the default namespace
 		kubectl cp /tmp/foo_dir <some-pod>:/tmp/bar_dir
 
-        # Copy /tmp/foo local file to /tmp/bar in a remote pod in a specific container
+		# Copy /tmp/foo local file to /tmp/bar in a remote pod in a specific container
 		kubectl cp /tmp/foo <some-pod>:/tmp/bar -c <specific-container>
 
 		# Copy /tmp/foo local file to /tmp/bar in a remote pod in namespace <some-namespace>
@@ -52,8 +52,8 @@ var (
 		kubectl cp <some-namespace>/<some-pod>:/tmp/foo /tmp/bar`))
 
 	cpUsageStr = dedent.Dedent(`
-	    expected 'cp <file-spec-src> <file-spec-dest> [-c container]'.
+		expected 'cp <file-spec-src> <file-spec-dest> [-c container]'.
-	    <file-spec> is:
+		<file-spec> is:
 		[namespace/]pod-name:/file/path for a remote file
 		/file/path for a local file`)
 )
@@ -64,7 +64,7 @@ func NewCmdCp(f cmdutil.Factory, cmdOut, cmdErr io.Writer) *cobra.Command {
 		Use:     "cp <file-spec-src> <file-spec-dest>",
 		Short:   i18n.T("Copy files and directories to and from containers."),
 		Long:    "Copy files and directories to and from containers.",
-		Example: cp_example,
+		Example: cpExample,
 		Run: func(cmd *cobra.Command, args []string) {
 			cmdutil.CheckErr(runCopy(f, cmd, cmdOut, cmdErr, args))
 		},

pkg/kubectl/cmd/create.go

@@ -19,7 +19,7 @@ package cmd
 import (
 	"fmt"
 	"io"
-	gruntime "runtime"
+	"runtime"
 
 	"github.com/spf13/cobra"
 
@@ -39,12 +39,12 @@ type CreateOptions struct {
 }
 
 var (
-	create_long = templates.LongDesc(i18n.T(`
+	createLong = templates.LongDesc(i18n.T(`
-		Create a resource by filename or stdin.
+		Create a resource from a file or from stdin.
 
 		JSON and YAML formats are accepted.`))
 
-	create_example = templates.Examples(i18n.T(`
+	createExample = templates.Examples(i18n.T(`
 		# Create a pod using the data in pod.json.
 		kubectl create -f ./pod.json
 
@@ -60,9 +60,9 @@ func NewCmdCreate(f cmdutil.Factory, out, errOut io.Writer) *cobra.Command {
 
 	cmd := &cobra.Command{
 		Use:     "create -f FILENAME",
-		Short:   i18n.T("Create a resource by filename or stdin"),
+		Short:   i18n.T("Create a resource from a file or from stdin."),
-		Long:    create_long,
+		Long:    createLong,
-		Example: create_example,
+		Example: createExample,
 		Run: func(cmd *cobra.Command, args []string) {
 			if cmdutil.IsFilenameEmpty(options.FilenameOptions.Filenames) {
 				defaultRunFunc := cmdutil.DefaultSubCommandRun(errOut)
@@ -80,7 +80,8 @@ func NewCmdCreate(f cmdutil.Factory, out, errOut io.Writer) *cobra.Command {
 	cmdutil.AddValidateFlags(cmd)
 	cmdutil.AddPrinterFlags(cmd)
 	cmd.Flags().BoolVar(&options.EditBeforeCreate, "edit", false, "Edit the API resource before creating")
-	cmd.Flags().Bool("windows-line-endings", gruntime.GOOS == "windows", "Only relevant if --edit=true. Use Windows line-endings (default Unix line-endings)")
+	cmd.Flags().Bool("windows-line-endings", runtime.GOOS == "windows",
+		"Only relevant if --edit=true. Defaults to the line ending native to your platform.")
 	cmdutil.AddApplyAnnotationFlags(cmd)
 	cmdutil.AddRecordFlag(cmd)
 	cmdutil.AddDryRunFlag(cmd)

pkg/kubectl/cmd/create_clusterrolebinding.go

@@ -17,7 +17,6 @@ limitations under the License.
 package cmd
 
 import (
-	"fmt"
 	"io"
 
 	"github.com/spf13/cobra"
@@ -77,7 +76,7 @@ func CreateClusterRoleBinding(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.Co
 			ServiceAccounts: cmdutil.GetFlagStringArray(cmd, "serviceaccount"),
 		}
 	default:
-		return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not supported.", generatorName))
+		return errUnsupportedGenerator(cmd, generatorName)
 	}
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
 		Name:                name,

pkg/kubectl/cmd/create_configmap.go

@@ -17,7 +17,6 @@ limitations under the License.
 package cmd
 
 import (
-	"fmt"
 	"io"
 
 	"github.com/spf13/cobra"
@@ -97,7 +96,7 @@ func CreateConfigMap(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.Command, ar
 			EnvFileSource:  cmdutil.GetFlagString(cmd, "from-env-file"),
 		}
 	default:
-		return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not supported.", generatorName))
+		return errUnsupportedGenerator(cmd, generatorName)
 	}
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
 		Name:                name,

pkg/kubectl/cmd/create_deployment.go

@@ -38,7 +38,9 @@ var (
     kubectl create deployment my-dep --image=busybox`))
 )
 
-// NewCmdCreateDeployment is a macro command to create a new deployment
+// NewCmdCreateDeployment is a macro command to create a new deployment.
+// This command is better known to users as `kubectl create deployment`.
+// Note that this command overlaps significantly with the `kubectl run` command.
 func NewCmdCreateDeployment(f cmdutil.Factory, cmdOut, cmdErr io.Writer) *cobra.Command {
 	cmd := &cobra.Command{
 		Use:     "deployment NAME --image=image [--dry-run]",
@@ -47,7 +49,7 @@ func NewCmdCreateDeployment(f cmdutil.Factory, cmdOut, cmdErr io.Writer) *cobra.
 		Long:    deploymentLong,
 		Example: deploymentExample,
 		Run: func(cmd *cobra.Command, args []string) {
-			err := CreateDeployment(f, cmdOut, cmdErr, cmd, args)
+			err := createDeployment(f, cmdOut, cmdErr, cmd, args)
 			cmdutil.CheckErr(err)
 		},
 	}
@@ -60,8 +62,7 @@ func NewCmdCreateDeployment(f cmdutil.Factory, cmdOut, cmdErr io.Writer) *cobra.
 	return cmd
 }
 
-// CreateDeployment implements the behavior to run the create deployment command
+func createDeployment(f cmdutil.Factory, cmdOut, cmdErr io.Writer, cmd *cobra.Command, args []string) error {
-func CreateDeployment(f cmdutil.Factory, cmdOut, cmdErr io.Writer, cmd *cobra.Command, args []string) error {
 	name, err := NameFromCommandArgs(cmd, args)
 	if err != nil {
 		return err
@@ -91,7 +92,7 @@ func CreateDeployment(f cmdutil.Factory, cmdOut, cmdErr io.Writer, cmd *cobra.Co
 	case cmdutil.DeploymentBasicV1Beta1GeneratorName:
 		generator = &kubectl.DeploymentBasicGeneratorV1{Name: name, Images: cmdutil.GetFlagStringSlice(cmd, "image")}
 	default:
-		return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not supported.", generatorName))
+		return errUnsupportedGenerator(cmd, generatorName)
 	}
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
 		Name:                name,

pkg/kubectl/cmd/create_deployment_test.go

@@ -80,6 +80,6 @@ func TestCreateDeploymentNoImage(t *testing.T) {
 	cmd := NewCmdCreateDeployment(f, buf, buf)
 	cmd.Flags().Set("dry-run", "true")
 	cmd.Flags().Set("output", "name")
-	err := CreateDeployment(f, buf, buf, cmd, []string{depName})
+	err := createDeployment(f, buf, buf, cmd, []string{depName})
 	assert.Error(t, err, "at least one image must be specified")
 }

pkg/kubectl/cmd/create_namespace.go

@@ -17,7 +17,6 @@ limitations under the License.
 package cmd
 
 import (
-	"fmt"
 	"io"
 
 	"github.com/spf13/cobra"
@@ -69,7 +68,7 @@ func CreateNamespace(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.Command, ar
 	case cmdutil.NamespaceV1GeneratorName:
 		generator = &kubectl.NamespaceGeneratorV1{Name: name}
 	default:
-		return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not supported.", generatorName))
+		return errUnsupportedGenerator(cmd, generatorName)
 	}
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
 		Name:                name,

pkg/kubectl/cmd/create_pdb.go

@@ -17,7 +17,6 @@ limitations under the License.
 package cmd
 
 import (
-	"fmt"
 	"io"
 
 	"github.com/spf13/cobra"
@@ -89,7 +88,7 @@ func CreatePodDisruptionBudget(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.C
 			Selector:       cmdutil.GetFlagString(cmd, "selector"),
 		}
 	default:
-		return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not supported.", generatorName))
+		return errUnsupportedGenerator(cmd, generatorName)
 	}
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
 		Name:                name,

pkg/kubectl/cmd/create_quota.go

@@ -17,7 +17,6 @@ limitations under the License.
 package cmd
 
 import (
-	"fmt"
 	"io"
 
 	"github.com/spf13/cobra"
@@ -78,7 +77,7 @@ func CreateQuota(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.Command, args [
 			Scopes: cmdutil.GetFlagString(cmd, "scopes"),
 		}
 	default:
-		return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not supported.", generatorName))
+		return errUnsupportedGenerator(cmd, generatorName)
 	}
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
 		Name:                name,

pkg/kubectl/cmd/create_rolebinding.go

@@ -17,7 +17,6 @@ limitations under the License.
 package cmd
 
 import (
-	"fmt"
 	"io"
 
 	"github.com/spf13/cobra"
@@ -78,7 +77,7 @@ func CreateRoleBinding(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.Command,
 			ServiceAccounts: cmdutil.GetFlagStringArray(cmd, "serviceaccount"),
 		}
 	default:
-		return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not supported.", generatorName))
+		return errUnsupportedGenerator(cmd, generatorName)
 	}
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
 		Name:                name,

pkg/kubectl/cmd/create_secret.go

@@ -17,7 +17,6 @@ limitations under the License.
 package cmd
 
 import (
-	"fmt"
 	"io"
 
 	"github.com/spf13/cobra"
@@ -110,7 +109,7 @@ func CreateSecretGeneric(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.Command
 			EnvFileSource:  cmdutil.GetFlagString(cmd, "from-env-file"),
 		}
 	default:
-		return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not supported.", generatorName))
+		return errUnsupportedGenerator(cmd, generatorName)
 	}
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
 		Name:                name,
@@ -191,7 +190,7 @@ func CreateSecretDockerRegistry(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.
 			Server:   cmdutil.GetFlagString(cmd, "docker-server"),
 		}
 	default:
-		return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not supported.", generatorName))
+		return errUnsupportedGenerator(cmd, generatorName)
 	}
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
 		Name:                name,
@@ -254,7 +253,7 @@ func CreateSecretTLS(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.Command, ar
 			Cert: cmdutil.GetFlagString(cmd, "cert"),
 		}
 	default:
-		return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not supported.", generatorName))
+		return errUnsupportedGenerator(cmd, generatorName)
 	}
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
 		Name:                name,

pkg/kubectl/cmd/create_service.go

@@ -17,7 +17,6 @@ limitations under the License.
 package cmd
 
 import (
-	"fmt"
 	"io"
 
 	"github.com/spf13/cobra"
@@ -83,6 +82,10 @@ func NewCmdCreateServiceClusterIP(f cmdutil.Factory, cmdOut io.Writer) *cobra.Co
 	return cmd
 }
 
+func errUnsupportedGenerator(cmd *cobra.Command, generatorName string) error {
+	return cmdutil.UsageError(cmd, "Generator %s not supported. ", generatorName)
+}
+
 // CreateServiceClusterIP implements the behavior to run the create service clusterIP command
 func CreateServiceClusterIP(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.Command, args []string) error {
 	name, err := NameFromCommandArgs(cmd, args)
@@ -99,7 +102,7 @@ func CreateServiceClusterIP(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.Comm
 			ClusterIP: cmdutil.GetFlagString(cmd, "clusterip"),
 		}
 	default:
-		return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not supported.", generatorName))
+		return errUnsupportedGenerator(cmd, generatorName)
 	}
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
 		Name:                name,
@@ -156,7 +159,7 @@ func CreateServiceNodePort(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.Comma
 			NodePort:  cmdutil.GetFlagInt(cmd, "node-port"),
 		}
 	default:
-		return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not supported.", generatorName))
+		return errUnsupportedGenerator(cmd, generatorName)
 	}
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
 		Name:                name,
@@ -211,7 +214,7 @@ func CreateServiceLoadBalancer(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.C
 			ClusterIP: "",
 		}
 	default:
-		return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not supported.", generatorName))
+		return errUnsupportedGenerator(cmd, generatorName)
 	}
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
 		Name:                name,
@@ -272,7 +275,7 @@ func CreateExternalNameService(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.C
 			ClusterIP:    "",
 		}
 	default:
-		return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not supported.", generatorName))
+		return errUnsupportedGenerator(cmd, generatorName)
 	}
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
 		Name:                name,

pkg/kubectl/cmd/create_serviceaccount.go

@@ -17,7 +17,6 @@ limitations under the License.
 package cmd
 
 import (
-	"fmt"
 	"io"
 
 	"github.com/spf13/cobra"
@@ -69,7 +68,7 @@ func CreateServiceAccount(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.Comman
 	case cmdutil.ServiceAccountV1GeneratorName:
 		generator = &kubectl.ServiceAccountGeneratorV1{Name: name}
 	default:
-		return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not supported.", generatorName))
+		return errUnsupportedGenerator(cmd, generatorName)
 	}
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
 		Name:                name,

pkg/kubectl/cmd/delete.go

@@ -327,7 +327,7 @@ func deleteResource(info *resource.Info, out io.Writer, shortOutput bool, mapper
 	return nil
 }
 
-// objectDeletionWaitInterval is the interval to wait between checks for deletion. Exposed for testing.
+// objectDeletionWaitInterval is the interval to wait between checks for deletion.
 var objectDeletionWaitInterval = time.Second
 
 // waitForObjectDeletion refreshes the object, waiting until it is deleted, a timeout is reached, or

pkg/kubectl/cmd/edit.go

@@ -19,7 +19,7 @@ package cmd
 import (
 	"fmt"
 	"io"
-	gruntime "runtime"
+	"runtime"
 
 	"github.com/spf13/cobra"
 
@@ -108,7 +108,9 @@ func NewCmdEdit(f cmdutil.Factory, out, errOut io.Writer) *cobra.Command {
 	cmdutil.AddValidateOptionFlags(cmd, &options.ValidateOptions)
 	cmd.Flags().StringVarP(&options.Output, "output", "o", "yaml", "Output format. One of: yaml|json.")
 
-	cmd.Flags().BoolVar(&options.WindowsLineEndings, "windows-line-endings", gruntime.GOOS == "windows", "Use Windows line-endings (default Unix line-endings)")
+	cmd.Flags().BoolVar(&options.WindowsLineEndings, "windows-line-endings", runtime.GOOS == "windows",
+		"Defaults to the line ending native to your platform.")
+
 	cmdutil.AddApplyAnnotationVarFlags(cmd, &options.ApplyAnnotation)
 	cmdutil.AddRecordVarFlag(cmd, &options.Record)
 	cmdutil.AddInclude3rdPartyVarFlags(cmd, &options.Include3rdParty)

pkg/kubectl/cmd/edit_test.go

@@ -116,7 +116,7 @@ func TestEdit(t *testing.T) {
 			if step.StepType != "edit" {
 				t.Fatalf("%s, step %d: expected edit step, got %s %s", name, i, req.Method, req.URL.Path)
 			}
-			if bytes.Compare(body, expectedInput) != 0 {
+			if !bytes.Equal(body, expectedInput) {
 				if updateInputFixtures {
 					// Convenience to allow recapturing the input and persisting it here
 					ioutil.WriteFile(inputFile, body, os.FileMode(0644))
@@ -139,7 +139,7 @@ func TestEdit(t *testing.T) {
 					req.Method, req.URL.Path, req.Header.Get("Content-Type"),
 				)
 			}
-			if bytes.Compare(body, expectedInput) != 0 {
+			if !bytes.Equal(body, expectedInput) {
 				if updateInputFixtures {
 					// Convenience to allow recapturing the input and persisting it here
 					ioutil.WriteFile(inputFile, body, os.FileMode(0644))

pkg/kubectl/cmd/expose.go

@@ -34,9 +34,9 @@ import (
 )
 
 var (
-	expose_resources = `pod (po), service (svc), replicationcontroller (rc), deployment (deploy), replicaset (rs)`
+	exposeResources = `pod (po), service (svc), replicationcontroller (rc), deployment (deploy), replicaset (rs)`
 
-	expose_long = templates.LongDesc(`
+	exposeLong = templates.LongDesc(`
 		Expose a resource as a new Kubernetes service.
 
 		Looks up a deployment, service, replica set, replication controller or pod by name and uses the selector
@@ -48,9 +48,9 @@ var (
 
 		Possible resources include (case insensitive):
 
-		` + expose_resources)
+		` + exposeResources)
 
-	expose_example = templates.Examples(i18n.T(`
+	exposeExample = templates.Examples(i18n.T(`
 		# Create a service for a replicated nginx, which serves on port 80 and connects to the containers on port 8000.
 		kubectl expose rc nginx --port=80 --target-port=8000
 
@@ -77,7 +77,7 @@ func NewCmdExposeService(f cmdutil.Factory, out io.Writer) *cobra.Command {
 	options := &resource.FilenameOptions{}
 
 	validArgs, argAliases := []string{}, []string{}
-	resources := regexp.MustCompile(`\s*,`).Split(expose_resources, -1)
+	resources := regexp.MustCompile(`\s*,`).Split(exposeResources, -1)
 	for _, r := range resources {
 		validArgs = append(validArgs, strings.Fields(r)[0])
 		argAliases = kubectl.ResourceAliases(validArgs)
@@ -86,8 +86,8 @@ func NewCmdExposeService(f cmdutil.Factory, out io.Writer) *cobra.Command {
 	cmd := &cobra.Command{
 		Use:     "expose (-f FILENAME | TYPE NAME) [--port=port] [--protocol=TCP|UDP] [--target-port=number-or-name] [--name=name] [--external-ip=external-ip-of-service] [--type=type]",
 		Short:   i18n.T("Take a replication controller, service, deployment or pod and expose it as a new Kubernetes Service"),
-		Long:    expose_long,
+		Long:    exposeLong,
-		Example: expose_example,
+		Example: exposeExample,
 		Run: func(cmd *cobra.Command, args []string) {
 			err := RunExpose(f, out, cmd, args, options)
 			cmdutil.CheckErr(err)

pkg/kubectl/cmd/get.go

@@ -289,8 +289,7 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
 					first = false
 					return false, nil
 				}
-				err := printer.PrintObj(e.Object, out)
+				return false, printer.PrintObj(e.Object, out)
-				return false, err
 			})
 			return err
 		})

pkg/kubectl/cmd/get_test.go

@@ -506,9 +506,7 @@ func extractResourceList(objs []runtime.Object) ([]runtime.Object, error) {
 		if err != nil {
 			return nil, err
 		}
-		for _, item := range items {
+		finalObjs = append(finalObjs, items...)
-			finalObjs = append(finalObjs, item)
-		}
 	}
 	return finalObjs, nil
 }

pkg/kubectl/cmd/label.go

@@ -308,7 +308,7 @@ func parseLabels(spec []string) (map[string]string, []string, error) {
 	labels := map[string]string{}
 	var remove []string
 	for _, labelSpec := range spec {
-		if strings.Index(labelSpec, "=") != -1 {
+		if strings.Contains(labelSpec, "=") {
 			parts := strings.Split(labelSpec, "=")
 			if len(parts) != 2 {
 				return nil, nil, fmt.Errorf("invalid label spec: %v", labelSpec)

pkg/kubectl/cmd/run.go

@@ -95,7 +95,7 @@ func NewCmdRun(f cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer) *co
 		Example: runExample,
 		Run: func(cmd *cobra.Command, args []string) {
 			argsLenAtDash := cmd.ArgsLenAtDash()
-			err := Run(f, cmdIn, cmdOut, cmdErr, cmd, args, argsLenAtDash)
+			err := RunRun(f, cmdIn, cmdOut, cmdErr, cmd, args, argsLenAtDash)
 			cmdutil.CheckErr(err)
 		},
 	}
@@ -136,7 +136,7 @@ func addRunFlags(cmd *cobra.Command) {
 	cmd.Flags().String("schedule", "", i18n.T("A schedule in the Cron format the job should be run with."))
 }
 
-func Run(f cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer, cmd *cobra.Command, args []string, argsLenAtDash int) error {
+func RunRun(f cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer, cmd *cobra.Command, args []string, argsLenAtDash int) error {
 	// Let kubectl run follow rules for `--`, see #13004 issue
 	if len(args) == 0 || argsLenAtDash == 0 {
 		return cmdutil.UsageError(cmd, "NAME is required for run")

pkg/kubectl/cmd/run_test.go

@@ -181,7 +181,7 @@ func TestRunArgsFollowDashRules(t *testing.T) {
 		cmd := NewCmdRun(f, os.Stdin, os.Stdout, os.Stderr)
 		cmd.Flags().Set("image", "nginx")
 		cmd.Flags().Set("generator", "run/v1")
-		err := Run(f, os.Stdin, os.Stdout, os.Stderr, cmd, test.args, test.argsLenAtDash)
+		err := RunRun(f, os.Stdin, os.Stdout, os.Stderr, cmd, test.args, test.argsLenAtDash)
 		if test.expectError && err == nil {
 			t.Errorf("unexpected non-error (%s)", test.name)
 		}
@@ -438,7 +438,7 @@ func TestRunValidations(t *testing.T) {
 		for flagName, flagValue := range test.flags {
 			cmd.Flags().Set(flagName, flagValue)
 		}
-		err := Run(f, inBuf, outBuf, errBuf, cmd, test.args, cmd.ArgsLenAtDash())
+		err := RunRun(f, inBuf, outBuf, errBuf, cmd, test.args, cmd.ArgsLenAtDash())
 		if err != nil && len(test.expectedErr) > 0 {
 			if !strings.Contains(err.Error(), test.expectedErr) {
 				t.Errorf("unexpected error: %v", err)

pkg/kubectl/cmd/taint.go

@@ -178,7 +178,7 @@ func parseTaints(spec []string) ([]v1.Taint, []v1.Taint, error) {
 	uniqueTaints := map[v1.TaintEffect]sets.String{}
 
 	for _, taintSpec := range spec {
-		if strings.Index(taintSpec, "=") != -1 && strings.Index(taintSpec, ":") != -1 {
+		if strings.Contains(taintSpec, "=") && strings.Contains(taintSpec, ":") {
 			newTaint, err := utiltaints.ParseTaint(taintSpec)
 			if err != nil {
 				return nil, nil, err
@@ -197,7 +197,7 @@ func parseTaints(spec []string) ([]v1.Taint, []v1.Taint, error) {
 		} else if strings.HasSuffix(taintSpec, "-") {
 			taintKey := taintSpec[:len(taintSpec)-1]
 			var effect v1.TaintEffect
-			if strings.Index(taintKey, ":") != -1 {
+			if strings.Contains(taintKey, ":") {
 				parts := strings.Split(taintKey, ":")
 				taintKey = parts[0]
 				effect = v1.TaintEffect(parts[1])

pkg/kubectl/cmd/version.go

@@ -107,11 +107,7 @@ func RunVersion(f cmdutil.Factory, out io.Writer, cmd *cobra.Command) error {
 
 	}
 
-	if serverErr != nil {
+	return serverErr
-		return serverErr
-	}
-
-	return nil
 }
 
 func retrieveServerVersion(f cmdutil.Factory) (*apimachineryversion.Info, error) {

pkg/kubectl/doc.go

@@ -14,7 +14,8 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-// Package kubectl is a set of libraries that are used by the kubectl command line tool.
+// Package kubectl provides the functions used by the kubectl command line tool
-// They are separated out into a library to support unit testing.  Most functionality should
+// under k8s.io/kubernetes/cmd. The functions are kept in this package to better
-// be included in this package, and the main kubectl should really just be an entry point.
+// support unit testing. The main() method for kubectl is only an entry point
+// and should contain no functionality.
 package kubectl // import "k8s.io/kubernetes/pkg/kubectl"

pkg/kubectl/env_file.go

@@ -28,6 +28,47 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation"
 )
 
+var utf8bom = []byte{0xEF, 0xBB, 0xBF}
+
+// proccessEnvFileLine returns a blank key if the line is empty or a comment.
+// The value will be retrieved from the environment if necessary.
+func proccessEnvFileLine(line []byte, filePath string,
+	currentLine int) (key, value string, err error) {
+
+	if !utf8.Valid(line) {
+		return ``, ``, fmt.Errorf("env file %s contains invalid utf8 bytes at line %d: %v",
+			filePath, currentLine+1, line)
+	}
+
+	// We trim UTF8 BOM from the first line of the file but no others
+	if currentLine == 0 {
+		line = bytes.TrimPrefix(line, utf8bom)
+	}
+
+	// trim the line from all leading whitespace first
+	line = bytes.TrimLeftFunc(line, unicode.IsSpace)
+
+	// If the line is empty or a comment, we return a blank key/value pair.
+	if len(line) == 0 || line[0] == '#' {
+		return ``, ``, nil
+	}
+
+	data := strings.SplitN(string(line), "=", 2)
+	key = data[0]
+	if errs := validation.IsCIdentifier(key); len(errs) != 0 {
+		return ``, ``, fmt.Errorf("%q is not a valid key name: %s", key, strings.Join(errs, ";"))
+	}
+
+	if len(data) == 2 {
+		value = data[1]
+	} else {
+		// No value (no `=` in the line) is a signal to obtain the value
+		// from the environment.
+		value = os.Getenv(key)
+	}
+	return
+}
+
 // addFromEnvFile processes an env file allows a generic addTo to handle the
 // collection of key value pairs or returns an error.
 func addFromEnvFile(filePath string, addTo func(key, value string) error) error {
@@ -39,38 +80,23 @@ func addFromEnvFile(filePath string, addTo func(key, value string) error) error
 
 	scanner := bufio.NewScanner(f)
 	currentLine := 0
-	utf8bom := []byte{0xEF, 0xBB, 0xBF}
 	for scanner.Scan() {
+		// Proccess the current line, retrieving a key/value pair if
+		// possible.
 		scannedBytes := scanner.Bytes()
-		if !utf8.Valid(scannedBytes) {
+		key, value, err := proccessEnvFileLine(scannedBytes, filePath, currentLine)
-			return fmt.Errorf("env file %s contains invalid utf8 bytes at line %d: %v", filePath, currentLine+1, scannedBytes)
+		if err != nil {
+			return err
 		}
-		// We trim UTF8 BOM
-		if currentLine == 0 {
-			scannedBytes = bytes.TrimPrefix(scannedBytes, utf8bom)
-		}
-		// trim the line from all leading whitespace first
-		line := strings.TrimLeftFunc(string(scannedBytes), unicode.IsSpace)
 		currentLine++
-		// line is not empty, and not starting with '#'
-		if len(line) > 0 && !strings.HasPrefix(line, "#") {
-			data := strings.SplitN(line, "=", 2)
-			key := data[0]
-			if errs := validation.IsCIdentifier(key); len(errs) != 0 {
-				return fmt.Errorf("%q is not a valid key name: %s", key, strings.Join(errs, ";"))
-			}
 
-			value := ""
+		if len(key) == 0 {
-			if len(data) > 1 {
+			// no key means line was empty or a comment
-				// pass the value through, no trimming
+			continue
-				value = data[1]
+		}
-			} else {
+
-				// a pass-through variable is given
+		if err = addTo(key, value); err != nil {
-				value = os.Getenv(key)
+			return err
-			}
-			if err = addTo(key, value); err != nil {
-				return err
-			}
 		}
 	}
 	return nil

pkg/kubectl/env_file_test.go

@@ -0,0 +1,103 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package kubectl
+
+import (
+	"os"
+	"strings"
+	"testing"
+)
+
+// Test the cases of proccessEnvFileLine that can be run without touching the
+// environment.
+func Test_processEnvFileLine(t *testing.T) {
+	testCases := []struct {
+		name          string
+		line          []byte
+		currentLine   int
+		expectedKey   string
+		expectedValue string
+		expectedErr   string
+	}{
+		{"the utf8bom is trimmed on the first line",
+			append(utf8bom, 'a', '=', 'c'), 0, "a", "c", ""},
+
+		{"the utf8bom is NOT trimmed on the second line",
+			append(utf8bom, 'a', '=', 'c'), 1, "", "", "not a valid key name"},
+
+		{"no key is returned on a comment line",
+			[]byte{' ', '#', 'c'}, 0, "", "", ""},
+
+		{"no key is returned on a blank line",
+			[]byte{' ', ' ', '\t'}, 0, "", "", ""},
+
+		{"key is returned even with no value",
+			[]byte{' ', 'x', '='}, 0, "x", "", ""},
+	}
+	for _, c := range testCases {
+		key, value, err := proccessEnvFileLine(c.line, `filename`, c.currentLine)
+		t.Logf("Testing that %s.", c.name)
+		if c.expectedKey != key {
+			t.Errorf("\texpected key %q, received %q", c.expectedKey, key)
+		}
+		if c.expectedValue != value {
+			t.Errorf("\texpected value %q, received %q", c.expectedValue, value)
+		}
+		if len(c.expectedErr) == 0 {
+			if err != nil {
+				t.Errorf("\tunexpected err %v", err)
+			}
+		} else {
+			if !strings.Contains(err.Error(), c.expectedErr) {
+				t.Errorf("\terr %v doesn't match expected %q", err, c.expectedErr)
+			}
+		}
+	}
+}
+
+// proccessEnvFileLine needs to fetch the value from the environment if no
+// equals sign is provided.
+// For example:
+//
+//	my_key1=alpha
+//	my_key2=beta
+//	my_key3
+//
+// In this file, my_key3 must be fetched from the environment.
+// Test this capability.
+func Test_processEnvFileLine_readEnvironment(t *testing.T) {
+	const realKey = "k8s_test_env_file_key"
+	const realValue = `my_value`
+
+	// Just in case, these two lines ensure the environment is restored to
+	// its original state.
+	original := os.Getenv(realKey)
+	defer func() { os.Setenv(realKey, original) }()
+
+	os.Setenv(realKey, `my_value`)
+
+	key, value, err := proccessEnvFileLine([]byte(realKey), `filename`, 3)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if key != realKey {
+		t.Errorf(`expected key %q, received %q`, realKey, key)
+	}
+	if value != realValue {
+		t.Errorf(`expected value %q, received %q`, realValue, value)
+	}
+}

pkg/kubectl/generate.go

@@ -35,7 +35,9 @@ type GeneratorParam struct {
 	Required bool
 }
 
-// Generator is an interface for things that can generate API objects from input parameters.
+// Generator is an interface for things that can generate API objects from input
+// parameters. One example is the "expose" generator that is capable of exposing
+// new replication controllers and services, among other things.
 type Generator interface {
 	// Generate creates an API object given a set of parameters
 	Generate(params map[string]interface{}) (runtime.Object, error)

pkg/kubectl/kubectl.go

@@ -212,7 +212,10 @@ func parseFileSource(source string) (keyName, filePath string, err error) {
 	}
 }
 
-// parseLiteralSource parses the source key=val pair
+// parseLiteralSource parses the source key=val pair into its component pieces.
+// This functionality is distinguished from strings.SplitN(source, "=", 2) since
+// it returns an error in the case of empty keys, values, or a missing equals
+// sign.
 func parseLiteralSource(source string) (keyName, value string, err error) {
 	// leading equal is invalid
 	if strings.Index(source, "=") == 0 {

pkg/kubectl/proxy_server.go

@@ -108,7 +108,8 @@ func (f *FilterServer) accept(method, path, host string) bool {
 	return false
 }
 
-// Make a copy of f which passes requests along to the new delegate.
+// HandlerFor makes a shallow copy of f which passes its requests along to the
+// new delegate.
 func (f *FilterServer) HandlerFor(delegate http.Handler) *FilterServer {
 	f2 := *f
 	f2.delegate = delegate

pkg/kubectl/rolling_updater.go

@@ -565,10 +565,7 @@ func Rename(c coreclient.ReplicationControllersGetter, rc *api.ReplicationContro
 	}
 	// Then create the same RC with the new name.
 	_, err = c.ReplicationControllers(rc.Namespace).Create(rc)
-	if err != nil {
+	return err
-		return err
-	}
-	return nil
 }
 
 func LoadExistingNextReplicationController(c coreclient.ReplicationControllersGetter, namespace, newName string) (*api.ReplicationController, error) {
@@ -684,14 +681,14 @@ func UpdateExistingReplicationController(rcClient coreclient.ReplicationControll
 	if _, found := oldRc.Spec.Selector[deploymentKey]; !found {
 		SetNextControllerAnnotation(oldRc, newName)
 		return AddDeploymentKeyToReplicationController(oldRc, rcClient, podClient, deploymentKey, deploymentValue, namespace, out)
-	} else {
-		// If we didn't need to update the controller for the deployment key, we still need to write
-		// the "next" controller.
-		applyUpdate := func(rc *api.ReplicationController) {
-			SetNextControllerAnnotation(rc, newName)
-		}
-		return updateRcWithRetries(rcClient, namespace, oldRc, applyUpdate)
 	}
+
+	// If we didn't need to update the controller for the deployment key, we still need to write
+	// the "next" controller.
+	applyUpdate := func(rc *api.ReplicationController) {
+		SetNextControllerAnnotation(rc, newName)
+	}
+	return updateRcWithRetries(rcClient, namespace, oldRc, applyUpdate)
 }
 
 func AddDeploymentKeyToReplicationController(oldRc *api.ReplicationController, rcClient coreclient.ReplicationControllersGetter, podClient coreclient.PodsGetter, deploymentKey, deploymentValue, namespace string, out io.Writer) (*api.ReplicationController, error) {

pkg/kubectl/run.go

@@ -868,9 +868,3 @@ func parseEnvs(envArray []string) ([]v1.EnvVar, error) {
 	}
 	return envs, nil
 }
-
-func newBool(val bool) *bool {
-	p := new(bool)
-	*p = val
-	return p
-}

pkg/kubectl/stop.go

@@ -46,11 +46,12 @@ const (
 	Timeout  = time.Minute * 5
 )
 
-// A Reaper handles terminating an object as gracefully as possible.
+// A Reaper terminates an object as gracefully as possible.
-// timeout is how long we'll wait for the termination to be successful
-// gracePeriod is time given to an API object for it to delete itself cleanly,
-// e.g., pod shutdown. It may or may not be supported by the API object.
 type Reaper interface {
+	// Stop a given object within a namespace. timeout is how long we'll
+	// wait for the termination to be successful. gracePeriod is time given
+	// to an API object for it to delete itself cleanly (e.g., pod
+	// shutdown). It may or may not be supported by the API object.
 	Stop(namespace, name string, timeout time.Duration, gracePeriod *metav1.DeleteOptions) error
 }
 
@@ -135,11 +136,6 @@ type StatefulSetReaper struct {
 	pollInterval, timeout time.Duration
 }
 
-type objInterface interface {
-	Delete(name string) error
-	Get(name string) (metav1.Object, error)
-}
-
 // getOverlappingControllers finds rcs that this controller overlaps, as well as rcs overlapping this controller.
 func getOverlappingControllers(rcClient coreclient.ReplicationControllerInterface, rc *api.ReplicationController) ([]api.ReplicationController, error) {
 	rcs, err := rcClient.List(metav1.ListOptions{})
@@ -336,6 +332,8 @@ func (reaper *StatefulSetReaper) Stop(namespace, name string, timeout time.Durat
 	}
 	if timeout == 0 {
 		numReplicas := ss.Spec.Replicas
+
+		// BUG: this timeout is never used.
 		timeout = Timeout + time.Duration(10*numReplicas)*time.Second
 	}
 	retry := NewRetryParams(reaper.pollInterval, reaper.timeout)