From 4737ed4906dd4b511418c741adf5f592e2f77ed4 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 10 Oct 2025 17:21:57 +0200 Subject: [PATCH 1/5] cli/command/image/build: fix linting, add sub-tests - fix minor linting issues (unhandled errors) - rename vars to prevent shadowing - use sub-tests for tests that already prepared for it Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 2c539a6530ec318f5aedb2f3dbb1db870a8d410b) Signed-off-by: Sebastiaan van Stijn --- cli/command/image/build/context.go | 30 ++++++++++++---------- cli/command/image/build/context_test.go | 34 +++++++++++++++---------- cli/command/image/build/dockerignore.go | 4 ++- 3 files changed, 39 insertions(+), 29 deletions(-) diff --git a/cli/command/image/build/context.go b/cli/command/image/build/context.go index add175c2c0f3..a61185108c75 100644 --- a/cli/command/image/build/context.go +++ b/cli/command/image/build/context.go @@ -80,7 +80,7 @@ func ValidateContextDirectory(srcPath string, excludes []string) error { if err != nil && os.IsPermission(err) { return errors.Errorf("no permission to read from '%s'", filePath) } - currentFile.Close() + _ = currentFile.Close() } return nil }) @@ -97,10 +97,10 @@ func filepathMatches(matcher *patternmatcher.PatternMatcher, file string) (bool, // DetectArchiveReader detects whether the input stream is an archive or a // Dockerfile and returns a buffered version of input, safe to consume in lieu -// of input. If an archive is detected, isArchive is set to true, and to false +// of input. If an archive is detected, ok is set to true, and to false // otherwise, in which case it is safe to assume input represents the contents // of a Dockerfile. -func DetectArchiveReader(input io.ReadCloser) (rc io.ReadCloser, isArchive bool, err error) { +func DetectArchiveReader(input io.ReadCloser) (rc io.ReadCloser, ok bool, err error) { buf := bufio.NewReader(input) magic, err := buf.Peek(archiveHeaderSize * 2) @@ -141,12 +141,12 @@ func WriteTempDockerfile(rc io.ReadCloser) (dockerfileDir string, err error) { // Dockerfile or tar archive. Returns a tar archive used as a context and a // path to the Dockerfile inside the tar. func GetContextFromReader(rc io.ReadCloser, dockerfileName string) (out io.ReadCloser, relDockerfile string, err error) { - rc, isArchive, err := DetectArchiveReader(rc) + rc, ok, err := DetectArchiveReader(rc) if err != nil { return nil, "", err } - if isArchive { + if ok { return rc, dockerfileName, nil } @@ -171,7 +171,7 @@ func GetContextFromReader(rc io.ReadCloser, dockerfileName string) (out io.ReadC return newReadCloserWrapper(tarArchive, func() error { err := tarArchive.Close() - os.RemoveAll(dockerfileDir) + _ = os.RemoveAll(dockerfileDir) return err }), DefaultDockerfileName, nil } @@ -242,7 +242,7 @@ func getWithStatusError(url string) (resp *http.Response, err error) { } msg := fmt.Sprintf("failed to GET %s with status %s", url, resp.Status) body, err := io.ReadAll(resp.Body) - resp.Body.Close() + _ = resp.Body.Close() if err != nil { return nil, errors.Wrapf(err, "%s: error reading body", msg) } @@ -374,7 +374,7 @@ func isUNC(path string) bool { // the relative path to the dockerfile in the context. func AddDockerfileToBuildContext(dockerfileCtx io.ReadCloser, buildCtx io.ReadCloser) (io.ReadCloser, string, error) { file, err := io.ReadAll(dockerfileCtx) - dockerfileCtx.Close() + _ = dockerfileCtx.Close() if err != nil { return nil, "", err } @@ -438,17 +438,19 @@ func Compress(buildCtx io.ReadCloser) (io.ReadCloser, error) { go func() { compressWriter, err := compression.CompressStream(pipeWriter, archive.Gzip) if err != nil { - pipeWriter.CloseWithError(err) + _ = pipeWriter.CloseWithError(err) } - defer buildCtx.Close() + defer func() { + _ = buildCtx.Close() + }() if _, err := io.Copy(compressWriter, buildCtx); err != nil { - pipeWriter.CloseWithError(errors.Wrap(err, "failed to compress context")) - compressWriter.Close() + _ = pipeWriter.CloseWithError(errors.Wrap(err, "failed to compress context")) + _ = compressWriter.Close() return } - compressWriter.Close() - pipeWriter.Close() + _ = compressWriter.Close() + _ = pipeWriter.Close() }() return pipeReader, nil diff --git a/cli/command/image/build/context_test.go b/cli/command/image/build/context_test.go index 45533d6ebb4c..f7ebce2413f9 100644 --- a/cli/command/image/build/context_test.go +++ b/cli/command/image/build/context_test.go @@ -135,7 +135,7 @@ func TestGetContextFromReaderString(t *testing.T) { } buff := new(bytes.Buffer) - buff.ReadFrom(tarReader) + _, _ = buff.ReadFrom(tarReader) contents := buff.String() _, err = tarReader.Next() @@ -182,7 +182,7 @@ func TestGetContextFromReaderTar(t *testing.T) { } buff := new(bytes.Buffer) - buff.ReadFrom(tarReader) + _, _ = buff.ReadFrom(tarReader) contents := buff.String() _, err = tarReader.Next() @@ -263,7 +263,7 @@ func chdir(t *testing.T, dir string) { } func TestIsArchive(t *testing.T) { - testcases := []struct { + tests := []struct { doc string header []byte expected bool @@ -289,13 +289,15 @@ func TestIsArchive(t *testing.T) { expected: false, }, } - for _, testcase := range testcases { - assert.Check(t, is.Equal(testcase.expected, IsArchive(testcase.header)), testcase.doc) + for _, tc := range tests { + t.Run(tc.doc, func(t *testing.T) { + assert.Check(t, is.Equal(tc.expected, IsArchive(tc.header)), tc.doc) + }) } } func TestDetectArchiveReader(t *testing.T) { - testcases := []struct { + tests := []struct { file string desc string expected bool @@ -316,14 +318,18 @@ func TestDetectArchiveReader(t *testing.T) { expected: false, }, } - for _, testcase := range testcases { - content, err := os.Open(testcase.file) - assert.NilError(t, err) - defer content.Close() - - _, isArchive, err := DetectArchiveReader(content) - assert.NilError(t, err) - assert.Check(t, is.Equal(testcase.expected, isArchive), testcase.file) + for _, tc := range tests { + t.Run(tc.desc, func(t *testing.T) { + content, err := os.Open(tc.file) + assert.NilError(t, err) + defer func() { + _ = content.Close() + }() + + _, isArchive, err := DetectArchiveReader(content) + assert.NilError(t, err) + assert.Check(t, is.Equal(tc.expected, isArchive), tc.file) + }) } } diff --git a/cli/command/image/build/dockerignore.go b/cli/command/image/build/dockerignore.go index 357210437c03..8d11aa5e10d6 100644 --- a/cli/command/image/build/dockerignore.go +++ b/cli/command/image/build/dockerignore.go @@ -21,7 +21,9 @@ func ReadDockerignore(contextDir string) ([]string, error) { case err != nil: return nil, err } - defer f.Close() + defer func() { + _ = f.Close() + }() patterns, err := ignorefile.ReadAll(f) if err != nil { From 22487ad6f6990c373db82163d7797f601ff92b73 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 10 Oct 2025 17:23:57 +0200 Subject: [PATCH 2/5] cli/command/image/build: deprecate IsArchive utility It was only used internally. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 64be664e858d6ebcf0a119e349f6667ef67e7a83) Signed-off-by: Sebastiaan van Stijn --- cli/command/image/build/context.go | 10 +++++++++- cli/command/image/build/context_test.go | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/cli/command/image/build/context.go b/cli/command/image/build/context.go index a61185108c75..ca57d6ecb85f 100644 --- a/cli/command/image/build/context.go +++ b/cli/command/image/build/context.go @@ -108,7 +108,7 @@ func DetectArchiveReader(input io.ReadCloser) (rc io.ReadCloser, ok bool, err er return nil, false, errors.Errorf("failed to peek context header from STDIN: %v", err) } - return newReadCloserWrapper(buf, func() error { return input.Close() }), IsArchive(magic), nil + return newReadCloserWrapper(buf, func() error { return input.Close() }), isArchive(magic), nil } // WriteTempDockerfile writes a Dockerfile stream to a temporary file with a @@ -178,7 +178,15 @@ func GetContextFromReader(rc io.ReadCloser, dockerfileName string) (out io.ReadC // IsArchive checks for the magic bytes of a tar or any supported compression // algorithm. +// +// Deprecated: this utility was used internally and will be removed in the next release. func IsArchive(header []byte) bool { + return isArchive(header) +} + +// isArchive checks for the magic bytes of a tar or any supported compression +// algorithm. +func isArchive(header []byte) bool { if compression.Detect(header) != compression.None { return true } diff --git a/cli/command/image/build/context_test.go b/cli/command/image/build/context_test.go index f7ebce2413f9..8630d81146fe 100644 --- a/cli/command/image/build/context_test.go +++ b/cli/command/image/build/context_test.go @@ -291,7 +291,7 @@ func TestIsArchive(t *testing.T) { } for _, tc := range tests { t.Run(tc.doc, func(t *testing.T) { - assert.Check(t, is.Equal(tc.expected, IsArchive(tc.header)), tc.doc) + assert.Check(t, is.Equal(tc.expected, isArchive(tc.header)), tc.doc) }) } } From 7063a0ef077e2030b7c0d78c0e817e69d4c2a05d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 10 Oct 2025 17:25:04 +0200 Subject: [PATCH 3/5] cli/command/image: runBuild: inline vars and minor cleanups Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 9e646f6d922610eea8c3f6f4f4a3add184b3a183) Signed-off-by: Sebastiaan van Stijn --- cli/command/image/build.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/cli/command/image/build.go b/cli/command/image/build.go index c0b002390cb7..aebb421ee40a 100644 --- a/cli/command/image/build.go +++ b/cli/command/image/build.go @@ -184,7 +184,6 @@ func (out *lastProgressOutput) WriteProgress(prog progress.Progress) error { //nolint:gocyclo func runBuild(ctx context.Context, dockerCli command.Cli, options buildOptions) error { var ( - err error buildCtx io.ReadCloser dockerfileCtx io.ReadCloser contextDir string @@ -266,7 +265,7 @@ func runBuild(ctx context.Context, dockerCli command.Cli, options buildOptions) } if err := build.ValidateContextDirectory(contextDir, excludes); err != nil { - return errors.Wrap(err, "error checking context") + return errors.Wrap(err, "checking context") } // And canonicalize dockerfile name to a platform-independent one @@ -353,7 +352,6 @@ func runBuild(ctx context.Context, dockerCli command.Cli, options buildOptions) } } buildOpts := imageBuildOptions(dockerCli, options) - buildOpts.Version = buildtypes.BuilderV1 buildOpts.Dockerfile = relDockerfile buildOpts.AuthConfigs = authConfigs buildOpts.RemoteContext = remote @@ -363,7 +361,6 @@ func runBuild(ctx context.Context, dockerCli command.Cli, options buildOptions) if options.quiet { _, _ = fmt.Fprintf(dockerCli.Err(), "%s", progBuff) } - cancel() return err } defer response.Body.Close() @@ -380,7 +377,8 @@ func runBuild(ctx context.Context, dockerCli command.Cli, options buildOptions) err = jsonstream.Display(ctx, response.Body, streams.NewOut(buildBuff), jsonstream.WithAuxCallback(aux)) if err != nil { - if jerr, ok := err.(*jsonstream.JSONError); ok { + var jerr *jsonstream.JSONError + if errors.As(err, &jerr) { // If no error code is set, default to 1 if jerr.Code == 0 { jerr.Code = 1 @@ -544,6 +542,7 @@ func replaceDockerfileForContentTrust(ctx context.Context, inputTarStream io.Rea func imageBuildOptions(dockerCli command.Cli, options buildOptions) buildtypes.ImageBuildOptions { configFile := dockerCli.ConfigFile() return buildtypes.ImageBuildOptions{ + Version: buildtypes.BuilderV1, Memory: options.memory.Value(), MemorySwap: options.memorySwap.Value(), Tags: options.tags.GetSlice(), From 36d9523e317d958c4022d7a56c8bf85bbcb9c043 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 9 Oct 2025 13:46:33 +0200 Subject: [PATCH 4/5] opts: deprecate ValidateMACAddress It was a wrapper around net.ParseMAC from stdlib, so users should use that directly. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 17d6a929548ae0693affef54f36f03d27f00e603) Signed-off-by: Sebastiaan van Stijn --- cli/command/container/opts.go | 5 +++-- opts/opts.go | 2 ++ opts/opts_test.go | 14 -------------- 3 files changed, 5 insertions(+), 16 deletions(-) diff --git a/cli/command/container/opts.go b/cli/command/container/opts.go index 5ff39c4ee03f..b520d6b13a51 100644 --- a/cli/command/container/opts.go +++ b/cli/command/container/opts.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "fmt" + "net" "os" "path" "path/filepath" @@ -350,7 +351,7 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con // Validate the input mac address if copts.macAddress != "" { - if _, err := opts.ValidateMACAddress(copts.macAddress); err != nil { + if _, err := net.ParseMAC(strings.TrimSpace(copts.macAddress)); err != nil { return nil, errors.Errorf("%s is not a valid mac address", copts.macAddress) } } @@ -883,7 +884,7 @@ func parseNetworkAttachmentOpt(ep opts.NetworkAttachmentOpts) (*networktypes.End } } if ep.MacAddress != "" { - if _, err := opts.ValidateMACAddress(ep.MacAddress); err != nil { + if _, err := net.ParseMAC(strings.TrimSpace(ep.MacAddress)); err != nil { return nil, errors.Errorf("%s is not a valid mac address", ep.MacAddress) } epConfig.MacAddress = ep.MacAddress diff --git a/opts/opts.go b/opts/opts.go index 94eda0560a17..d1072f6ca11c 100644 --- a/opts/opts.go +++ b/opts/opts.go @@ -264,6 +264,8 @@ func ValidateIPAddress(val string) (string, error) { } // ValidateMACAddress validates a MAC address. +// +// Deprecated: use [net.ParseMAC]. This function will be removed in the next release. func ValidateMACAddress(val string) (string, error) { _, err := net.ParseMAC(strings.TrimSpace(val)) if err != nil { diff --git a/opts/opts_test.go b/opts/opts_test.go index 7dc87cad9e83..d49c7fb5fca4 100644 --- a/opts/opts_test.go +++ b/opts/opts_test.go @@ -396,20 +396,6 @@ func TestNamedMapOpts(t *testing.T) { } } -func TestValidateMACAddress(t *testing.T) { - if _, err := ValidateMACAddress(`92:d0:c6:0a:29:33`); err != nil { - t.Fatalf("ValidateMACAddress(`92:d0:c6:0a:29:33`) got %s", err) - } - - if _, err := ValidateMACAddress(`92:d0:c6:0a:33`); err == nil { - t.Fatalf("ValidateMACAddress(`92:d0:c6:0a:33`) succeeded; expected failure on invalid MAC") - } - - if _, err := ValidateMACAddress(`random invalid string`); err == nil { - t.Fatalf("ValidateMACAddress(`random invalid string`) succeeded; expected failure on invalid MAC") - } -} - func TestValidateLink(t *testing.T) { valid := []string{ "name", From d8bab717470622740d6b3007f35294d000cc6860 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 13 Oct 2025 11:47:39 +0200 Subject: [PATCH 5/5] opts: deprecate ListOpts.Delete() This method was added as part of a refactor in [moby@1ba1138], at which time it was used to delete original values for "--host" and "--volume" after normalizing. This beccame redundant in [moby@6200002], which added specialized options that used a validate function, which both validated and normalized inputs. It's no longer used, so let's mark it deprecated so that we can remove it. [moby@1ba1138]: https://github.com/moby/moby/commit/1ba11384bf82f824b0efbab31aaca439cfba1b4f [moby@6200002]: https://github.com/moby/moby/commit/6200002669874f3314856527fecd0c004060913c Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 193db8ec4171e17f2ef443ccc3ad77965f9629d8) Signed-off-by: Sebastiaan van Stijn --- opts/opts.go | 2 ++ opts/opts_test.go | 21 ++++++++------------- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/opts/opts.go b/opts/opts.go index d1072f6ca11c..c555c2d18884 100644 --- a/opts/opts.go +++ b/opts/opts.go @@ -60,6 +60,8 @@ func (opts *ListOpts) Set(value string) error { } // Delete removes the specified element from the slice. +// +// Deprecated: this method is no longer used and will be removed in the next release. func (opts *ListOpts) Delete(key string) { for i, k := range *opts.values { if k == key { diff --git a/opts/opts_test.go b/opts/opts_test.go index d49c7fb5fca4..f363a2a6440a 100644 --- a/opts/opts_test.go +++ b/opts/opts_test.go @@ -142,18 +142,14 @@ func TestListOptsWithoutValidator(t *testing.T) { if o.Get("baz") { t.Error(`o.Get("baz") == true`) } - o.Delete("foo") - if o.String() != "[bar bar]" { - t.Errorf("%s != [bar bar]", o.String()) + if listOpts := o.GetSlice(); len(listOpts) != 3 || listOpts[0] != "foo" || listOpts[1] != "bar" || listOpts[2] != "bar" { + t.Errorf("Expected [[foo bar bar]], got [%v]", listOpts) } - if listOpts := o.GetAll(); len(listOpts) != 2 || listOpts[0] != "bar" || listOpts[1] != "bar" { - t.Errorf("Expected [[bar bar]], got [%v]", listOpts) + if listOpts := o.GetAll(); len(listOpts) != 3 || listOpts[0] != "foo" || listOpts[1] != "bar" || listOpts[2] != "bar" { + t.Errorf("Expected [[foo bar bar]], got [%v]", listOpts) } - if listOpts := o.GetSlice(); len(listOpts) != 2 || listOpts[0] != "bar" || listOpts[1] != "bar" { - t.Errorf("Expected [[bar bar]], got [%v]", listOpts) - } - if mapListOpts := o.GetMap(); len(mapListOpts) != 1 { - t.Errorf("Expected [map[bar:{}]], got [%v]", mapListOpts) + if mapListOpts := o.GetMap(); len(mapListOpts) != 2 { + t.Errorf("Expected [map[bar:{} foo:{}]], got [%v]", mapListOpts) } } @@ -186,9 +182,8 @@ func TestListOptsWithValidator(t *testing.T) { if o.Get("baz") { t.Error(`o.Get("baz") == true`) } - o.Delete("valid-option2=2") - if o.String() != "" { - t.Errorf(`%s != ""`, o.String()) + if expected := "[valid-option2=2]"; o.String() != expected { + t.Errorf(`%s != %q`, o.String(), expected) } }