From 4cce7bb2fc655b5362b4ea64c4efcf2f04e99ccb Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 28 Aug 2023 11:23:37 +0200 Subject: [PATCH 1/2] rewrite TestParseRestartPolicy to use sub-tests Signed-off-by: Sebastiaan van Stijn --- cli/command/container/opts_test.go | 87 +++++++++++++++++++++--------- 1 file changed, 62 insertions(+), 25 deletions(-) diff --git a/cli/command/container/opts_test.go b/cli/command/container/opts_test.go index 4b8b8f07cc..460c74fad0 100644 --- a/cli/command/container/opts_test.go +++ b/cli/command/container/opts_test.go @@ -700,34 +700,71 @@ func TestRunFlagsParseShmSize(t *testing.T) { } func TestParseRestartPolicy(t *testing.T) { - invalids := map[string]string{ - "always:2:3": "invalid restart policy format: maximum retry count must be an integer", - "on-failure:invalid": "invalid restart policy format: maximum retry count must be an integer", - } - valids := map[string]container.RestartPolicy{ - "": {}, - "always": { - Name: "always", - MaximumRetryCount: 0, + tests := []struct { + input string + expected container.RestartPolicy + expectedErr string + }{ + { + input: "", }, - "on-failure:1": { - Name: "on-failure", - MaximumRetryCount: 1, + { + input: "no", + expected: container.RestartPolicy{ + Name: "no", + }, + }, + { + input: "always", + expected: container.RestartPolicy{ + Name: "always", + }, + }, + { + input: "always:1", + expected: container.RestartPolicy{ + Name: "always", + MaximumRetryCount: 1, + }, + }, + { + input: "always:2:3", + expectedErr: "invalid restart policy format: maximum retry count must be an integer", + }, + { + input: "on-failure:1", + expected: container.RestartPolicy{ + Name: "on-failure", + MaximumRetryCount: 1, + }, + }, + { + input: "on-failure:invalid", + expectedErr: "invalid restart policy format: maximum retry count must be an integer", + }, + { + input: "unless-stopped", + expected: container.RestartPolicy{ + Name: "unless-stopped", + }, + }, + { + input: "unless-stopped:invalid", + expectedErr: "invalid restart policy format: maximum retry count must be an integer", }, } - for restart, expectedError := range invalids { - if _, _, _, err := parseRun([]string{fmt.Sprintf("--restart=%s", restart), "img", "cmd"}); err == nil || err.Error() != expectedError { - t.Fatalf("Expected an error with message '%v' for %v, got %v", expectedError, restart, err) - } - } - for restart, expected := range valids { - _, hostconfig, _, err := parseRun([]string{fmt.Sprintf("--restart=%v", restart), "img", "cmd"}) - if err != nil { - t.Fatal(err) - } - if hostconfig.RestartPolicy != expected { - t.Fatalf("Expected %v, got %v", expected, hostconfig.RestartPolicy) - } + for _, tc := range tests { + tc := tc + t.Run(tc.input, func(t *testing.T) { + _, hostConfig, _, err := parseRun([]string{"--restart=" + tc.input, "img", "cmd"}) + if tc.expectedErr != "" { + assert.Check(t, is.Nil(hostConfig)) + assert.Check(t, is.Error(err, tc.expectedErr)) + } else { + assert.Check(t, is.DeepEqual(hostConfig.RestartPolicy, tc.expected)) + assert.Check(t, err) + } + }) } } From 261c18f9ee3f2fe9a812372e424d0230b04f35ef Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 28 Aug 2023 11:46:49 +0200 Subject: [PATCH 2/2] ParseRestartPolicy: validate for missing policy-names Also make it slightly more clearer we're returning a default (empty) policy if the input is empty. Signed-off-by: Sebastiaan van Stijn --- cli/command/container/opts_test.go | 8 ++++++-- opts/parse.go | 15 ++++++++++----- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/cli/command/container/opts_test.go b/cli/command/container/opts_test.go index 460c74fad0..d8bb7043d4 100644 --- a/cli/command/container/opts_test.go +++ b/cli/command/container/opts_test.go @@ -714,6 +714,10 @@ func TestParseRestartPolicy(t *testing.T) { Name: "no", }, }, + { + input: ":1", + expectedErr: "invalid restart policy format: no policy provided before colon", + }, { input: "always", expected: container.RestartPolicy{ @@ -758,11 +762,11 @@ func TestParseRestartPolicy(t *testing.T) { t.Run(tc.input, func(t *testing.T) { _, hostConfig, _, err := parseRun([]string{"--restart=" + tc.input, "img", "cmd"}) if tc.expectedErr != "" { - assert.Check(t, is.Nil(hostConfig)) assert.Check(t, is.Error(err, tc.expectedErr)) + assert.Check(t, is.Nil(hostConfig)) } else { + assert.NilError(t, err) assert.Check(t, is.DeepEqual(hostConfig.RestartPolicy, tc.expected)) - assert.Check(t, err) } }) } diff --git a/opts/parse.go b/opts/parse.go index 017577e4bf..8b8012d300 100644 --- a/opts/parse.go +++ b/opts/parse.go @@ -71,17 +71,22 @@ func ConvertKVStringsToMapWithNil(values []string) map[string]*string { // ParseRestartPolicy returns the parsed policy or an error indicating what is incorrect func ParseRestartPolicy(policy string) (container.RestartPolicy, error) { - p := container.RestartPolicy{} - if policy == "" { - return p, nil + // for backward-compatibility, we don't set the default ("no") + // policy here, because older versions of the engine may not + // support it. + return container.RestartPolicy{}, nil } - k, v, _ := strings.Cut(policy, ":") + p := container.RestartPolicy{} + k, v, ok := strings.Cut(policy, ":") + if ok && k == "" { + return container.RestartPolicy{}, fmt.Errorf("invalid restart policy format: no policy provided before colon") + } if v != "" { count, err := strconv.Atoi(v) if err != nil { - return p, fmt.Errorf("invalid restart policy format: maximum retry count must be an integer") + return container.RestartPolicy{}, fmt.Errorf("invalid restart policy format: maximum retry count must be an integer") } p.MaximumRetryCount = count }