Merge pull request #198 from sallyom/cli-cleanup

cli cleanup: minor

pkg/cmd/create_replication.go

@@ -103,25 +103,31 @@ func NewCmdScribeStartReplication(streams genericclioptions.IOStreams) *cobra.Co
 //nolint:lll
 func (o *SetupReplicationOptions) bindFlags(cmd *cobra.Command, v *viper.Viper) error {
 	flags := cmd.Flags()
-	flags.StringVar(&o.CopyMethod, "source-copy-method", o.CopyMethod, "the method of creating a point-in-time image of the source volume; one of 'None|Clone|Snapshot'")
+	flags.StringVar(&o.CopyMethod, "source-copy-method", o.CopyMethod, "the method of creating a point-in-time image of the source volume. "+
+		"one of 'None|Clone|Snapshot'")
 	flags.StringVar(&o.Capacity, "source-capacity", o.Capacity, "provided to override the capacity of the point-in-Time image.")
 	flags.StringVar(&o.StorageClass, "source-storage-class-name", o.StorageClass, "provided to override the StorageClass of the point-in-Time image.")
-	flags.StringVar(&o.AccessMode, "source-access-mode", o.AccessMode, "provided to override the accessModes of the point-in-Time image. One of 'ReadWriteOnce|ReadOnlyMany|ReadWriteMany")
-	flags.StringVar(&o.VolumeSnapshotClassName, "source-volume-snapshot-class", o.VolumeSnapshotClassName, "name of VolumeSnapshotClass for the source volume, only if copyMethod is 'Snapshot'. If empty, default VSC will be used.")
+	flags.StringVar(&o.AccessMode, "source-access-mode", o.AccessMode, "provided to override the accessModes of the point-in-Time image. "+
+		"One of 'ReadWriteOnce|ReadOnlyMany|ReadWriteMany")
+	flags.StringVar(&o.VolumeSnapshotClassName, "source-volume-snapshot-class", o.VolumeSnapshotClassName, ""+
+		"name of VolumeSnapshotClass for the source volume, only if copyMethod is 'Snapshot'. If empty, default VSC will be used.")
 	flags.StringVar(&o.SourcePVC, "source-pvc", o.SourcePVC, "name of an existing PersistentVolumeClaim (PVC) to replicate.")
 	// TODO: Default to every 3min for source?
 	flags.StringVar(&o.Schedule, "source-cron-spec", "*/5 * * * *", "cronspec to be used to schedule capturing the state of the source volume.")
 	// Defaults to "root" after creation
 	flags.StringVar(&o.SSHUser, "source-ssh-user", o.SSHUser, "username for outgoing SSH connections (default 'root')")
 	// Defaults to ClusterIP after creation
-	flags.StringVar(&o.ServiceType, "source-service-type", o.ServiceType, "one of ClusterIP|LoadBalancer. Service type that will be created for incoming SSH connections. (default 'ClusterIP')")
+	flags.StringVar(&o.ServiceType, "source-service-type", o.ServiceType, ""+
+		"one of ClusterIP|LoadBalancer. Service type that will be created for incoming SSH connections. (default 'ClusterIP')")
 	// TODO: Defaulted in CLI, should it be??
 	flags.StringVar(&o.Name, "source-name", o.Name, "name of the ReplicationSource resource (default '<source-ns>-source')")
 	// defaults to 22 after creation
 	flags.Int32Var(&o.Port, "source-port", o.Port, "SSH port to connect to for replication. (default 22)")
-	flags.StringVar(&o.Provider, "source-provider", o.Provider, "name of an external replication provider, if applicable; pass as 'domain.com/provider'")
+	flags.StringVar(&o.Provider, "source-provider", o.Provider, "name of an external replication provider, if applicable. "+
+		"Provide as 'domain.com/provider'")
 	// TODO: I don't know how many params providers have? If a lot, can pass a file instead
-	flags.StringVar(&o.ProviderParameters, "source-provider-parameters", o.ProviderParameters, "provider-specific key=value configuration parameters, for an external provider; pass 'key=value,key1=value1'")
+	flags.StringVar(&o.ProviderParameters, "source-provider-parameters", o.ProviderParameters, ""+
+		"provider-specific key=value configuration parameters, for an external provider; pass 'key=value,key1=value1'")
 	// TODO: Defaulted with CLI, should it be??
 	if err := cmd.MarkFlagRequired("source-copy-method"); err != nil {
 		return err

pkg/cmd/destination.go

@@ -38,25 +38,35 @@ type DestinationOptions struct {
 //nolint:lll
 func (o *DestinationOptions) Bind(cmd *cobra.Command, v *viper.Viper) error {
 	flags := cmd.Flags()
-	flags.StringVar(&o.CopyMethod, "dest-copy-method", o.CopyMethod, "the method of creating a point-in-time image of the destination volume; one of 'None|Clone|Snapshot'")
+	flags.StringVar(&o.CopyMethod, "dest-copy-method", o.CopyMethod, ""+
+		"the method of creating a point-in-time image of the destination volume; one of 'None|Clone|Snapshot'")
 	flags.StringVar(&o.Address, "dest-address", o.Address, "the remote address to connect to for replication.")
 	// TODO: Defaulted with CLI, should it be??
 	flags.StringVar(&o.Capacity, "dest-capacity", "2Gi", "Size of the destination volume to create. Must be provided if --dest-pvc is not provided.")
-	flags.StringVar(&o.StorageClass, "dest-storage-class", o.StorageClass, "name of the StorageClass of the destination volume. If not set, the default StorageClass will be used.")
-	flags.StringVar(&o.AccessMode, "dest-access-mode", o.AccessMode, "the access modes for the destination volume. Must be provided if --dest-pvc is not provided; One of 'ReadWriteOnce|ReadOnlyMany|ReadWriteMany")
-	flags.StringVar(&o.VolumeSnapshotClassName, "dest-volume-snapshot-class", o.VolumeSnapshotClassName, "name of the VolumeSnapshotClass to be used for the destination volume, only if the copyMethod is 'Snapshot'. If not set, the default VSC will be used.")
-	flags.StringVar(&o.DestPVC, "dest-pvc", o.DestPVC, "name of an existing empty PVC in the destination namespace to use as the transfer destination volume. If empty, one will be provisioned.")
-	flags.StringVar(&o.Schedule, "dest-cron-spec", o.Schedule, "cronspec to be used to schedule replication to occur at regular, time-based intervals. If not set replication will be continuous.")
+	flags.StringVar(&o.StorageClass, "dest-storage-class", o.StorageClass, ""+
+		"name of the StorageClass of the destination volume. If not set, the default StorageClass will be used.")
+	flags.StringVar(&o.AccessMode, "dest-access-mode", o.AccessMode, ""+
+		"the access modes for the destination volume. Must be provided if --dest-pvc is not provided. "+
+		"One of 'ReadWriteOnce|ReadOnlyMany|ReadWriteMany")
+	flags.StringVar(&o.VolumeSnapshotClassName, "dest-volume-snapshot-class", o.VolumeSnapshotClassName, ""+
+		"name of the VolumeSnapshotClass to be used for the destination volume, only if the copyMethod is 'Snapshot'. "+
+		"If not set, the default VSC will be used.")
+	flags.StringVar(&o.DestPVC, "dest-pvc", o.DestPVC, ""+
+		"name of an existing empty PVC in the destination namespace to use as the transfer destination volume. If empty, one will be provisioned.")
+	flags.StringVar(&o.Schedule, "dest-cron-spec", o.Schedule, ""+
+		"cronspec to be used to schedule replication to occur at regular, time-based intervals. If not set replication will be continuous.")
 	// Defaults to "root" after creation
 	flags.StringVar(&o.SSHUser, "dest-ssh-user", o.SSHUser, "username for outgoing SSH connections (default 'root')")
 	// Defaults to ClusterIP after creation
-	flags.StringVar(&o.ServiceType, "dest-service-type", o.ServiceType, "one of ClusterIP|LoadBalancer. Service type to be created for incoming SSH connections. (default 'ClusterIP')")
+	flags.StringVar(&o.ServiceType, "dest-service-type", o.ServiceType, ""+
+		"one of ClusterIP|LoadBalancer. Service type to be created for incoming SSH connections. (default 'ClusterIP')")
 	// TODO: Defaulted in CLI, should it be??
 	flags.StringVar(&o.Name, "dest-name", o.Name, "name of the ReplicationDestination resource. (default '<current-namespace>-scribe-destination')")
 	flags.Int32Var(&o.Port, "dest-port", o.Port, "SSH port to connect to for replication. (default 22)")
 	flags.StringVar(&o.Provider, "dest-provider", o.Provider, "name of an external replication provider, if applicable; pass as 'domain.com/provider'")
 	// TODO: I don't know how many params providers have? If a lot, can pass a file instead
-	flags.StringVar(&o.ProviderParameters, "dest-provider-parameters", o.ProviderParameters, "provider-specific key=value configuration parameters, for an external provider; pass 'key=value,key1=value1'")
+	flags.StringVar(&o.ProviderParameters, "dest-provider-parameters", o.ProviderParameters, ""+
+		"provider-specific key=value configuration parameters, for an external provider; pass 'key=value,key1=value1'")
 	// defaults to "/" after creation
 	flags.StringVar(&o.Path, "dest-path", o.Path, "the remote path to rsync to (default '/')")
 	if err := cmd.MarkFlagRequired("dest-copy-method"); err != nil {

pkg/cmd/options.go

@@ -5,9 +5,10 @@ import (
 	"fmt"
 	"strings"
 
-	scribev1alpha1 "github.com/backube/scribe/api/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
+
+	scribev1alpha1 "github.com/backube/scribe/api/v1alpha1"
 )
 
 const (
@@ -31,15 +32,14 @@ type sharedOptions struct {
 
 //nolint:funlen
 func (o *SetupReplicationOptions) getCommonOptions(c *sharedOptions, mode string) error {
-	var ok bool
-	if ok := o.getCopyMethod(c.CopyMethod, mode); !ok {
-		return fmt.Errorf("error applying %s copyMethod %s", mode, o.CopyMethod)
+	if err := o.getCopyMethod(c.CopyMethod, mode); err != nil {
+		return err
 	}
-	if ok = o.getAccessModes(c.AccessMode, mode); !ok {
-		return fmt.Errorf("error applying %s accessModes %s", mode, o.AccessMode)
+	if err := o.getAccessModes(c.AccessMode, mode); err != nil {
+		return err
 	}
-	if ok = o.getServiceType(c.ServiceType, mode); !ok {
-		return fmt.Errorf("error applying %s serviceType %s", mode, o.ServiceType)
+	if err := o.getServiceType(c.ServiceType, mode); err != nil {
+		return err
 	}
 	port := &c.Port
 	if c.Port == 0 {
@@ -120,7 +120,7 @@ func (o *SetupReplicationOptions) getCapacity(c string, mode string) error {
 	return nil
 }
 
-func (o *SetupReplicationOptions) getCopyMethod(c string, mode string) bool {
+func (o *SetupReplicationOptions) getCopyMethod(c string, mode string) error {
 	var cm scribev1alpha1.CopyMethodType
 	// CopyMethod is always required
 	switch strings.ToLower(c) {
@@ -131,18 +131,18 @@ func (o *SetupReplicationOptions) getCopyMethod(c string, mode string) bool {
 	case "snapshot":
 		cm = scribev1alpha1.CopyMethodSnapshot
 	default:
-		return false
+		return fmt.Errorf("unsupported %s copyMethod %s", mode, c)
 	}
 	switch mode {
 	case scribeDest:
 		o.RepOpts.Dest.CopyMethod = cm
 	case scribeSource:
 		o.RepOpts.Source.CopyMethod = cm
 	}
-	return true
+	return nil
 }
 
-func (o *SetupReplicationOptions) getAccessModes(c string, mode string) bool {
+func (o *SetupReplicationOptions) getAccessModes(c string, mode string) error {
 	var am []corev1.PersistentVolumeAccessMode
 	// AccessMode not always required
 	if len(c) > 0 {
@@ -154,7 +154,7 @@ func (o *SetupReplicationOptions) getAccessModes(c string, mode string) bool {
 		case "ReadOnlyMany":
 			am = []corev1.PersistentVolumeAccessMode{corev1.ReadOnlyMany}
 		default:
-			return false
+			return fmt.Errorf("unsupported %s accessMode: %s", mode, c)
 		}
 	}
 	switch mode {
@@ -163,10 +163,10 @@ func (o *SetupReplicationOptions) getAccessModes(c string, mode string) bool {
 	case scribeSource:
 		o.RepOpts.Source.AccessModes = am
 	}
-	return true
+	return nil
 }
 
-func (o *SetupReplicationOptions) getServiceType(c string, mode string) bool {
+func (o *SetupReplicationOptions) getServiceType(c string, mode string) error {
 	// defaults to ClusterIP if not set
 	var st corev1.ServiceType
 	if len(c) > 0 {
@@ -176,7 +176,7 @@ func (o *SetupReplicationOptions) getServiceType(c string, mode string) bool {
 		case "loadbalancer":
 			st = corev1.ServiceTypeLoadBalancer
 		default:
-			return false
+			return fmt.Errorf("unsupported %s serviceType %s", mode, c)
 		}
 	}
 	switch mode {
@@ -185,5 +185,5 @@ func (o *SetupReplicationOptions) getServiceType(c string, mode string) bool {
 	case scribeSource:
 		o.RepOpts.Source.ServiceType = st
 	}
-	return true
+	return nil
 }

pkg/cmd/remove_replication.go

@@ -86,7 +86,9 @@ func (o *FinalizeOptions) RemoveReplication() error {
 	}
 	if *repSource.Spec.Rsync.Address != *repDest.Status.Rsync.Address {
 		klog.Info("Refusing to remove replication, source and destination do not match")
-		return fmt.Errorf("Source RsyncAddress: %v does not match Destination RsyncAddress: %v", *repSource.Spec.Rsync.Address, *repDest.Status.Rsync.Address)
+		return fmt.Errorf(
+			"Source RsyncAddress: %v does not match Destination RsyncAddress: %v",
+			*repSource.Spec.Rsync.Address, *repDest.Status.Rsync.Address)
 	}
 	sshSecret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{

pkg/cmd/scribe.go

@@ -91,9 +91,12 @@ type ScribeDestinationOptions struct {
 //nolint:lll
 func (o *ScribeSourceOptions) Bind(cmd *cobra.Command, v *viper.Viper) {
 	flags := cmd.Flags()
-	flags.StringVar(&o.KubeContext, "source-kube-context", o.KubeContext, "the name of the kubeconfig context to use for the destination cluster. Defaults to current-context.")
-	flags.StringVar(&o.KubeClusterName, "source-kube-clustername", o.KubeClusterName, "the name of the kubeconfig cluster to use for the destination cluster. Defaults to current cluster.")
-	flags.StringVar(&o.Namespace, "source-namespace", o.Namespace, "the transfer source namespace and/or location of a ReplicationSource. This namespace must exist. Defaults to current namespace.")
+	flags.StringVar(&o.KubeContext, "source-kube-context", o.KubeContext, ""+
+		"the name of the kubeconfig context to use for the destination cluster. Defaults to current-context.")
+	flags.StringVar(&o.KubeClusterName, "source-kube-clustername", o.KubeClusterName, ""+
+		"the name of the kubeconfig cluster to use for the destination cluster. Defaults to current cluster.")
+	flags.StringVar(&o.Namespace, "source-namespace", o.Namespace, ""+
+		"the transfer source namespace and/or location of a ReplicationSource. This namespace must exist. Defaults to current namespace.")
 	flags.VisitAll(func(f *pflag.Flag) {
 		// Apply the viper config value to the flag when the flag is not set and viper has a value
 		if v.IsSet(f.Name) {
@@ -106,9 +109,12 @@ func (o *ScribeSourceOptions) Bind(cmd *cobra.Command, v *viper.Viper) {
 //nolint:lll
 func (o *ScribeDestinationOptions) Bind(cmd *cobra.Command, v *viper.Viper) {
 	flags := cmd.Flags()
-	flags.StringVar(&o.KubeContext, "dest-kube-context", o.KubeContext, "the name of the kubeconfig context to use for the destination cluster. Defaults to current-context.")
-	flags.StringVar(&o.KubeClusterName, "dest-kube-clustername", o.KubeClusterName, "the name of the kubeconfig cluster to use for the destination cluster. Defaults to current-cluster.")
-	flags.StringVar(&o.Namespace, "dest-namespace", o.Namespace, "the transfer destination namespace and/or location of a ReplicationDestination. This namespace must exist. Defaults to current namespace.")
+	flags.StringVar(&o.KubeContext, "dest-kube-context", o.KubeContext, ""+
+		"the name of the kubeconfig context to use for the destination cluster. Defaults to current-context.")
+	flags.StringVar(&o.KubeClusterName, "dest-kube-clustername", o.KubeClusterName, ""+
+		"the name of the kubeconfig cluster to use for the destination cluster. Defaults to current-cluster.")
+	flags.StringVar(&o.Namespace, "dest-namespace", o.Namespace, ""+
+		"the transfer destination namespace and/or location of a ReplicationDestination. This namespace must exist. Defaults to current namespace.")
 	flags.VisitAll(func(f *pflag.Flag) {
 		// Apply the viper config value to the flag when the flag is not set and viper has a value
 		if v.IsSet(f.Name) {
@@ -121,7 +127,9 @@ func (o *ScribeDestinationOptions) Bind(cmd *cobra.Command, v *viper.Viper) {
 //nolint:lll
 func (o *Config) bindFlags(cmd *cobra.Command, v *viper.Viper) {
 	flags := cmd.Flags()
-	flags.StringVar(&o.config, "config", o.config, "the path to file holding flag values. If empty, looks for ./config.yaml then ~/.scribeconfig/config.yaml. Command line values override config file.")
+	flags.StringVar(&o.config, "config", o.config, ""+
+		"the path to file holding flag values. If empty, looks for ./config.yaml then ~/.scribeconfig/config.yaml. "+
+		"Command line values override config file.")
 	flags.VisitAll(func(f *pflag.Flag) {
 		// Apply the viper config value to the flag when the flag is not set and viper has a value
 		if v.IsSet(f.Name) {

pkg/cmd/set_replication.go

@@ -91,10 +91,13 @@ func (o *FinalizeOptions) bindFlags(cmd *cobra.Command, v *viper.Viper) error {
 	flags := cmd.Flags()
 	flags.StringVar(&o.sourceName, "source-replication-name", o.sourceName, "name of ReplicationSource (default '<source-ns>-source')")
 	flags.StringVar(&o.destName, "dest-replication-name", o.destName, "name of ReplicationDestination (default '<dest-ns>-destination') ")
-	flags.StringVar(&o.destPVC, "dest-pvc", o.destPVC, "name of not-yet-existing destination PVC. Default is sourcePVC name, or if PVC with sourcePVC name exists in destination namespace, then 'sourcePVC-<date-tag>'")
+	flags.StringVar(&o.destPVC, "dest-pvc", o.destPVC, "name of not-yet-existing destination PVC. "+
+		"Default is sourcePVC name, or if PVC with sourcePVC name exists in destination namespace, then 'sourcePVC-<date-tag>'")
 	flags.StringVar(&o.destCapacity, "dest-capacity", o.destCapacity, "size of the destination volume to create. Default is source volume capacity.")
-	flags.StringVar(&o.destStorageClass, "dest-storage-class", o.destStorageClass, "name of the StorageClass of the destination volume. If not set, the default StorageClass will be used.")
-	flags.DurationVar(&o.timeout, "timeout", time.Minute*5, "length of time to wait for final sync to complete. Default is 5m. Pass values as time unit (e.g. 1,, 2m, 3h)")
+	flags.StringVar(&o.destStorageClass, "dest-storage-class", o.destStorageClass, ""+
+		"name of the StorageClass of the destination volume. If not set, the default StorageClass will be used.")
+	flags.DurationVar(&o.timeout, "timeout", time.Minute*5, "length of time to wait for final sync to complete. "+
+		"Default is 5m. Pass values as time unit (e.g. 1,, 2m, 3h)")
 	flags.VisitAll(func(f *pflag.Flag) {
 		if !f.Changed && v.IsSet(f.Name) {
 			val := v.Get(f.Name)

pkg/cmd/sync_secret.go

@@ -23,7 +23,8 @@ type SSHKeysSecretOptions struct {
 //nolint:lll
 func (o *SSHKeysSecretOptions) Bind(cmd *cobra.Command, v *viper.Viper) {
 	flags := cmd.Flags()
-	flags.StringVar(&o.SSHKeysSecret, "ssh-keys-secret", o.SSHKeysSecret, "name of existing valid SSHKeys secret for authentication. If not set, the dest-src SSHKey secret-name will be used from destinationlocation.")
+	flags.StringVar(&o.SSHKeysSecret, "ssh-keys-secret", o.SSHKeysSecret, ""+
+		"name of existing valid SSHKeys secret for authentication. If not set, the dest-src SSHKey secret-name will be used from destinationlocation.")
 
 	flags.VisitAll(func(f *pflag.Flag) {
 		if !f.Changed && v.IsSet(f.Name) {