-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't automatically request size if --size
was explicitly set to false
#4067
Conversation
/cc @thaJeztah |
Oh! Was looking if we could update one of the unit tests to test this, but forgot to post; looks like we have some test for testing the auto-detect; cli/cli/command/container/list_test.go Lines 233 to 243 in cb5463a
I guess we could make that a test-table to test the various options; I gave that a quick go; func TestContainerListFormatSizeSetsOption(t *testing.T) {
tests := []struct {
doc, format, sizeFlag string
sizeExpected bool
}{
{
doc: "detect with all fields",
format: `{{json .}}`,
sizeExpected: true,
},
{
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())
})
}
} |
That's great! I thought about adding a little test case and ended up forgetting about it, but this is more comprehensive. I'll add it in. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #4067 +/- ##
==========================================
- Coverage 59.20% 59.17% -0.04%
==========================================
Files 287 287
Lines 24698 24731 +33
==========================================
+ Hits 14622 14634 +12
- Misses 9192 9212 +20
- Partials 884 885 +1 |
Thanks! Probably fine to squash those commits to keep the changes together |
…alse` Signed-off-by: Laura Brehm <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I added the docs-revisit
label; we need to look if the documentation shows examples for this, and perhaps have an example / mention that this option can be used to explicitly disable the size-calculation.
- What I did
Check if the
--size
flag was explicitly set to false (with--size=false
) and don't automatically request the size when that's the case.Also changed the flow a bit to always validate the template when
--format
is used.Provides a workaround for docker/for-linux#1179
- How I did it
With Neovim 👀
- How to verify it
Run
docker ps --format={{json .}} --size=false
and check that size isn't calculated- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)