Vars handling centralized (#1934)

Vars file handling centralized

We have the same coding for handling varsf-files and vars. With that change we shift to having one common coding for that

cmd/cloudFoundryCreateService.go

@@ -74,24 +74,22 @@ func cloudFoundryCreateServiceRequest(config *cloudFoundryCreateServiceOptions,
 		cfCreateServiceScript = append(cfCreateServiceScript, "-t", config.CfServiceTags)
 	}
 	if config.ServiceManifest != "" && fileExists(config.ServiceManifest) {
-		var varPart []string
 
 		cfCreateServiceScript = []string{"create-service-push", "--no-push", "--service-manifest", config.ServiceManifest}
 
 		if len(config.ManifestVariablesFiles) >= 0 {
-			for _, v := range config.ManifestVariablesFiles {
-				if fileExists(v) {
-					cfCreateServiceScript = append(cfCreateServiceScript, "--vars-file", v)
-				} else {
-					return fmt.Errorf("Failed to append Manifest Variables File: %w", errors.New(v+" is not a file"))
-				}
+			varFileOpts, err := cloudfoundry.GetVarsFileOptions(config.ManifestVariablesFiles)
+			if err != nil {
+				return errors.Wrapf(err, "Cannot prepare var-file-options: '%v'", config.ManifestVariablesFiles)
 			}
+			cfCreateServiceScript = append(cfCreateServiceScript, varFileOpts...)
 		}
 		if len(config.ManifestVariables) >= 0 {
-			varPart, err = varOptions(config.ManifestVariables)
-		}
-		for _, s := range varPart {
-			cfCreateServiceScript = append(cfCreateServiceScript, s)
+			varOptions, err := cloudfoundry.GetVarsOptions(config.ManifestVariables)
+			if err != nil {
+				return errors.Wrapf(err, "Cannot prepare var-options: '%v'", config.ManifestVariables)
+			}
+			cfCreateServiceScript = append(cfCreateServiceScript, varOptions...)
 		}
 	}
 	err = c.RunExecutable("cf", cfCreateServiceScript...)
@@ -101,11 +99,3 @@ func cloudFoundryCreateServiceRequest(config *cloudFoundryCreateServiceOptions,
 	}
 	return nil
 }
-
-func varOptions(options []string) ([]string, error) {
-	var varOptionsString []string
-	for _, s := range options {
-		varOptionsString = append(varOptionsString, "--var", s)
-	}
-	return varOptionsString, nil
-}

cmd/cloudFoundryCreateService_test.go

@@ -215,11 +215,12 @@ func TestCloudFoundryCreateService(t *testing.T) {
 			Password:               "testPassword",
 			ServiceManifest:        "manifestTest.yml",
 			ManifestVariablesFiles: manifestVariablesFiles,
+			ManifestVariables:      []string{"a=b", "x=y"},
 		}
 		error := runCloudFoundryCreateService(&config, &telemetryData, cf)
 		if assert.NoError(t, error) {
 			assert.Equal(t, []mock.ExecCall{{Execution: (*mock.Execution)(nil), Async: false, Exec: "cf", Params: []string{"login", "-a", "https://api.endpoint.com", "-o", "testOrg", "-s", "testSpace", "-u", "testUser", "-p", "testPassword"}},
-				{Execution: (*mock.Execution)(nil), Async: false, Exec: "cf", Params: []string{"create-service-push", "--no-push", "--service-manifest", "manifestTest.yml", "--vars-file", "varsTest.yml", "--vars-file", "varsTest2.yml"}},
+				{Execution: (*mock.Execution)(nil), Async: false, Exec: "cf", Params: []string{"create-service-push", "--no-push", "--service-manifest", "manifestTest.yml", "--vars-file", "varsTest.yml", "--vars-file", "varsTest2.yml", "--var", "a=b", "--var", "x=y"}},
 				{Execution: (*mock.Execution)(nil), Async: false, Exec: "cf", Params: []string{"logout"}}},
 				m.Calls)
 		}

cmd/cloudFoundryDeploy.go

@@ -33,6 +33,8 @@ var _cfLogin = cfLogin
 var _cfLogout = cfLogout
 var _getManifest = getManifest
 var _replaceVariables = yaml.Substitute
+var _getVarsOptions = cloudfoundry.GetVarsOptions
+var _getVarsFileOptions = cloudfoundry.GetVarsFileOptions
 var fileUtils cfFileUtil = piperutils.Files{}
 
 // for simplify mocking. Maybe we find a more elegant way (mock for CFUtils)
@@ -554,14 +556,20 @@ func handleLegacyCfManifest(manifestFile string) error {
 func prepareCfPushCfNativeDeploy(config *cloudFoundryDeployOptions) (string, []string, []string, error) {
 
 	deployOptions := []string{}
-	varOptions, err := getVarOptions(config.ManifestVariables)
+	varOptions, err := _getVarsOptions(config.ManifestVariables)
 	if err != nil {
 		return "", []string{}, []string{}, errors.Wrapf(err, "Cannot prepare var-options: '%v'", config.ManifestVariables)
 	}
 
-	varFileOptions, err := getVarFileOptions(config.ManifestVariablesFiles)
+	varFileOptions, err := _getVarsFileOptions(config.ManifestVariablesFiles)
 	if err != nil {
-		return "", []string{}, []string{}, errors.Wrapf(err, "Cannot prepare var-file-options: '%v'", config.ManifestVariablesFiles)
+		if e, ok := err.(*cloudfoundry.VarsFilesNotFoundError); ok {
+			for _, missingVarFile := range e.MissingFiles {
+				log.Entry().Warningf("We skip adding not-existing file '%s' as a vars-file to the cf create-service-push call", missingVarFile)
+			}
+		} else {
+			return "", []string{}, []string{}, errors.Wrapf(err, "Cannot prepare var-file-options: '%v'", config.ManifestVariablesFiles)
+		}
 	}
 
 	deployOptions = append(deployOptions, varOptions...)
@@ -591,54 +599,6 @@ func toStringInterfaceMap(in *orderedmap.OrderedMap, err error) (map[string]inte
 
 	return out, err
 }
-func getVarOptions(vars []string) ([]string, error) {
-
-	varsMap, err := toParameterMap(vars)
-	if err != nil {
-		return []string{}, err
-	}
-
-	varsResult := []string{}
-
-	for _, key := range varsMap.Keys() {
-		val, _ := varsMap.Get(key)
-		if v, ok := val.(string); ok {
-			varsResult = append(varsResult, "--var", fmt.Sprintf("%s=%s", key, v))
-		} else {
-			return []string{}, fmt.Errorf("Cannot cast '%v' to string", val)
-		}
-	}
-	return varsResult, nil
-}
-
-func getVarFileOptions(manifestVariableFiles []string) ([]string, error) {
-
-	varFiles, err := validateManifestVariablesFiles(manifestVariableFiles)
-	if err != nil {
-		return []string{}, errors.Wrapf(err, "Cannot validate manifest variables files '%v'", manifestVariableFiles)
-	}
-
-	varFilesResult := []string{}
-
-	for _, varFile := range varFiles {
-		fExists, err := fileUtils.FileExists(varFile)
-		if err != nil {
-			return []string{}, err
-		}
-
-		if !fExists {
-			log.Entry().Warningf("We skip adding not-existing file '%s' as a vars-file to the cf create-service-push call", varFile)
-			continue
-		}
-
-		varFilesResult = append(varFilesResult, "--vars-file", varFile)
-	}
-
-	if len(varFilesResult) > 0 {
-		log.Entry().Infof("We will add the following string to the cf push call: '%s'", strings.Join(varFilesResult, " "))
-	}
-	return varFilesResult, nil
-}
 
 func checkAndUpdateDeployTypeForNotSupportedManifest(config *cloudFoundryDeployOptions) (string, error) {
 

cmd/cloudFoundryDeploy_test.go

@@ -827,9 +827,17 @@ func TestCfDeployment(t *testing.T) {
 
 		defer func() {
 			_getManifest = getManifest
+			_getVarsOptions = cloudfoundry.GetVarsOptions
+			_getVarsFileOptions = cloudfoundry.GetVarsFileOptions
 		}()
 
-		filesMock.AddFile("vars.yaml", []byte("content does not matter"))
+		_getVarsOptions = func(vars []string) ([]string, error) {
+			return []string{"--var", "appName=testApplicationFromVarsList"}, nil
+		}
+		_getVarsFileOptions = func(varFiles []string) ([]string, error) {
+			return []string{"--vars-file", "vars.yaml"}, nil
+		}
+
 		filesMock.AddFile("test-manifest.yml", []byte("content does not matter"))
 
 		_getManifest = func(name string) (cloudfoundry.Manifest, error) {
@@ -887,10 +895,11 @@ func TestCfDeployment(t *testing.T) {
 			filesMock.FileRemove("test-manifest.yml")
 			filesMock.FileRemove("vars.yaml")
 			_getManifest = getManifest
+			_getVarsOptions = cloudfoundry.GetVarsOptions
+			_getVarsFileOptions = cloudfoundry.GetVarsFileOptions
 		}()
 
 		filesMock.AddFile("test-manifest.yml", []byte("content does not matter"))
-		filesMock.AddFile("vars.yaml", []byte("content does not matter"))
 
 		_getManifest = func(name string) (cloudfoundry.Manifest, error) {
 			return manifestMock{
@@ -906,10 +915,30 @@ func TestCfDeployment(t *testing.T) {
 
 		s := mock.ExecMockRunner{}
 
+		var receivedVarOptions []string
+		var receivedVarsFileOptions []string
+
+		_getVarsOptions = func(vars []string) ([]string, error) {
+			receivedVarOptions = vars
+			return []string{}, nil
+		}
+		_getVarsFileOptions = func(varFiles []string) ([]string, error) {
+			receivedVarsFileOptions = varFiles
+			return []string{"--vars-file", "vars.yaml"}, nil
+		}
+
 		err := runCloudFoundryDeploy(&config, nil, nil, &s)
 
 		if assert.NoError(t, err) {
 
+			t.Run("check received vars options", func(t *testing.T) {
+				assert.Empty(t, receivedVarOptions)
+			})
+
+			t.Run("check received vars file options", func(t *testing.T) {
+				assert.Equal(t, []string{"vars.yaml", "vars-does-not-exist.yaml"}, receivedVarsFileOptions)
+			})
+
 			t.Run("check shell calls", func(t *testing.T) {
 
 				withLoginAndLogout(t, func(t *testing.T) {
@@ -1011,61 +1040,6 @@ func TestValidateDeployTool(t *testing.T) {
 	}
 }
 
-func TestManifestVariableFiles(t *testing.T) {
-
-	defer func() {
-		fileUtils = &piperutils.Files{}
-	}()
-
-	filesMock := mock.FilesMock{}
-	fileUtils = &filesMock
-
-	filesMock.AddFile("a/varsA.txt", []byte("content does not matter"))
-	filesMock.AddFile("varsB.txt", []byte("content does not matter"))
-
-	t.Run("straight forward", func(t *testing.T) {
-		varOpts, err := getVarFileOptions([]string{"a/varsA.txt", "varsB.txt"})
-		if assert.NoError(t, err) {
-			assert.Equal(t, []string{"--vars-file", "a/varsA.txt", "--vars-file", "varsB.txt"}, varOpts)
-		}
-	})
-
-	t.Run("no var filesprovided", func(t *testing.T) {
-		varOpts, err := getVarFileOptions([]string{})
-		if assert.NoError(t, err) {
-			assert.Equal(t, []string{}, varOpts)
-		}
-	})
-
-	t.Run("one var file does not exist", func(t *testing.T) {
-		varOpts, err := getVarFileOptions([]string{"a/varsA.txt", "doesNotExist.txt"})
-		if assert.NoError(t, err) {
-			assert.Equal(t, []string{"--vars-file", "a/varsA.txt"}, varOpts)
-		}
-	})
-}
-
-func TestManifestVariables(t *testing.T) {
-	t.Run("straight forward", func(t *testing.T) {
-		varOpts, err := getVarOptions([]string{"a=b", "c=d"})
-		if assert.NoError(t, err) {
-			assert.Equal(t, []string{"--var", "a=b", "--var", "c=d"}, varOpts)
-		}
-	})
-
-	t.Run("empty variabls list", func(t *testing.T) {
-		varOpts, err := getVarOptions([]string{})
-		if assert.NoError(t, err) {
-			assert.Equal(t, []string{}, varOpts)
-		}
-	})
-
-	t.Run("no equal sign in variable", func(t *testing.T) {
-		_, err := getVarOptions([]string{"ab"})
-		assert.EqualError(t, err, "Invalid parameter provided (expected format <key>=<val>: 'ab'")
-	})
-}
-
 func TestMtarLookup(t *testing.T) {
 
 	defer func() {

pkg/cloudfoundry/Vars.go

@@ -0,0 +1,77 @@
+package cloudfoundry
+
+import (
+	"fmt"
+	"github.com/SAP/jenkins-library/pkg/piperutils"
+	"regexp"
+)
+
+// VarsFilesNotFoundError ...
+type VarsFilesNotFoundError struct {
+	Message      string
+	MissingFiles []string
+}
+
+func (e *VarsFilesNotFoundError) Error() string {
+	return fmt.Sprintf("%s: %v", e.Message, e.MissingFiles)
+}
+
+var _fileUtils piperutils.FileUtils = piperutils.Files{}
+
+// GetVarsFileOptions Returns a string array containing valid var file options,
+// e.g.: --vars myVars.yml
+// The options array contains all vars files which could be resolved in the file system.
+// In case some vars files cannot be found, the missing files are reported
+// via the error which is in this case a VarsFilesNotFoundError. In that case the options
+// array contains nevertheless the options for all existing files.
+func GetVarsFileOptions(varsFiles []string) ([]string, error) {
+	varsFilesOpts := []string{}
+	notFound := []string{}
+	var err error
+	for _, varsFile := range varsFiles {
+		varsFileExists, err := _fileUtils.FileExists(varsFile)
+		if err != nil {
+			return nil, fmt.Errorf("Error accessing file system: %w", err)
+		}
+		if varsFileExists {
+			varsFilesOpts = append(varsFilesOpts, "--vars-file", varsFile)
+		} else {
+			notFound = append(notFound, varsFile)
+		}
+	}
+	if len(notFound) > 0 {
+		err = &VarsFilesNotFoundError{
+			Message:      "Some vars files could not be found",
+			MissingFiles: notFound,
+		}
+	}
+	return varsFilesOpts, err
+}
+
+// GetVarsOptions Returns the vars as valid var option string slice
+// InvalidVars are reported via error.
+func GetVarsOptions(vars []string) ([]string, error) {
+	invalidVars := []string{}
+	varOptions := []string{}
+	var err error
+	for _, v := range vars {
+		valid, e := validateVar(v)
+		if e != nil {
+			return []string{}, fmt.Errorf("Cannot validate var '%s': %w", v, e)
+		}
+		if !valid {
+			invalidVars = append(invalidVars, v)
+			continue
+		}
+		varOptions = append(varOptions, "--var", v)
+	}
+
+	if len(invalidVars) > 0 {
+		return []string{}, fmt.Errorf("Invalid vars: %v", invalidVars)
+	}
+	return varOptions, err
+}
+
+func validateVar(v string) (bool, error) {
+	return regexp.MatchString(`\S{1,}=\S{1,}`, v)
+}

pkg/cloudfoundry/Vars_test.go

@@ -0,0 +1,54 @@
+package cloudfoundry
+
+import (
+	"github.com/SAP/jenkins-library/pkg/mock"
+	"github.com/SAP/jenkins-library/pkg/piperutils"
+	"github.com/stretchr/testify/assert"
+	"testing"
+)
+
+func TestVarsFiles(t *testing.T) {
+
+	defer func() {
+		_fileUtils = piperutils.Files{}
+	}()
+
+	filesMock := mock.FilesMock{}
+	filesMock.AddDir("/home/me")
+	filesMock.Chdir("/home/me")
+	filesMock.AddFile("varsA.yml", []byte("file content does not matter"))
+	filesMock.AddFile("varsB.yml", []byte("file content does not matter"))
+	_fileUtils = &filesMock
+
+	t.Run("All vars files found", func(t *testing.T) {
+		opts, err := GetVarsFileOptions([]string{"varsA.yml", "varsB.yml"})
+		if assert.NoError(t, err) {
+			assert.Equal(t, []string{"--vars-file", "varsA.yml", "--vars-file", "varsB.yml"}, opts)
+		}
+	})
+
+	t.Run("Some vars files missing", func(t *testing.T) {
+		opts, err := GetVarsFileOptions([]string{"varsA.yml", "varsC.yml", "varsD.yml"})
+		if assert.EqualError(t, err, "Some vars files could not be found: [varsC.yml varsD.yml]") {
+			assert.IsType(t, &VarsFilesNotFoundError{}, err)
+			assert.Equal(t, []string{"--vars-file", "varsA.yml"}, opts)
+		}
+	})
+}
+
+func TestVars(t *testing.T) {
+
+	t.Run("Empty vars", func(t *testing.T) {
+		opts, err := GetVarsOptions([]string{})
+		if assert.NoError(t, err) {
+			assert.Equal(t, []string{}, opts)
+		}
+	})
+
+	t.Run("Some vars", func(t *testing.T) {
+		opts, err := GetVarsOptions([]string{"a=b", "x=y"})
+		if assert.NoError(t, err) {
+			assert.Equal(t, []string{"--var", "a=b", "--var", "x=y"}, opts)
+		}
+	})
+}