Merge pull request #6560 from thaJeztah/28.x_backport_deprecations

[28.x backport] opts, image/build: deprecate IsArchive, ValidateMACAddress,  ListOpts.Delete()

Author: austinvazquez

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

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(),

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,18 +97,18 @@ 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)
 	if err != nil && err != io.EOF {
 		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
@@ -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,14 +171,22 @@ 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
 }
 
 // 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
 	}
@@ -242,7 +250,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 +382,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 +446,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

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)
+		})
 	}
 }
 

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 {

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 {
@@ -264,6 +266,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 {

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)
 	}
 }
 
@@ -396,20 +391,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",