Refactor code and fix build (#1228)

pkg/loader/compose/v1v2.go

@@ -261,40 +261,8 @@ func libComposeToKomposeMapping(composeObject *project.Project) (kobject.Kompose
 		// canonical "Custom Labels" handler
 		// Labels used to influence conversion of kompose will be handled
 		// from here for docker-compose. Each loader will have such handler.
-		serviceConfig.Labels = make(map[string]string)
+		if err := parseKomposeLabels(composeServiceConfig.Labels, &serviceConfig); err != nil {
-		for key, value := range composeServiceConfig.Labels {
+			return kobject.KomposeObject{}, err
-			switch key {
-			case LabelServiceType:
-				serviceType, err := handleServiceType(value)
-				if err != nil {
-					return kobject.KomposeObject{}, errors.Wrap(err, "handleServiceType failed")
-				}
-
-				serviceConfig.ServiceType = serviceType
-			case LabelServiceExpose:
-				serviceConfig.ExposeService = strings.Trim(strings.ToLower(value), " ,")
-			case LabelServiceExposeTLSSecret:
-				serviceConfig.ExposeServiceTLS = value
-			case LabelNodePortPort:
-				serviceConfig.NodePortPort = cast.ToInt32(value)
-			case LabelImagePullSecret:
-				serviceConfig.ImagePullSecret = value
-			case LabelImagePullPolicy:
-				serviceConfig.ImagePullPolicy = value
-			default:
-				serviceConfig.Labels[key] = value
-			}
-		}
-		if serviceConfig.ExposeService == "" && serviceConfig.ExposeServiceTLS != "" {
-			return kobject.KomposeObject{}, errors.New("kompose.service.expose.tls-secret was specified without kompose.service.expose")
-		}
-
-		if serviceConfig.ServiceType != string(api.ServiceTypeNodePort) && serviceConfig.NodePortPort != 0 {
-			return kobject.KomposeObject{}, errors.New("kompose.service.type must be nodeport when assign node port value")
-		}
-
-		if len(serviceConfig.Port) > 1 && serviceConfig.NodePortPort != 0 {
-			return kobject.KomposeObject{}, errors.New("cannnot set kompose.service.nodeport.port when service has multiple ports")
 		}
 
 		err = checkLabelsPorts(len(serviceConfig.Port), composeServiceConfig.Labels[LabelServiceType], name)

pkg/loader/compose/v3.go

@@ -406,24 +406,8 @@ func dockerComposeToKomposeMapping(composeObject *types.Config) (kobject.Kompose
 		serviceConfig.BuildArgs = composeServiceConfig.Build.Args
 		serviceConfig.BuildLabels = composeServiceConfig.Build.Labels
 
-		// Gather the environment values
+		// env
-		// DockerCompose uses map[string]*string while we use []string
+		parseV3Environment(&composeServiceConfig, &serviceConfig)
-		// So let's convert that using this hack
-		// Note: unset env pick up the env value on host if exist
-		for name, value := range composeServiceConfig.Environment {
-			var env kobject.EnvVar
-			if value != nil {
-				env = kobject.EnvVar{Name: name, Value: *value}
-			} else {
-				result, ok := os.LookupEnv(name)
-				if ok {
-					env = kobject.EnvVar{Name: name, Value: result}
-				} else {
-					continue
-				}
-			}
-			serviceConfig.Environment = append(serviceConfig.Environment, env)
-		}
 
 		// Get env_file
 		serviceConfig.EnvFile = composeServiceConfig.EnvFile
@@ -440,41 +424,8 @@ func dockerComposeToKomposeMapping(composeObject *types.Config) (kobject.Kompose
 		// https://docs.docker.com/compose/compose-file/#long-syntax-3
 		serviceConfig.VolList = loadV3Volumes(composeServiceConfig.Volumes)
 
-		// Label handler
+		if err := parseKomposeLabels(composeServiceConfig.Labels, &serviceConfig); err != nil {
-		// Labels used to influence conversion of kompose will be handled
+			return kobject.KomposeObject{}, err
-		// from here for docker-compose. Each loader will have such handler.
-		for key, value := range composeServiceConfig.Labels {
-			switch key {
-			case LabelServiceType:
-				serviceType, err := handleServiceType(value)
-				if err != nil {
-					return kobject.KomposeObject{}, errors.Wrap(err, "handleServiceType failed")
-				}
-
-				serviceConfig.ServiceType = serviceType
-			case LabelServiceExpose:
-				serviceConfig.ExposeService = strings.Trim(strings.ToLower(value), " ,")
-			case LabelNodePortPort:
-				serviceConfig.NodePortPort = cast.ToInt32(value)
-			case LabelServiceExposeTLSSecret:
-				serviceConfig.ExposeServiceTLS = value
-			case LabelImagePullSecret:
-				serviceConfig.ImagePullSecret = value
-			case LabelImagePullPolicy:
-				serviceConfig.ImagePullPolicy = value
-			}
-		}
-
-		if serviceConfig.ExposeService == "" && serviceConfig.ExposeServiceTLS != "" {
-			return kobject.KomposeObject{}, errors.New("kompose.service.expose.tls-secret was specified without kompose.service.expose")
-		}
-
-		if serviceConfig.ServiceType != string(api.ServiceTypeNodePort) && serviceConfig.NodePortPort != 0 {
-			return kobject.KomposeObject{}, errors.New("kompose.service.type must be nodeport when assign node port value")
-		}
-
-		if len(serviceConfig.Port) > 1 && serviceConfig.NodePortPort != 0 {
-			return kobject.KomposeObject{}, errors.New("cannot set kompose.service.nodeport.port when service has multiple ports")
 		}
 
 		// Log if the name will been changed
@@ -496,6 +447,76 @@ func dockerComposeToKomposeMapping(composeObject *types.Config) (kobject.Kompose
 	return komposeObject, nil
 }
 
+func parseV3Environment(composeServiceConfig *types.ServiceConfig, serviceConfig *kobject.ServiceConfig) {
+	// Gather the environment values
+	// DockerCompose uses map[string]*string while we use []string
+	// So let's convert that using this hack
+	// Note: unset env pick up the env value on host if exist
+	for name, value := range composeServiceConfig.Environment {
+		var env kobject.EnvVar
+		if value != nil {
+			env = kobject.EnvVar{Name: name, Value: *value}
+		} else {
+			result, ok := os.LookupEnv(name)
+			if ok {
+				env = kobject.EnvVar{Name: name, Value: result}
+			} else {
+				continue
+			}
+		}
+		serviceConfig.Environment = append(serviceConfig.Environment, env)
+	}
+}
+
+// parseKomposeLabels parse kompose labels, also do some validation
+func parseKomposeLabels(labels map[string]string, serviceConfig *kobject.ServiceConfig) error {
+	// Label handler
+	// Labels used to influence conversion of kompose will be handled
+	// from here for docker-compose. Each loader will have such handler.
+
+	if serviceConfig.Labels == nil {
+		serviceConfig.Labels = make(map[string]string)
+	}
+
+	for key, value := range labels {
+		switch key {
+		case LabelServiceType:
+			serviceType, err := handleServiceType(value)
+			if err != nil {
+				return errors.Wrap(err, "handleServiceType failed")
+			}
+
+			serviceConfig.ServiceType = serviceType
+		case LabelServiceExpose:
+			serviceConfig.ExposeService = strings.Trim(strings.ToLower(value), " ,")
+		case LabelNodePortPort:
+			serviceConfig.NodePortPort = cast.ToInt32(value)
+		case LabelServiceExposeTLSSecret:
+			serviceConfig.ExposeServiceTLS = value
+		case LabelImagePullSecret:
+			serviceConfig.ImagePullSecret = value
+		case LabelImagePullPolicy:
+			serviceConfig.ImagePullPolicy = value
+		default:
+			serviceConfig.Labels[key] = value
+		}
+	}
+
+	if serviceConfig.ExposeService == "" && serviceConfig.ExposeServiceTLS != "" {
+		return errors.New("kompose.service.expose.tls-secret was specified without kompose.service.expose")
+	}
+
+	if serviceConfig.ServiceType != string(api.ServiceTypeNodePort) && serviceConfig.NodePortPort != 0 {
+		return errors.New("kompose.service.type must be nodeport when assign node port value")
+	}
+
+	if len(serviceConfig.Port) > 1 && serviceConfig.NodePortPort != 0 {
+		return errors.New("cannot set kompose.service.nodeport.port when service has multiple ports")
+	}
+
+	return nil
+}
+
 func handleV3Volume(komposeObject *kobject.KomposeObject, volumes *map[string]types.VolumeConfig) {
 	for name := range komposeObject.ServiceConfigs {
 		// retrieve volumes of service
@@ -714,20 +735,30 @@ func mergeComposeObject(oldCompose *types.Config, newCompose *types.Config) (*ty
 
 			// Store volumes by Target
 			volumeConfigsMap := make(map[string]types.ServiceVolumeConfig)
+			// map is not iterated in the same order.
+			// but why only this code cause test error?
+			var keys []string
+
 			// populate the older values
 			for _, volConfig := range tmpOldService.Volumes {
 				volumeConfigsMap[volConfig.Target] = volConfig
+				keys = append(keys, volConfig.Target)
 			}
 			// add the new values, overriding as needed
 			for _, volConfig := range service.Volumes {
+				if _, ok := volumeConfigsMap[volConfig.Target]; !ok {
+					keys = append(keys, volConfig.Target)
+				}
 				volumeConfigsMap[volConfig.Target] = volConfig
 			}
 
 			// get the new list of volume configs
 			var volumes []types.ServiceVolumeConfig
-			for _, volConfig := range volumeConfigsMap {
+
-				volumes = append(volumes, volConfig)
+			for _, key := range keys {
+				volumes = append(volumes, volumeConfigsMap[key])
 			}
+
 			tmpOldService.Volumes = volumes
 		}
 		if service.WorkingDir != "" {

pkg/transformer/kubernetes/k8sutils.go

@@ -491,38 +491,9 @@ func (k *Kubernetes) UpdateKubernetesObjects(name string, service kobject.Servic
 			}
 		}
 
-		// Configure the resource limits
+		TranslatePodResource(&service, template)
-		if service.MemLimit != 0 || service.CPULimit != 0 {
-			resourceLimit := api.ResourceList{}
-
-			if service.MemLimit != 0 {
-				resourceLimit[api.ResourceMemory] = *resource.NewQuantity(int64(service.MemLimit), "RandomStringForFormat")
-			}
-
-			if service.CPULimit != 0 {
-				resourceLimit[api.ResourceCPU] = *resource.NewMilliQuantity(service.CPULimit, resource.DecimalSI)
-			}
-
-			template.Spec.Containers[0].Resources.Limits = resourceLimit
-		}
-
-		// Configure the resource requests
-		if service.MemReservation != 0 || service.CPUReservation != 0 {
-			resourceRequests := api.ResourceList{}
-
-			if service.MemReservation != 0 {
-				resourceRequests[api.ResourceMemory] = *resource.NewQuantity(int64(service.MemReservation), "RandomStringForFormat")
-			}
-
-			if service.CPUReservation != 0 {
-				resourceRequests[api.ResourceCPU] = *resource.NewMilliQuantity(service.CPUReservation, resource.DecimalSI)
-			}
-
-			template.Spec.Containers[0].Resources.Requests = resourceRequests
-		}
 
 		// Configure resource reservations
-
 		podSecurityContext := &api.PodSecurityContext{}
 
 		//set pid namespace mode
@@ -570,28 +541,17 @@ func (k *Kubernetes) UpdateKubernetesObjects(name string, service kobject.Servic
 		template.ObjectMeta.Labels = transformer.ConfigLabelsWithNetwork(name, service.Network)
 
 		// Configure the image pull policy
-		switch service.ImagePullPolicy {
+		if policy, err := GetImagePullPolicy(name, service.ImagePullPolicy); err != nil {
-		case "":
+			return err
-		case "Always":
+		} else {
-			template.Spec.Containers[0].ImagePullPolicy = api.PullAlways
+			template.Spec.Containers[0].ImagePullPolicy = policy
-		case "Never":
-			template.Spec.Containers[0].ImagePullPolicy = api.PullNever
-		case "IfNotPresent":
-			template.Spec.Containers[0].ImagePullPolicy = api.PullIfNotPresent
-		default:
-			return errors.New("Unknown image-pull-policy " + service.ImagePullPolicy + " for service " + name)
 		}
 
 		// Configure the container restart policy.
-		switch service.Restart {
+		if restart, err := GetRestartPolicy(name, service.Restart); err != nil {
-		case "", "always", "any":
+			return err
-			template.Spec.RestartPolicy = api.RestartPolicyAlways
+		} else {
-		case "no", "none":
+			template.Spec.RestartPolicy = restart
-			template.Spec.RestartPolicy = api.RestartPolicyNever
-		case "on-failure":
-			template.Spec.RestartPolicy = api.RestartPolicyOnFailure
-		default:
-			return errors.New("Unknown restart policy " + service.Restart + " for service " + name)
 		}
 
 		// Configure hostname/domain_name settings
@@ -628,6 +588,70 @@ func (k *Kubernetes) UpdateKubernetesObjects(name string, service kobject.Servic
 	return nil
 }
 
+func TranslatePodResource(service *kobject.ServiceConfig, template *api.PodTemplateSpec) {
+	// Configure the resource limits
+	if service.MemLimit != 0 || service.CPULimit != 0 {
+		resourceLimit := api.ResourceList{}
+
+		if service.MemLimit != 0 {
+			resourceLimit[api.ResourceMemory] = *resource.NewQuantity(int64(service.MemLimit), "RandomStringForFormat")
+		}
+
+		if service.CPULimit != 0 {
+			resourceLimit[api.ResourceCPU] = *resource.NewMilliQuantity(service.CPULimit, resource.DecimalSI)
+		}
+
+		template.Spec.Containers[0].Resources.Limits = resourceLimit
+	}
+
+	// Configure the resource requests
+	if service.MemReservation != 0 || service.CPUReservation != 0 {
+		resourceRequests := api.ResourceList{}
+
+		if service.MemReservation != 0 {
+			resourceRequests[api.ResourceMemory] = *resource.NewQuantity(int64(service.MemReservation), "RandomStringForFormat")
+		}
+
+		if service.CPUReservation != 0 {
+			resourceRequests[api.ResourceCPU] = *resource.NewMilliQuantity(service.CPUReservation, resource.DecimalSI)
+		}
+
+		template.Spec.Containers[0].Resources.Requests = resourceRequests
+	}
+
+	return
+
+}
+
+func GetImagePullPolicy(name, policy string) (api.PullPolicy, error) {
+	switch policy {
+	case "":
+	case "Always":
+		return api.PullAlways, nil
+	case "Never":
+		return api.PullNever, nil
+	case "IfNotPresent":
+		return api.PullIfNotPresent, nil
+	default:
+		return "", errors.New("Unknown image-pull-policy " + policy + " for service " + name)
+	}
+	return "", nil
+
+}
+
+func GetRestartPolicy(name, restart string) (api.RestartPolicy, error) {
+	switch restart {
+	case "", "always", "any":
+		return api.RestartPolicyAlways, nil
+	case "no", "none":
+		return api.RestartPolicyNever, nil
+	case "on-failure":
+		return api.RestartPolicyOnFailure, nil
+	default:
+		return "", errors.New("Unknown restart policy " + restart + " for service " + name)
+	}
+}
+
 // SortServicesFirst - the objects that we get can be in any order this keeps services first
 // according to best practice kubernetes services should be created first
 // http://kubernetes.io/docs/user-guide/config-best-practices/