Review comments and other improvements

fabianofranz · fabianofranz · commit 2761945adf95 · 2015-05-12T18:38:58.000-03:00
diff --git a/hack/test-cmd.sh b/hack/test-cmd.sh
@@ -414,8 +414,6 @@ osc describe deploymentConfigs test-deployment-config
 [ ! "$(echo "#OTHER=foo" | osc env dc/test-deployment-config -e - --list | grep OTHER=foo)" ]
 [ "$(osc env dc/test-deployment-config TEST=bar OTHER=baz BAR-)" ]
 osc deploy test-deployment-config
-osc deploy test-deployment-config -L
-osc deploy test-deployment-config -L 3
 osc delete deploymentConfigs test-deployment-config
 echo "deploymentConfigs: ok"
 
diff --git a/pkg/cmd/cli/cmd/deploy.go b/pkg/cmd/cli/cmd/deploy.go
@@ -1,21 +1,36 @@
 package cmd
 
 import (
+	"errors"
 	"fmt"
 	"io"
 
 	"github.com/spf13/cobra"
 
 	kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	kerrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
+	kclient "github.com/GoogleCloudPlatform/kubernetes/pkg/client"
 	cmdutil "github.com/GoogleCloudPlatform/kubernetes/pkg/kubectl/cmd/util"
 
+	"github.com/openshift/origin/pkg/client"
 	"github.com/openshift/origin/pkg/cmd/cli/describe"
 	"github.com/openshift/origin/pkg/cmd/util/clientcmd"
 	deployapi "github.com/openshift/origin/pkg/deploy/api"
 	deployutil "github.com/openshift/origin/pkg/deploy/util"
 )
 
+type DeployOptions struct {
+	out             io.Writer
+	osClient        *client.Client
+	kubeClient      *kclient.Client
+	namespace       string
+	baseCommandName string
+
+	deploymentConfigName string
+	deployLatest         bool
+	retryDeploy          bool
+}
+
 const (
 	deploy_long = `View, start and restart deployments.
 
@@ -26,92 +41,115 @@ NOTE: This command is still under active development and is subject to change.`
 	deploy_example = `  // Display the latest deployment for the 'database' deployment config
   $ %[1]s deploy database
 
-  // List all deployments for the 'database' deployment config
-  $ %[1]s deploy database -L
+  // Start a new deployment based on the 'database' deployment config
+  $ %[1]s deploy frontend --latest
 
-  // List the most recent deployments for the 'database' deployment config, limited to three
-  $ %[1]s deploy database -L 3
-
-  // Start a new deployment based on the 'frontend' deployment config
-  $ %[1]s deploy frontend --latest`
+  // Retry the latest failed deployment based on the 'frontend' deployment config
+  $ %[1]s deploy frontend --retry`
 )
 
 // NewCmdDeploy creates a new `deploy` command.
 func NewCmdDeploy(fullName string, f *clientcmd.Factory, out io.Writer) *cobra.Command {
-	var deployLatest bool
-	var retryDeploy bool
-	var listSize int
+	options := &DeployOptions{
+		baseCommandName: fullName,
+	}
 
 	cmd := &cobra.Command{
 		Use:     "deploy DEPLOYMENTCONFIG",
 		Short:   "View, start and restart deployments.",
 		Long:    deploy_long,
 		Example: fmt.Sprintf(deploy_example, fullName),
 		Run: func(cmd *cobra.Command, args []string) {
-			if len(args) == 0 || len(args[0]) == 0 {
-				fmt.Println(cmdutil.UsageError(cmd, "A deploymentConfig name is required."))
-				return
-			}
-			listSizeProvided := cmd.Flags().Changed("list")
-			if listSizeProvided && (deployLatest || retryDeploy) {
-				fmt.Println(cmdutil.UsageError(cmd, "The -L|--list flag can't be used with --latest or --retry."))
-				return
+			if err := options.Complete(f, args, out); err != nil {
+				cmdutil.CheckErr(err)
 			}
-			if deployLatest && retryDeploy {
-				fmt.Println(cmdutil.UsageError(cmd, "Only one of --latest or --retry is allowed."))
-				return
+
+			if err := options.Validate(args); err != nil {
+				cmdutil.CheckErr(cmdutil.UsageError(cmd, err.Error()))
 			}
-			if !listSizeProvided {
-				listSize = 1
+
+			if err := options.RunDeploy(); err != nil {
+				cmdutil.CheckErr(err)
 			}
+		},
+	}
 
-			configName := args[0]
+	cmd.Flags().BoolVar(&options.deployLatest, "latest", false, "Start a new deployment now.")
+	cmd.Flags().BoolVar(&options.retryDeploy, "retry", false, "Retry the latest failed deployment.")
 
-			osClient, kubeClient, err := f.Clients()
-			cmdutil.CheckErr(err)
+	return cmd
+}
 
-			namespace, err := f.DefaultNamespace()
-			cmdutil.CheckErr(err)
+func (o *DeployOptions) Complete(f *clientcmd.Factory, args []string, out io.Writer) error {
+	var err error
 
-			config, err := osClient.DeploymentConfigs(namespace).Get(configName)
-			cmdutil.CheckErr(err)
+	o.osClient, o.kubeClient, err = f.Clients()
+	if err != nil {
+		return err
+	}
+	o.namespace, err = f.DefaultNamespace()
+	if err != nil {
+		return err
+	}
 
-			commandClient := &deployCommandClientImpl{
-				GetDeploymentFn: func(namespace, name string) (*kapi.ReplicationController, error) {
-					return kubeClient.ReplicationControllers(namespace).Get(name)
-				},
-				UpdateDeploymentConfigFn: func(config *deployapi.DeploymentConfig) (*deployapi.DeploymentConfig, error) {
-					return osClient.DeploymentConfigs(config.Namespace).Update(config)
-				},
-				UpdateDeploymentFn: func(deployment *kapi.ReplicationController) (*kapi.ReplicationController, error) {
-					return kubeClient.ReplicationControllers(deployment.Namespace).Update(deployment)
-				},
-			}
+	o.out = out
 
-			switch {
-			case deployLatest:
-				c := &deployLatestCommand{client: commandClient}
-				err = c.deploy(config, out)
-			case retryDeploy:
-				c := &retryDeploymentCommand{client: commandClient}
-				err = c.retry(config, out)
-			default:
-				describer := describe.NewLatestDeploymentsDescriber(osClient, kubeClient, listSize)
-				desc, err := describer.Describe(config.Namespace, config.Name)
-				cmdutil.CheckErr(err)
-				fmt.Fprintln(out, desc)
-				fmt.Fprintf(out, "For more details run '%v describe dc/%v'.\n", fullName, config.Name)
-			}
-			cmdutil.CheckErr(err)
+	if len(args) > 0 {
+		o.deploymentConfigName = args[0]
+	}
+
+	return nil
+}
+
+func (o DeployOptions) Validate(args []string) error {
+	if len(args) == 0 || len(args[0]) == 0 {
+		return errors.New("A deploymentConfig name is required.")
+	}
+	if len(args) > 1 {
+		return errors.New("Only one deploymentConfig name is supported as argument.")
+	}
+	if o.deployLatest && o.retryDeploy {
+		return errors.New("Only one of --latest or --retry is allowed.")
+	}
+	return nil
+}
+
+func (o DeployOptions) RunDeploy() error {
+	config, err := o.osClient.DeploymentConfigs(o.namespace).Get(o.deploymentConfigName)
+	if err != nil {
+		return err
+	}
+
+	commandClient := &deployCommandClientImpl{
+		GetDeploymentFn: func(namespace, name string) (*kapi.ReplicationController, error) {
+			return o.kubeClient.ReplicationControllers(namespace).Get(name)
+		},
+		UpdateDeploymentConfigFn: func(config *deployapi.DeploymentConfig) (*deployapi.DeploymentConfig, error) {
+			return o.osClient.DeploymentConfigs(config.Namespace).Update(config)
+		},
+		UpdateDeploymentFn: func(deployment *kapi.ReplicationController) (*kapi.ReplicationController, error) {
+			return o.kubeClient.ReplicationControllers(deployment.Namespace).Update(deployment)
 		},
 	}
 
-	cmd.Flags().BoolVar(&deployLatest, "latest", false, "Start a new deployment now.")
-	cmd.Flags().BoolVar(&retryDeploy, "retry", false, "Retry the latest failed deployment.")
-	cmd.Flags().IntVarP(&listSize, "list", "L", -1, "Print a list of the latest deployments, limited to the amount provided.")
-	cmd.Flags().MarkOptional("list")
+	switch {
+	case o.deployLatest:
+		c := &deployLatestCommand{client: commandClient}
+		err = c.deploy(config, o.out)
+	case o.retryDeploy:
+		c := &retryDeploymentCommand{client: commandClient}
+		err = c.retry(config, o.out)
+	default:
+		describer := describe.NewLatestDeploymentsDescriber(o.osClient, o.kubeClient, 1)
+		desc, err := describer.Describe(config.Namespace, config.Name)
+		if err != nil {
+			return err
+		}
+		fmt.Fprintln(o.out, desc)
+		fmt.Fprintf(o.out, "For more details run '%v describe dc/%v'.\n", o.baseCommandName, config.Name)
+	}
 
-	return cmd
+	return err
 }
 
 // deployCommandClient abstracts access to the API server.
diff --git a/pkg/cmd/cli/describe/deployments.go b/pkg/cmd/cli/describe/deployments.go
@@ -148,7 +148,7 @@ func (d *DeploymentConfigDescriber) Describe(namespace, name string) (string, er
 			sorted = append(sorted, deploymentsHistory.Items...)
 			sort.Sort(sorted)
 			for _, item := range sorted {
-				if item.Name != deploymentName {
+				if item.Name != deploymentName && deploymentConfig.Name == item.Annotations[deployapi.DeploymentConfigAnnotation] {
 					header := fmt.Sprintf("Deployment #%v", item.Annotations[deployapi.DeploymentVersionAnnotation])
 					printDeploymentRc(&item, d.client, out, header, false)
 				}
@@ -244,15 +244,17 @@ func printDeploymentRc(deployment *kapi.ReplicationController, client deployment
 		fmt.Fprintf(w, "%v:\n", header)
 	}
 
-	fmt.Fprintf(w, "\tName:\t%s\n", deployment.Name)
+	if verbose {
+		fmt.Fprintf(w, "\tName:\t%s\n", deployment.Name)
+	}
 	timeAt := strings.ToLower(formatRelativeTime(deployment.CreationTimestamp.Time))
 	fmt.Fprintf(w, "\tCreated:\t%s ago\n", timeAt)
 	fmt.Fprintf(w, "\tStatus:\t%s\n", deployment.Annotations[deployapi.DeploymentStatusAnnotation])
-	fmt.Fprintf(w, "\tSelector:\t%s\n", formatLabels(deployment.Spec.Selector))
+	fmt.Fprintf(w, "\tReplicas:\t%d current / %d desired\n", deployment.Status.Replicas, deployment.Spec.Replicas)
 
 	if verbose {
+		fmt.Fprintf(w, "\tSelector:\t%s\n", formatLabels(deployment.Spec.Selector))
 		fmt.Fprintf(w, "\tLabels:\t%s\n", formatLabels(deployment.Labels))
-		fmt.Fprintf(w, "\tReplicas:\t%d current / %d desired\n", deployment.Status.Replicas, deployment.Spec.Replicas)
 		running, waiting, succeeded, failed, err := getPodStatusForDeployment(deployment, client)
 		if err != nil {
 			return err
