From 69bb2c9e60b5bb41f56479b7bb8b6f8d617f8154 Mon Sep 17 00:00:00 2001
From: Oliver Nocon <33484802+OliverNocon@users.noreply.github.com>
Date: Fri, 22 Nov 2019 10:30:44 +0100
Subject: [PATCH] Fix issues in config resolution (#1000)

* Respect defaults from step definition everywhere
* prevent empty config to overwrite default in aliasing

Address PR feedback
---
 pkg/config/config.go                  | 30 +++++++++++++++++++++------
 pkg/config/config_test.go             | 21 +++++++++++++++++++
 resources/metadata/githubrelease.yaml |  4 ++--
 3 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/pkg/config/config.go b/pkg/config/config.go
index 22cd145b6..eb7734cca 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -59,7 +59,10 @@ func (c *Config) ApplyAliasConfig(parameters []StepParameters, filters StepFilte
 func setParamValueFromAlias(configMap map[string]interface{}, filter []string, p StepParameters) map[string]interface{} {
 	if configMap != nil && configMap[p.Name] == nil && sliceContains(filter, p.Name) {
 		for _, a := range p.Aliases {
-			configMap[p.Name] = getDeepAliasValue(configMap, a.Name)
+			aliasVal := getDeepAliasValue(configMap, a.Name)
+			if aliasVal != nil {
+				configMap[p.Name] = aliasVal
+			}
 			if configMap[p.Name] != nil {
 				return configMap
 			}
@@ -114,22 +117,25 @@ func (c *Config) GetStepConfig(flagValues map[string]interface{}, paramJSON stri
 		}
 	}
 
-	// first: read defaults & merge general -> steps (-> general -> steps ...)
+	// initialize with defaults from step.yaml
+	stepConfig.mixInStepDefaults(parameters)
+
+	// read defaults & merge general -> steps (-> general -> steps ...)
 	for _, def := range d.Defaults {
 		def.ApplyAliasConfig(parameters, filters, stageName, stepName)
 		stepConfig.mixIn(def.General, filters.General)
 		stepConfig.mixIn(def.Steps[stepName], filters.Steps)
 	}
 
-	// second: read config & merge - general -> steps -> stages
+	// read config & merge - general -> steps -> stages
 	stepConfig.mixIn(c.General, filters.General)
 	stepConfig.mixIn(c.Steps[stepName], filters.Steps)
 	stepConfig.mixIn(c.Stages[stageName], filters.Stages)
 
-	// third: merge parameters provided via env vars
+	// merge parameters provided via env vars
 	stepConfig.mixIn(envValues(filters.All), filters.All)
 
-	// fourth: if parameters are provided in JSON format merge them
+	// if parameters are provided in JSON format merge them
 	if len(paramJSON) != 0 {
 		var params map[string]interface{}
 		json.Unmarshal([]byte(paramJSON), &params)
@@ -142,7 +148,7 @@ func (c *Config) GetStepConfig(flagValues map[string]interface{}, paramJSON stri
 		stepConfig.mixIn(params, filters.Parameters)
 	}
 
-	// fifth: merge command line flags
+	// merge command line flags
 	if flagValues != nil {
 		stepConfig.mixIn(flagValues, filters.Parameters)
 	}
@@ -222,6 +228,18 @@ func (s *StepConfig) mixIn(mergeData map[string]interface{}, filter []string) {
 	s.Config = merge(s.Config, filterMap(mergeData, filter))
 }
 
+func (s *StepConfig) mixInStepDefaults(stepParams []StepParameters) {
+	if s.Config == nil {
+		s.Config = map[string]interface{}{}
+	}
+
+	for _, p := range stepParams {
+		if p.Default != nil {
+			s.Config[p.Name] = p.Default
+		}
+	}
+}
+
 func filterMap(data map[string]interface{}, filter []string) map[string]interface{} {
 	result := map[string]interface{}{}
 
diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go
index cd7049db0..69856b319 100644
--- a/pkg/config/config_test.go
+++ b/pkg/config/config_test.go
@@ -198,6 +198,19 @@ steps:
 
 	})
 
+	t.Run("Consider defaults from step config", func(t *testing.T) {
+		var c Config
+
+		stepParams := []StepParameters{StepParameters{Name: "p0", Scope: []string{"GENERAL"}, Type: "string", Default: "p0_step_default", Aliases: []Alias{{Name: "p0_alias"}}}}
+		testConf := "general:\n p1: p1_conf"
+
+		stepConfig, err := c.GetStepConfig(nil, "", ioutil.NopCloser(strings.NewReader(testConf)), nil, StepFilters{General: []string{"p0", "p1"}}, stepParams, "stage1", "step1")
+
+		assert.NoError(t, err, "Error occured but no error expected")
+		assert.Equal(t, "p0_step_default", stepConfig.Config["p0"])
+		assert.Equal(t, "p1_conf", stepConfig.Config["p1"])
+	})
+
 	t.Run("Failure case config", func(t *testing.T) {
 		var c Config
 		myConfig := ioutil.NopCloser(strings.NewReader("invalid config"))
@@ -283,6 +296,12 @@ func TestApplyAliasConfig(t *testing.T) {
 				{Name: "p6_alias"},
 			},
 		},
+		{
+			Name: "p7",
+			Aliases: []Alias{
+				{Name: "p7_alias"},
+			},
+		},
 	}
 
 	filters := StepFilters{
@@ -311,6 +330,7 @@ func TestApplyAliasConfig(t *testing.T) {
 			"step1": map[string]interface{}{
 				"p5_notused": "p5_step",
 				"p6_alias":   "p6_step",
+				"p7":         "p7_step",
 			},
 		},
 	}
@@ -331,6 +351,7 @@ func TestApplyAliasConfig(t *testing.T) {
 	t.Run("Stage", func(t *testing.T) {
 		assert.Nil(t, c.General["p5"])
 		assert.Equal(t, "p6_step", c.Steps["step1"]["p6"])
+		assert.Equal(t, "p7_step", c.Steps["step1"]["p7"])
 	})
 
 }
diff --git a/resources/metadata/githubrelease.yaml b/resources/metadata/githubrelease.yaml
index 9cf817f04..8773eafbf 100644
--- a/resources/metadata/githubrelease.yaml
+++ b/resources/metadata/githubrelease.yaml
@@ -90,8 +90,8 @@ spec:
       type: string
       mandatory: true
     - name: serverUrl
-      aliases:
-        - name: githubServerUrl
+#      aliases:
+#        - name: githubServerUrl
       description: 'GitHub server url for end-user access.'
       scope:
       - PARAMETERS