Fix docker compose ps to honor psFormat setting in .docker/config.json#13645
Fix docker compose ps to honor psFormat setting in .docker/config.json#13645maks2134 wants to merge 3 commits intodocker:mainfrom
Conversation
- Change format flag default from "table" to empty string to allow config override - Add logic to use PsFormat from config when no format argument provided - Fall back to "table" format when neither PsFormat nor format arg is set - Add warning when both --format and --quiet flags are used - Add test to verify format flag default value Fixes docker#13643 Signed-off-by: maks2134 <maks210306@yandex.by>
glours
left a comment
glours
left a comment
There was a problem hiding this comment.
Thanks for the contribution 🙏
This one need some changes to properly and transparently support psFormat setting
Can you also add more tests regarding the comment I made when changing or not the --format flag please?
| if len(opts.Format) == 0 { | ||
| opts.Format = "table" | ||
| } else if opts.Quiet { | ||
| _, _ = dockerCli.Err().Write([]byte("WARNING: Ignoring custom format, because both --format and --quiet are set.\n")) | ||
| } |
There was a problem hiding this comment.
With this changes users with psFormat in their config who runs docker compose ps -q would get a spurious warning.
We should only trigger this check when cmd.Flags().Changed("format") is true
There was a problem hiding this comment.
Also I would prefer we use fmt.Fprintln instead of Write like this
fmt.Fprintln(dockerCli.Err(), "WARNING: ...")
There was a problem hiding this comment.
Thanks for the review, I've made some changes.
…s-format-config-clean # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
- Fix warning logic to only trigger when --format flag is explicitly changed - Use fmt.Fprintln instead of Write for warning message - Add comprehensive tests for psFormat scenarios - Fix command closure to properly access cmd.Flags() Signed-off-by: Maks Kozlov <maks@example.com> Signed-off-by: maks2134 <maks210306@yandex.by>
|
Please take a look at the changes when you have the opportunity. @glours |
glours
left a comment
glours
left a comment
There was a problem hiding this comment.
Thanks for this work 🙏
I did a more deep dive review, and there is still fixes to apply
Tests are only checking the flags combinatory but none of them really check the runPs logic
Maybe we should add a e2e test for that
| }, | ||
| RunE: Adapt(func(ctx context.Context, args []string) error { | ||
| return runPs(ctx, dockerCli, backendOptions, args, opts) | ||
| return runPs(ctx, dockerCli, backendOptions, args, opts, psCmd) |
There was a problem hiding this comment.
| return runPs(ctx, dockerCli, backendOptions, args, opts, psCmd) | |
| return runPs(ctx, dockerCli, backendOptions, args, opts, cmd.Flags().Changed("format")) |
| if opts.Quiet { | ||
| for _, c := range containers { | ||
| _, _ = fmt.Fprintln(dockerCli.Out(), c.ID) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
We already leave the function here if the quiet flag is used by the user. So the code checking both quiet and format should be done in RunE function before we call runPs
| } | ||
|
|
||
| if opts.Format == "" { | ||
| if len(opts.Format) == 0 { |
There was a problem hiding this comment.
opts.Format is a string
| if len(opts.Format) == 0 { | |
| if opts.Format == "" { |
| if len(opts.Format) == 0 { | ||
| opts.Format = dockerCli.ConfigFile().PsFormat | ||
| } | ||
| if len(opts.Format) == 0 { |
There was a problem hiding this comment.
opts.Format is a string
| if len(opts.Format) == 0 { | |
| if opts.Format == "" { |
| assert.Equal(t, formatFlag.DefValue, "") | ||
| } | ||
|
|
||
| func TestPsCommandUsesConfigFormat(t *testing.T) { |
There was a problem hiding this comment.
As we don't check runPs behaviour, this test don't provide any additional value, this is already covered by TestPsCommandDefaultFormat
| assert.Assert(t, cmd.Flags().Changed("quiet")) | ||
| } | ||
|
|
||
| func TestPsCommandFormatFallback(t *testing.T) { |
There was a problem hiding this comment.
This test only check the flag was untouched not it defaulted to the table format
2 participants
Fixes #13643
What I did
Related issue
(not mandatory) A picture of a cute animal, if possible in relation to what you did