Preference cleanup (1/n) (redhat-developer#5822)

* Preference(Timeout) int > time.Duration

* Fix odo preference --help for unset examples

* Preference(PushTimeout) int > time.Duration

* Change set, and unset messages

* Preference(RegistryCacheTime) int > time.Duration

* use cobra.ExactArgs for set, and unset

* mockgen

* Unit tests and integration tests

* Fix unit test failure

* Update k8s.io/utils pkg

* Philippe's review

* Fix error message

* Philippe's review

Signed-off-by: Parthvi Vala <[email protected]>

* Add minimum acceptable value for preferences accepting time.Difference type

* Fix unit test failure

Signed-off-by: Parthvi Vala <[email protected]>

* Add migration plan with a warning, add better error message for incompatible formats

Signed-off-by: Parthvi Vala <[email protected]>

go.mod

@@ -55,7 +55,7 @@ require (
 	k8s.io/klog v1.0.0
 	k8s.io/klog/v2 v2.10.0
 	k8s.io/kubectl v0.22.1
-	k8s.io/utils v0.0.0-20210819203725-bdf08cb9a70a
+	k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9
 	sigs.k8s.io/controller-runtime v0.10.2
 	sigs.k8s.io/yaml v1.3.0
 )
@@ -83,7 +83,6 @@ require (
 	github.com/containerd/containerd v1.5.2 // indirect
 	github.com/danieljoos/wincred v1.1.0 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
-	github.com/deislabs/oras v0.8.1 // indirect
 	github.com/docker/docker v20.10.3+incompatible // indirect
 	github.com/emicklei/dot v0.15.0 // indirect
 	github.com/emirpasic/gods v1.12.0 // indirect
@@ -115,7 +114,6 @@ require (
 	github.com/hashicorp/errwrap v1.0.0 // indirect
 	github.com/hashicorp/go-multierror v1.1.1 // indirect
 	github.com/hashicorp/go-version v1.4.0 // indirect
-	github.com/hashicorp/hcl v1.0.0 // indirect
 	github.com/hpcloud/tail v1.0.0 // indirect
 	github.com/imdario/mergo v0.3.12 // indirect
 	github.com/inconshreveable/mousetrap v1.0.0 // indirect
@@ -127,15 +125,13 @@ require (
 	github.com/kevinburke/ssh_config v0.0.0-20201106050909-4977a11b4351 // indirect
 	github.com/klauspost/compress v1.13.1 // indirect
 	github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de // indirect
-	github.com/magiconair/properties v1.8.1 // indirect
 	github.com/mailru/easyjson v0.7.6 // indirect
 	github.com/mattn/go-isatty v0.0.14 // indirect
 	github.com/mattn/go-runewidth v0.0.13 // indirect
 	github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369 // indirect
 	github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d // indirect
 	github.com/mitchellh/go-homedir v1.1.0 // indirect
 	github.com/mitchellh/go-wordwrap v1.0.0 // indirect
-	github.com/mitchellh/mapstructure v1.1.2 // indirect
 	github.com/mitchellh/reflectwalk v1.0.1 // indirect
 	github.com/moby/locker v1.0.1 // indirect
 	github.com/moby/spdystream v0.2.0 // indirect
@@ -147,7 +143,6 @@ require (
 	github.com/opencontainers/go-digest v1.0.0 // indirect
 	github.com/opencontainers/image-spec v1.0.2-0.20190823105129-775207bd45b6 // indirect
 	github.com/openshift/library-go v0.0.0-20210923120925-caee30353c0d // indirect
-	github.com/pelletier/go-toml v1.8.1 // indirect
 	github.com/peterbourgon/diskv v2.0.1+incompatible // indirect
 	github.com/pkg/errors v0.9.1 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
@@ -160,11 +155,7 @@ require (
 	github.com/segmentio/backo-go v0.0.0-20200129164019-23eae7c10bd3 // indirect
 	github.com/sergi/go-diff v1.1.0 // indirect
 	github.com/sirupsen/logrus v1.8.1 // indirect
-	github.com/spf13/cast v1.3.0 // indirect
-	github.com/spf13/jwalterweatherman v1.0.0 // indirect
-	github.com/spf13/viper v1.7.1 // indirect
 	github.com/stretchr/testify v1.7.0 // indirect
-	github.com/subosito/gotenv v1.2.0 // indirect
 	github.com/tidwall/match v1.1.1 // indirect
 	github.com/tidwall/pretty v1.2.0 // indirect
 	github.com/xanzy/ssh-agent v0.3.0 // indirect
@@ -192,7 +183,6 @@ require (
 	google.golang.org/protobuf v1.26.0 // indirect
 	gopkg.in/fsnotify.v1 v1.4.7 // indirect
 	gopkg.in/inf.v0 v0.9.1 // indirect
-	gopkg.in/ini.v1 v1.51.0 // indirect
 	gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect
 	gopkg.in/warnings.v0 v0.1.2 // indirect
 	gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect

pkg/devfile/adapters/kubernetes/component/adapter.go

@@ -3,7 +3,6 @@ package component
 import (
 	"fmt"
 	"io"
-	"time"
 
 	"k8s.io/utils/pointer"
 
@@ -53,7 +52,7 @@ func (a *Adapter) getPod(refresh bool) (*corev1.Pod, error) {
 		podSelector := fmt.Sprintf("component=%s", a.ComponentName)
 
 		// Wait for Pod to be in running state otherwise we can't sync data to it.
-		pod, err := a.kubeClient.WaitAndGetPodWithEvents(podSelector, corev1.PodRunning, time.Duration(a.prefClient.GetPushTimeout())*time.Second)
+		pod, err := a.kubeClient.WaitAndGetPodWithEvents(podSelector, corev1.PodRunning, a.prefClient.GetPushTimeout())
 		if err != nil {
 			return nil, fmt.Errorf("error while waiting for pod %s: %w", podSelector, err)
 		}

pkg/devfile/adapters/kubernetes/component/adapter_test.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"reflect"
 	"testing"
+	"time"
 
 	"github.com/devfile/library/pkg/devfile/parser/data"
 	"github.com/golang/mock/gomock"
@@ -441,7 +442,7 @@ func TestWaitAndGetComponentPod(t *testing.T) {
 
 			ctrl := gomock.NewController(t)
 			prefClient := preference.NewMockClient(ctrl)
-			prefClient.EXPECT().GetPushTimeout().Return(10)
+			prefClient.EXPECT().GetPushTimeout().Return(100 * time.Second)
 			componentAdapter := New(adapterCtx, fkclient, prefClient)
 			_, err := componentAdapter.getPod(false)
 

pkg/kclient/pods_test.go

@@ -6,9 +6,10 @@ import (
 	"testing"
 	"time"
 
-	"github.com/redhat-developer/odo/pkg/preference"
 	"k8s.io/apimachinery/pkg/runtime"
 
+	"github.com/redhat-developer/odo/pkg/preference"
+
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
@@ -81,7 +82,7 @@ func TestWaitAndGetPodWithEvents(t *testing.T) {
 
 			podSelector := fmt.Sprintf("deploymentconfig=%s", tt.podName)
 
-			pod, err := fakeClient.WaitAndGetPodWithEvents(podSelector, corev1.PodRunning, preference.DefaultPushTimeout*time.Second)
+			pod, err := fakeClient.WaitAndGetPodWithEvents(podSelector, corev1.PodRunning, preference.DefaultPushTimeout)
 
 			if !tt.wantErr == (err != nil) {
 				t.Errorf("client.WaitAndGetPod(string) unexpected error %v, wantErr %v", err, tt.wantErr)

pkg/odo/cli/preference/set.go

@@ -13,9 +13,10 @@ import (
 	"github.com/redhat-developer/odo/pkg/odo/genericclioptions/clientset"
 	"github.com/redhat-developer/odo/pkg/preference"
 
-	"github.com/redhat-developer/odo/pkg/odo/genericclioptions"
 	"github.com/spf13/cobra"
 	ktemplates "k8s.io/kubectl/pkg/util/templates"
+
+	"github.com/redhat-developer/odo/pkg/odo/genericclioptions"
 )
 
 const setCommandName = "set"
@@ -79,7 +80,7 @@ func (o *SetOptions) Run(ctx context.Context) (err error) {
 		return err
 	}
 
-	log.Info("Global preference was successfully updated")
+	log.Successf("Value of '%s' preference was set to '%s'", o.paramName, o.paramValue)
 	return nil
 }
 
@@ -98,23 +99,12 @@ func NewCmdSet(name, fullName string) *cobra.Command {
 			properties := prefClient.NewPreferenceList()
 			for _, property := range properties.Items {
 				value := property.Default
-				if value == "" {
-					value = "foobar"
-				}
 				exampleString += fmt.Sprintf("\n  %s %s %v", fullName, property.Name, value)
 			}
 			return "\n" + exampleString
 		}(setExample, fullName),
-		Args: func(cmd *cobra.Command, args []string) error {
-			if len(args) < 2 {
-				return fmt.Errorf("please provide a parameter name and value")
-			} else if len(args) > 2 {
-				return fmt.Errorf("only one value per parameter is allowed")
-			} else {
-				return nil
-			}
-
-		}, Run: func(cmd *cobra.Command, args []string) {
+		Args: cobra.ExactArgs(2),
+		Run: func(cmd *cobra.Command, args []string) {
 			genericclioptions.GenericRun(o, cmd, args)
 		},
 	}

pkg/odo/cli/preference/unset.go

@@ -2,7 +2,6 @@ package preference
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"strings"
 
@@ -12,9 +11,10 @@ import (
 	"github.com/redhat-developer/odo/pkg/odo/genericclioptions"
 	"github.com/redhat-developer/odo/pkg/odo/genericclioptions/clientset"
 
-	"github.com/redhat-developer/odo/pkg/preference"
 	"github.com/spf13/cobra"
 	ktemplates "k8s.io/kubectl/pkg/util/templates"
+
+	"github.com/redhat-developer/odo/pkg/preference"
 )
 
 const unsetCommandName = "unset"
@@ -33,7 +33,7 @@ type UnsetOptions struct {
 	// Clients
 	clientset *clientset.Clientset
 
-	//Parameters
+	// Parameters
 	paramName string
 
 	// Flags
@@ -71,7 +71,7 @@ func (o *UnsetOptions) Run(ctx context.Context) (err error) {
 				return nil
 			}
 		} else {
-			return errors.New("preference already unset, cannot unset a preference which is not set")
+			return fmt.Errorf("value of '%s' is already unset", o.paramName)
 		}
 	}
 
@@ -80,7 +80,7 @@ func (o *UnsetOptions) Run(ctx context.Context) (err error) {
 		return err
 	}
 
-	log.Info("Global preference was successfully updated")
+	log.Successf("Value of '%s' preference was removed from preferences. Its default value will be used.", o.paramName)
 	return nil
 
 }
@@ -94,19 +94,15 @@ func NewCmdUnset(name, fullName string) *cobra.Command {
 		Long:  fmt.Sprintf(unsetLongDesc, preference.FormatSupportedParameters()),
 		Example: func(exampleString, fullName string) string {
 			// Just show one example of how to unset a value.
-			exampleString += fmt.Sprintf("\n  %s %s", fullName, preference.GetSupportedParameters()[0])
-			return "\n" + exampleString
-		}(unsetExample, fullName),
-		Args: func(cmd *cobra.Command, args []string) error {
-			if len(args) < 1 {
-				return fmt.Errorf("please provide a parameter name")
-			} else if len(args) > 1 {
-				return fmt.Errorf("only one parameter is allowed")
-			} else {
-				return nil
+			parameters := preference.GetSupportedParameters()
+			for _, param := range parameters {
+				exampleString += fmt.Sprintf("\n  %s %s", fullName, param)
 			}
 
-		}, Run: func(cmd *cobra.Command, args []string) {
+			return "\n" + exampleString
+		}(unsetExample, fullName),
+		Args: cobra.ExactArgs(1),
+		Run: func(cmd *cobra.Command, args []string) {
 			genericclioptions.GenericRun(o, cmd, args)
 		},
 	}

pkg/odo/cli/preference/unset_test.go

@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/golang/mock/gomock"
+
 	"github.com/redhat-developer/odo/pkg/odo/cmdline"
 	"github.com/redhat-developer/odo/pkg/odo/genericclioptions/clientset"
 	"github.com/redhat-developer/odo/pkg/preference"
@@ -27,7 +28,7 @@ func TestUnsetForce(t *testing.T) {
 			name:           "no force and parameter not exists",
 			forceFlag:      false,
 			exists:         false,
-			expectedRunErr: "preference already unset",
+			expectedRunErr: "is already unset",
 		},
 	}
 	for _, tt := range tests {

pkg/odo/cli/preference/view_test.go

@@ -3,6 +3,7 @@ package preference
 import (
 	"context"
 	"testing"
+	"time"
 
 	"github.com/golang/mock/gomock"
 
@@ -36,10 +37,11 @@ func TestView(t *testing.T) {
 		return
 	}
 
+	timeValue := 10 * time.Second
 	prefClient.EXPECT().UpdateNotification().Return(pointer.Bool(false))
-	prefClient.EXPECT().Timeout().Return(pointer.Int(10))
-	prefClient.EXPECT().RegistryCacheTime().Return(pointer.Int(240))
-	prefClient.EXPECT().PushTimeout().Return(pointer.Int(10))
+	prefClient.EXPECT().Timeout().Return(&timeValue)
+	prefClient.EXPECT().RegistryCacheTime().Return(&timeValue)
+	prefClient.EXPECT().PushTimeout().Return(&timeValue)
 	prefClient.EXPECT().EphemeralSourceVolume().Return(pointer.Bool(false))
 	prefClient.EXPECT().ConsentTelemetry().Return(pointer.Bool(false))
 

pkg/odo/cli/version/version.go

@@ -14,10 +14,11 @@ import (
 	"github.com/redhat-developer/odo/pkg/preference"
 	odoversion "github.com/redhat-developer/odo/pkg/version"
 
-	"github.com/redhat-developer/odo/pkg/odo/util"
 	"github.com/spf13/cobra"
 	"k8s.io/klog"
 	ktemplates "k8s.io/kubectl/pkg/util/templates"
+
+	"github.com/redhat-developer/odo/pkg/odo/util"
 )
 
 // RecommendedCommandName is the recommended version command name
@@ -63,11 +64,11 @@ func (o *VersionOptions) Complete(cmdline cmdline.Cmdline, args []string) (err e
 			// checking the value of timeout in preference
 			var timeout time.Duration
 			if o.clientset.PreferenceClient != nil {
-				timeout = time.Duration(o.clientset.PreferenceClient.GetTimeout()) * time.Second
+				timeout = o.clientset.PreferenceClient.GetTimeout()
 			} else {
 				// the default timeout will be used
 				// when the value is not readable from preference
-				timeout = preference.DefaultTimeout * time.Second
+				timeout = preference.DefaultTimeout
 			}
 			o.serverInfo, _ = client.GetServerVersion(timeout)
 		}

pkg/preference/errors.go

@@ -0,0 +1,11 @@
+package preference
+
+type MinimumDurationValueError struct{}
+
+func NewMinimumDurationValueError() MinimumDurationValueError {
+	return MinimumDurationValueError{}
+}
+
+func (v MinimumDurationValueError) Error() string {
+	return "value must be a positive Duration (e.g. 4s, 5m, 1h); minimum value: 1s"
+}