Add review suggestions

redhat-developer · Feb 23, 2022 · f3c7cf4 · f3c7cf4
1 parent de1d39b
commit f3c7cf4
Show file tree

Hide file tree

Showing 3 changed files with 91 additions and 84 deletions.
diff --git a/pkg/init/asker/asker.go b/pkg/init/asker/asker.go
@@ -93,39 +93,8 @@ func (o *Survey) AskCorrect() (bool, error) {
 }
 
 // AskPersonalizeConfiguration asks the configuration user wants to change
-func (o *Survey) AskPersonalizeConfiguration(configuration ContainerConfiguration) (ContainerMap, error) {
-	options := []string{
-		"NOTHING - configuration is correct",
-	}
-	tracker := []ContainerMap{{Ops: "Nothing"}}
-	for _, port := range configuration.Ports {
-		options = append(options, fmt.Sprintf("Delete port %q", port))
-		tracker = append(tracker, ContainerMap{
-			Ops:  "Delete",
-			Kind: "Port",
-			Key:  port,
-		})
-	}
-	options = append(options, "Add new port")
-	tracker = append(tracker, ContainerMap{
-		Ops:  "Add",
-		Kind: "Port",
-	})
-
-	for key := range configuration.Envs {
-		options = append(options, fmt.Sprintf("Delete environment variable %q", key))
-		tracker = append(tracker, ContainerMap{
-			Ops:  "Delete",
-			Kind: "EnvVar",
-			Key:  key,
-		})
-	}
-	options = append(options, "Add new environment variable")
-	tracker = append(tracker, ContainerMap{
-		Ops:  "Add",
-		Kind: "EnvVar",
-	})
-
+func (o *Survey) AskPersonalizeConfiguration(configuration ContainerConfiguration) (OperationOnContainer, error) {
+	options, tracker := buildPersonalizedConfigurationOptions(configuration)
 	configChangeQuestion := &survey.Select{
 		Message: "What configuration do you want change?",
 		Default: options[0],
@@ -134,7 +103,7 @@ func (o *Survey) AskPersonalizeConfiguration(configuration ContainerConfiguratio
 	var configChangeIndex int
 	err := survey.AskOne(configChangeQuestion, &configChangeIndex)
 	if err != nil {
-		return ContainerMap{}, err
+		return OperationOnContainer{}, err
 	}
 	return tracker[configChangeIndex], nil
 }
@@ -188,24 +157,50 @@ func (o *Survey) AskContainerName(containers []string) (string, error) {
 	return selectContainerAnswer, nil
 }
 
-type ContainerConfiguration struct {
-	Ports []string
-	Envs  map[string]string
-}
-
-type ContainerMap struct {
-	Ops  string
-	Kind string
-	Key  string
-}
-
-// key is container name
-type DevfileConfiguration map[string]ContainerConfiguration
-
 func (dc *DevfileConfiguration) GetContainers() []string {
 	keys := []string{}
 	for k := range *dc {
 		keys = append(keys, k)
 	}
 	return keys
 }
+
+func buildPersonalizedConfigurationOptions(configuration ContainerConfiguration) (options []string, tracker []OperationOnContainer) {
+	options = []string{
+		"NOTHING - configuration is correct",
+	}
+	// track the option selected by the user without relying on the UI message
+	tracker = []OperationOnContainer{{Ops: "Nothing"}}
+
+	// Add available ports
+	for _, port := range configuration.Ports {
+		options = append(options, fmt.Sprintf("Delete port %q", port))
+		tracker = append(tracker, OperationOnContainer{
+			Ops:  "Delete",
+			Kind: "Port",
+			Key:  port,
+		})
+	}
+	options = append(options, "Add new port")
+	tracker = append(tracker, OperationOnContainer{
+		Ops:  "Add",
+		Kind: "Port",
+	})
+
+	// Add available env vars
+	for key := range configuration.Envs {
+		options = append(options, fmt.Sprintf("Delete environment variable %q", key))
+		tracker = append(tracker, OperationOnContainer{
+			Ops:  "Delete",
+			Kind: "EnvVar",
+			Key:  key,
+		})
+	}
+	options = append(options, "Add new environment variable")
+	tracker = append(tracker, OperationOnContainer{
+		Ops:  "Add",
+		Kind: "EnvVar",
+	})
+
+	return
+}
diff --git a/pkg/init/asker/interface.go b/pkg/init/asker/interface.go
@@ -28,11 +28,25 @@ type Asker interface {
 	AskContainerName(containers []string) (string, error)
 
 	// AskPersonalizeConfiguration asks the configuration user wants to change
-	AskPersonalizeConfiguration(configuration ContainerConfiguration) (ContainerMap, error)
+	AskPersonalizeConfiguration(configuration ContainerConfiguration) (OperationOnContainer, error)
 
 	// AskAddEnvVar asks the key and value for env var
 	AskAddEnvVar() (string, string, error)
 
 	// AskAddPort asks the container name and port that user wants to add
 	AskAddPort() (string, error)
 }
+
+type ContainerConfiguration struct {
+	Ports []string
+	Envs  map[string]string
+}
+
+type OperationOnContainer struct {
+	Ops  string
+	Kind string
+	Key  string
+}
+
+// key is container name
+type DevfileConfiguration map[string]ContainerConfiguration
diff --git a/pkg/init/backend/interactive.go b/pkg/init/backend/interactive.go
@@ -109,29 +109,11 @@ func (o *InteractiveBackend) PersonalizeName(devfile parser.DevfileObj, flags ma
 
 func (o *InteractiveBackend) PersonalizeDevfileconfig(devfileobj parser.DevfileObj) error {
 	// TODO: Add tests
-	// TODO: Add mock methods
-	var config = asker.DevfileConfiguration{}
-	components, err := devfileobj.Data.GetComponents(parsercommon.DevfileOptions{})
+	config, err := getPortsAndEnvVar(devfileobj)
 	if err != nil {
 		return err
 	}
-	for _, component := range components {
-		var ports = []string{}
-		var envMap = map[string]string{}
-		if component.Container != nil {
-			// Fix this for component that are not a container
-			for _, ep := range component.Container.Endpoints {
-				ports = append(ports, strconv.Itoa(ep.TargetPort))
-			}
-			for _, env := range component.Container.Env {
-				envMap[env.Name] = env.Value
-			}
-		}
-		config[component.Name] = asker.ContainerConfiguration{
-			Ports: ports,
-			Envs:  envMap,
-		}
-	}
+
 	var selectContainerAnswer string
 	containerOptions := config.GetContainers()
 	containerOptions = append(containerOptions, "NONE - configuration is correct")
@@ -148,14 +130,14 @@ func (o *InteractiveBackend) PersonalizeDevfileconfig(devfileobj parser.DevfileO
 			break
 		}
 
-		var configOps asker.ContainerMap
+		var configOps asker.OperationOnContainer
 		for configOps.Ops != "Nothing" {
 			configOps, err = o.asker.AskPersonalizeConfiguration(selectedContainer)
 			if err != nil {
 				return err
 			}
-
-			if configOps.Ops == "Delete" && configOps.Kind == "Port" {
+			switch {
+			case configOps.Ops == "Delete" && configOps.Kind == "Port":
 
 				portToDelete := configOps.Key
 				indexToDelete := -1
@@ -172,8 +154,8 @@ func (o *InteractiveBackend) PersonalizeDevfileconfig(devfileobj parser.DevfileO
 					return err
 				}
 				selectedContainer.Ports = append(selectedContainer.Ports[:indexToDelete], selectedContainer.Ports[indexToDelete+1:]...)
-			} else if configOps.Ops == "Delete" && configOps.Kind == "EnvVar" {
 
+			case configOps.Ops == "Delete" && configOps.Kind == "EnvVar":
 				envToDelete := configOps.Key
 				if _, ok := selectedContainer.Envs[envToDelete]; !ok {
 					log.Warningf(fmt.Sprintf("unable to delete env %q, not found", envToDelete))
@@ -184,8 +166,7 @@ func (o *InteractiveBackend) PersonalizeDevfileconfig(devfileobj parser.DevfileO
 				}
 				delete(selectedContainer.Envs, envToDelete)
 
-			} else if configOps.Ops == "Add" && configOps.Kind == "Port" {
-
+			case configOps.Ops == "Add" && configOps.Kind == "Port":
 				var newPort string
 				newPort, err = o.asker.AskAddPort()
 				if err != nil {
@@ -198,7 +179,7 @@ func (o *InteractiveBackend) PersonalizeDevfileconfig(devfileobj parser.DevfileO
 				}
 				selectedContainer.Ports = append(selectedContainer.Ports, newPort)
 
-			} else if configOps.Ops == "Add" && configOps.Kind == "EnvVar" {
+			case configOps.Ops == "Add" && configOps.Kind == "EnvVar":
 
 				var newEnvNameAnswer, newEnvValueAnswer string
 				newEnvNameAnswer, newEnvValueAnswer, err = o.asker.AskAddEnvVar()
@@ -214,9 +195,9 @@ func (o *InteractiveBackend) PersonalizeDevfileconfig(devfileobj parser.DevfileO
 				}
 				selectedContainer.Envs[newEnvNameAnswer] = newEnvValueAnswer
 
-			} else if configOps.Ops == "Nothing" {
-				// nothing to do
-			} else {
+			case configOps.Ops == "Nothing":
+				continue
+			default:
 				return fmt.Errorf("Unknown configuration selected %q", fmt.Sprintf("%v %v %v", configOps.Ops, configOps.Kind, configOps.Key))
 			}
 			// Update the current configuration
@@ -245,11 +226,28 @@ func PrintConfiguration(config asker.DevfileConfiguration) {
 	}
 }
 
-func InArray(arr []string, element string) bool {
-	for _, ele := range arr {
-		if ele == element {
-			return true
+func getPortsAndEnvVar(obj parser.DevfileObj) (asker.DevfileConfiguration, error) {
+	var config = asker.DevfileConfiguration{}
+	components, err := obj.Data.GetComponents(parsercommon.DevfileOptions{})
+	if err != nil {
+		return config, err
+	}
+	for _, component := range components {
+		var ports = []string{}
+		var envMap = map[string]string{}
+		if component.Container != nil {
+			// Fix this for component that are not a container
+			for _, ep := range component.Container.Endpoints {
+				ports = append(ports, strconv.Itoa(ep.TargetPort))
+			}
+			for _, env := range component.Container.Env {
+				envMap[env.Name] = env.Value
+			}
+		}
+		config[component.Name] = asker.ContainerConfiguration{
+			Ports: ports,
+			Envs:  envMap,
 		}
 	}
-	return false
+	return config, nil
 }