Merge pull request #4107 from thaJeztah/23.0_backport_size_flag_ps

[23.0 backport] Don't automatically request size if `--size` was explicitly set to `false`
docker · Mar 21, 2023 · c2c122f · c2c122f
2 parents 40a48e4 + a43c9f3
commit c2c122f
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 25 deletions.
diff --git a/cli/command/container/list.go b/cli/command/container/list.go
@@ -17,14 +17,15 @@ import (
 )
 
 type psOptions struct {
-	quiet   bool
-	size    bool
-	all     bool
-	noTrunc bool
-	nLatest bool
-	last    int
-	format  string
-	filter  opts.FilterOpt
+	quiet       bool
+	size        bool
+	sizeChanged bool
+	all         bool
+	noTrunc     bool
+	nLatest     bool
+	last        int
+	format      string
+	filter      opts.FilterOpt
 }
 
 // NewPsCommand creates a new cobra.Command for `docker ps`
@@ -36,6 +37,7 @@ func NewPsCommand(dockerCli command.Cli) *cobra.Command {
 		Short: "List containers",
 		Args:  cli.NoArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
+			options.sizeChanged = cmd.Flags().Changed("size")
 			return runPs(dockerCli, &options)
 		},
 		Annotations: map[string]string{
@@ -78,13 +80,8 @@ func buildContainerListOptions(opts *psOptions) (*types.ContainerListOptions, er
 		options.Limit = 1
 	}
 
-	if !opts.quiet && !options.Size && len(opts.format) > 0 {
-		// The --size option isn't set, but .Size may be used in the template.
-		// Parse and execute the given template to detect if the .Size field is
-		// used. If it is, then automatically enable the --size option. See #24696
-		//
-		// Only requesting container size information when needed is an optimization,
-		// because calculating the size is a costly operation.
+	// always validate template when `--format` is used, for consistency
+	if len(opts.format) > 0 {
 		tmpl, err := templates.NewParse("", opts.format)
 		if err != nil {
 			return nil, errors.Wrap(err, "failed to parse template")
@@ -98,8 +95,19 @@ func buildContainerListOptions(opts *psOptions) (*types.ContainerListOptions, er
 			return nil, errors.Wrap(err, "failed to execute template")
 		}
 
-		if _, ok := optionsProcessor.FieldsUsed["Size"]; ok {
-			options.Size = true
+		// if `size` was not explicitly set to false (with `--size=false`)
+		// and `--quiet` is not set, request size if the template requires it
+		if !opts.quiet && !options.Size && !opts.sizeChanged {
+			// The --size option isn't set, but .Size may be used in the template.
+			// Parse and execute the given template to detect if the .Size field is
+			// used. If it is, then automatically enable the --size option. See #24696
+			//
+			// Only requesting container size information when needed is an optimization,
+			// because calculating the size is a costly operation.
+
+			if _, ok := optionsProcessor.FieldsUsed["Size"]; ok {
+				options.Size = true
+			}
 		}
 	}
 

diff --git a/cli/command/container/list_test.go b/cli/command/container/list_test.go
@@ -231,15 +231,56 @@ func TestContainerListFormatTemplateWithArg(t *testing.T) {
 }
 
 func TestContainerListFormatSizeSetsOption(t *testing.T) {
-	cli := test.NewFakeCli(&fakeClient{
-		containerListFunc: func(options types.ContainerListOptions) ([]types.Container, error) {
-			assert.Check(t, options.Size)
-			return []types.Container{}, nil
+	tests := []struct {
+		doc, format, sizeFlag string
+		sizeExpected          bool
+	}{
+		{
+			doc:          "detect with all fields",
+			format:       `{{json .}}`,
+			sizeExpected: true,
 		},
-	})
-	cmd := newListCommand(cli)
-	cmd.Flags().Set("format", `{{.Size}}`)
-	assert.NilError(t, cmd.Execute())
+		{
+			doc:          "detect with explicit field",
+			format:       `{{.Size}}`,
+			sizeExpected: true,
+		},
+		{
+			doc:          "detect no size",
+			format:       `{{.Names}}`,
+			sizeExpected: false,
+		},
+		{
+			doc:          "override enable",
+			format:       `{{.Names}}`,
+			sizeFlag:     "true",
+			sizeExpected: true,
+		},
+		{
+			doc:          "override disable",
+			format:       `{{.Size}}`,
+			sizeFlag:     "false",
+			sizeExpected: false,
+		},
+	}
+
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.doc, func(t *testing.T) {
+			cli := test.NewFakeCli(&fakeClient{
+				containerListFunc: func(options types.ContainerListOptions) ([]types.Container, error) {
+					assert.Check(t, is.Equal(options.Size, tc.sizeExpected))
+					return []types.Container{}, nil
+				},
+			})
+			cmd := newListCommand(cli)
+			cmd.Flags().Set("format", tc.format)
+			if tc.sizeFlag != "" {
+				cmd.Flags().Set("size", tc.sizeFlag)
+			}
+			assert.NilError(t, cmd.Execute())
+		})
+	}
 }
 
 func TestContainerListWithConfigFormat(t *testing.T) {