From 99c3d5bb043560ebf6c2816cbc43d798b8d4eb47 Mon Sep 17 00:00:00 2001 From: Arash Deshmeh Date: Wed, 21 Feb 2018 06:29:08 -0500 Subject: [PATCH 1/4] added support for mandatory variables to cli/compose Signed-off-by: Arash Deshmeh Upstream-commit: 99ecf57c6c2ecbe34e108a555c0541d6e2f8405f Component: cli --- .../cli/cli/compose/template/template.go | 22 ++++++- .../cli/cli/compose/template/template_test.go | 60 +++++++++++++++++++ 2 files changed, 81 insertions(+), 1 deletion(-) diff --git a/components/cli/cli/compose/template/template.go b/components/cli/cli/compose/template/template.go index 8426a23522..9bfc7cec5c 100644 --- a/components/cli/cli/compose/template/template.go +++ b/components/cli/cli/compose/template/template.go @@ -7,7 +7,7 @@ import ( ) var delimiter = "\\$" -var substitution = "[_a-z][_a-z0-9]*(?::?-[^}]+)?" +var substitution = "[_a-z][_a-z0-9]*(?::?[-?][^}]+)?" var patternString = fmt.Sprintf( "%s(?i:(?P%s)|(?P%s)|{(?P%s)}|(?P))", @@ -69,6 +69,26 @@ func Substitute(template string, mapping Mapping) (string, error) { return value } + if strings.Contains(substitution, ":?") { + name, errorMessage := partition(substitution, ":?") + value, ok := mapping(name) + if !ok || value == "" { + err = &InvalidTemplateError{Template: errorMessage} + return "" + } + return value + } + + if strings.Contains(substitution, "?") { + name, errorMessage := partition(substitution, "?") + value, ok := mapping(name) + if !ok { + err = &InvalidTemplateError{Template: errorMessage} + return "" + } + return value + } + // No default (fall back to empty string) value, ok := mapping(substitution) if !ok { diff --git a/components/cli/cli/compose/template/template_test.go b/components/cli/cli/compose/template/template_test.go index 4ad3f252ac..d11b0a8ec3 100644 --- a/components/cli/cli/compose/template/template_test.go +++ b/components/cli/cli/compose/template/template_test.go @@ -81,3 +81,63 @@ func TestNonAlphanumericDefault(t *testing.T) { assert.Nil(t, err) assert.Equal(t, "ok /non:-alphanumeric", result) } + +func TestMandatoryVariableErrors(t *testing.T) { + testCases := []struct { + template string + expectedError string + }{ + { + template: "not ok ${UNSET_VAR:?Mandatory Variable Unset}", + expectedError: "Mandatory Variable Unset", + }, + { + template: "not ok ${BAR:?Mandatory Variable Empty}", + expectedError: "Mandatory Variable Empty", + }, + { + template: "not ok ${UNSET_VAR:?}", + expectedError: "", + }, + { + template: "not ok ${UNSET_VAR?Mandatory Variable Unset", + expectedError: "Mandatory Variable Unset", + }, + { + template: "not ok ${UNSET_VAR?}", + expectedError: "", + }, + } + + for _, tc := range testCases { + _, err := Substitute(tc.template, defaultMapping) + assert.Error(t, err) + assert.IsType(t, &InvalidTemplateError{tc.expectedError}, err) + } +} + +func TestDefaultsForMandatoryVariables(t *testing.T) { + testCases := []struct { + template string + expected string + }{ + { + template: "ok ${FOO:?err}", + expected: "ok first", + }, + { + template: "ok ${FOO?err}", + expected: "ok first", + }, + { + template: "ok ${BAR?err}", + expected: "ok ", + }, + } + + for _, tc := range testCases { + result, err := Substitute(tc.template, defaultMapping) + assert.Nil(t, err) + assert.Equal(t, tc.expected, result) + } +} From 41c5444dac3bf035e98bf9dbcf1aa57867e3f08c Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 21 Feb 2018 15:16:12 -0500 Subject: [PATCH 2/4] Refactor substitute to reduce cyclo complexity Signed-off-by: Daniel Nephin Upstream-commit: ce544823b61b856fad8483e05796a2f2831116ca Component: cli --- .../cli/cli/compose/template/template.go | 95 +++++++++---------- .../cli/cli/compose/template/template_test.go | 7 ++ 2 files changed, 53 insertions(+), 49 deletions(-) diff --git a/components/cli/cli/compose/template/template.go b/components/cli/cli/compose/template/template.go index 9bfc7cec5c..2792a09ba5 100644 --- a/components/cli/cli/compose/template/template.go +++ b/components/cli/cli/compose/template/template.go @@ -37,77 +37,74 @@ func Substitute(template string, mapping Mapping) (string, error) { var err error result := pattern.ReplaceAllStringFunc(template, func(substring string) string { matches := pattern.FindStringSubmatch(substring) - groups := make(map[string]string) - for i, name := range pattern.SubexpNames() { - if i != 0 { - groups[name] = matches[i] - } + groups := matchGroups(matches) + if escaped := groups["escaped"]; escaped != "" { + return escaped } substitution := groups["named"] if substitution == "" { substitution = groups["braced"] } - if substitution != "" { - // Soft default (fall back if unset or empty) - if strings.Contains(substitution, ":-") { - name, defaultValue := partition(substitution, ":-") - value, ok := mapping(name) - if !ok || value == "" { - return defaultValue - } - return value - } - // Hard default (fall back if-and-only-if empty) - if strings.Contains(substitution, "-") { - name, defaultValue := partition(substitution, "-") - value, ok := mapping(name) - if !ok { - return defaultValue - } - return value - } + switch { - if strings.Contains(substitution, ":?") { - name, errorMessage := partition(substitution, ":?") - value, ok := mapping(name) - if !ok || value == "" { - err = &InvalidTemplateError{Template: errorMessage} - return "" - } - return value - } + case substitution == "": + err = &InvalidTemplateError{Template: template} + return "" - if strings.Contains(substitution, "?") { - name, errorMessage := partition(substitution, "?") - value, ok := mapping(name) - if !ok { - err = &InvalidTemplateError{Template: errorMessage} - return "" - } - return value + // Soft default (fall back if unset or empty) + case strings.Contains(substitution, ":-"): + name, defaultValue := partition(substitution, ":-") + value, ok := mapping(name) + if !ok || value == "" { + return defaultValue } + return value - // No default (fall back to empty string) - value, ok := mapping(substitution) + // Hard default (fall back if-and-only-if empty) + case strings.Contains(substitution, "-"): + name, defaultValue := partition(substitution, "-") + value, ok := mapping(name) if !ok { + return defaultValue + } + return value + + case strings.Contains(substitution, ":?"): + name, errorMessage := partition(substitution, ":?") + value, ok := mapping(name) + if !ok || value == "" { + err = &InvalidTemplateError{Template: errorMessage} + return "" + } + return value + + case strings.Contains(substitution, "?"): + name, errorMessage := partition(substitution, "?") + value, ok := mapping(name) + if !ok { + err = &InvalidTemplateError{Template: errorMessage} return "" } return value } - if escaped := groups["escaped"]; escaped != "" { - return escaped - } - - err = &InvalidTemplateError{Template: template} - return "" + value, _ := mapping(substitution) + return value }) return result, err } +func matchGroups(matches []string) map[string]string { + groups := make(map[string]string) + for i, name := range pattern.SubexpNames()[1:] { + groups[name] = matches[i+1] + } + return groups +} + // Split the string at the first occurrence of sep, and return the part before the separator, // and the part after the separator. // diff --git a/components/cli/cli/compose/template/template_test.go b/components/cli/cli/compose/template/template_test.go index d11b0a8ec3..ce75216dde 100644 --- a/components/cli/cli/compose/template/template_test.go +++ b/components/cli/cli/compose/template/template_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) var defaults = map[string]string{ @@ -22,6 +23,12 @@ func TestEscaped(t *testing.T) { assert.Equal(t, "${foo}", result) } +func TestSubstituteNoMatch(t *testing.T) { + result, err := Substitute("foo", defaultMapping) + require.NoError(t, err) + require.Equal(t, "foo", result) +} + func TestInvalid(t *testing.T) { invalidTemplates := []string{ "${", From bb11b4e9b1505767ab65c88009d738a153007733 Mon Sep 17 00:00:00 2001 From: Arash Deshmeh Date: Wed, 21 Feb 2018 17:02:38 -0500 Subject: [PATCH 3/4] Added error message check to TestMandatoryVariableErrors test Signed-off-by: Arash Deshmeh Upstream-commit: e33bc48752c35bf3ac4a86e904732aac4527887b Component: cli --- components/cli/cli/compose/template/template_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/components/cli/cli/compose/template/template_test.go b/components/cli/cli/compose/template/template_test.go index ce75216dde..66e50124ed 100644 --- a/components/cli/cli/compose/template/template_test.go +++ b/components/cli/cli/compose/template/template_test.go @@ -3,6 +3,7 @@ package template import ( "testing" + "github.com/docker/cli/internal/test/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -119,7 +120,8 @@ func TestMandatoryVariableErrors(t *testing.T) { for _, tc := range testCases { _, err := Substitute(tc.template, defaultMapping) assert.Error(t, err) - assert.IsType(t, &InvalidTemplateError{tc.expectedError}, err) + assert.IsType(t, &InvalidTemplateError{}, err) + testutil.ErrorContains(t, err, tc.expectedError) } } From 38e7824f05f4a24a388664ce1ce73ee4b46d49f8 Mon Sep 17 00:00:00 2001 From: Arash Deshmeh Date: Mon, 26 Feb 2018 15:23:52 -0500 Subject: [PATCH 4/4] fix the error message in Substitute function Signed-off-by: Arash Deshmeh Upstream-commit: 5d8ce59a2578d7c145c33461727aed6391aa22bf Component: cli --- components/cli/cli/compose/template/template.go | 10 +++++++--- components/cli/cli/compose/template/template_test.go | 12 ++++++------ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/components/cli/cli/compose/template/template.go b/components/cli/cli/compose/template/template.go index 2792a09ba5..086194b8fe 100644 --- a/components/cli/cli/compose/template/template.go +++ b/components/cli/cli/compose/template/template.go @@ -7,7 +7,7 @@ import ( ) var delimiter = "\\$" -var substitution = "[_a-z][_a-z0-9]*(?::?[-?][^}]+)?" +var substitution = "[_a-z][_a-z0-9]*(?::?[-?][^}]*)?" var patternString = fmt.Sprintf( "%s(?i:(?P%s)|(?P%s)|{(?P%s)}|(?P))", @@ -75,7 +75,9 @@ func Substitute(template string, mapping Mapping) (string, error) { name, errorMessage := partition(substitution, ":?") value, ok := mapping(name) if !ok || value == "" { - err = &InvalidTemplateError{Template: errorMessage} + err = &InvalidTemplateError{ + Template: fmt.Sprintf("required variable %s is missing a value: %s", name, errorMessage), + } return "" } return value @@ -84,7 +86,9 @@ func Substitute(template string, mapping Mapping) (string, error) { name, errorMessage := partition(substitution, "?") value, ok := mapping(name) if !ok { - err = &InvalidTemplateError{Template: errorMessage} + err = &InvalidTemplateError{ + Template: fmt.Sprintf("required variable %s is missing a value: %s", name, errorMessage), + } return "" } return value diff --git a/components/cli/cli/compose/template/template_test.go b/components/cli/cli/compose/template/template_test.go index 66e50124ed..4fec57e8e8 100644 --- a/components/cli/cli/compose/template/template_test.go +++ b/components/cli/cli/compose/template/template_test.go @@ -97,23 +97,23 @@ func TestMandatoryVariableErrors(t *testing.T) { }{ { template: "not ok ${UNSET_VAR:?Mandatory Variable Unset}", - expectedError: "Mandatory Variable Unset", + expectedError: "required variable UNSET_VAR is missing a value: Mandatory Variable Unset", }, { template: "not ok ${BAR:?Mandatory Variable Empty}", - expectedError: "Mandatory Variable Empty", + expectedError: "required variable BAR is missing a value: Mandatory Variable Empty", }, { template: "not ok ${UNSET_VAR:?}", - expectedError: "", + expectedError: "required variable UNSET_VAR is missing a value", }, { - template: "not ok ${UNSET_VAR?Mandatory Variable Unset", - expectedError: "Mandatory Variable Unset", + template: "not ok ${UNSET_VAR?Mandatory Variable Unset}", + expectedError: "required variable UNSET_VAR is missing a value: Mandatory Variable Unset", }, { template: "not ok ${UNSET_VAR?}", - expectedError: "", + expectedError: "required variable UNSET_VAR is missing a value", }, }