fix warnings and import cycle (UT/integration)

reingart · reingart · commit 7f7dbaa04b6a · 2025-01-13T23:10:57.000-06:00
diff --git a/cmd/skaffold/app/cmd/config/set_test.go b/cmd/skaffold/app/cmd/config/set_test.go
@@ -415,7 +415,7 @@ func TestGetConfigStructWithIndex(t *testing.T) {
 			description: "survey flag set",
 			cfg:         &config.ContextConfig{},
 			survey:      true,
-			expectedIdx: []int{7},
+			expectedIdx: []int{9},
 		},
 		{
 			description: "no survey flag set",
diff --git a/docs-v2/content/en/schemas/v4beta12.json b/docs-v2/content/en/schemas/v4beta12.json
@@ -1766,6 +1766,18 @@
             "[\"golang:1.10.1-alpine3.7\", \"alpine:3.7\"]"
           ]
         },
+        "cacheTo": {
+          "items": {
+            "type": "string"
+          },
+          "type": "array",
+          "description": "the Docker images used as cache destination. If omitted, cacheFrom is used with max mode to export all layers.",
+          "x-intellij-html-description": "the Docker images used as cache destination. If omitted, cacheFrom is used with max mode to export all layers.",
+          "default": "[]",
+          "examples": [
+            "[\"type=registry,ref=gcr.io/k8s-skaffold/example:cache,mode=max\"]"
+          ]
+        },
         "cliFlags": {
           "items": {
             "type": "string"
@@ -1830,6 +1842,7 @@
         "network",
         "addHost",
         "cacheFrom",
+        "cacheTo",
         "cliFlags",
         "pullParent",
         "noCache",
diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go
@@ -121,7 +121,7 @@ func NewCache(ctx context.Context, cfg Config, isLocalImage func(imageName strin
 	}
 
 	// for backward compatibility, extended build capabilities with BuildKit are disabled by default
-	buildx := cfg.DetectBuildX()
+	buildx := cfg.DetectBuildX() && docker.IsBuildXDetected()
 
 	return &cache{
 		artifactCache:      artifactCache,
diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go
@@ -87,7 +87,7 @@ func (c *cache) lookup(ctx context.Context, out io.Writer, a *latest.Artifact, t
 	}
 
 	if isLocal, err := c.isLocalImage(a.ImageName); err != nil {
-		log.Entry(ctx).Debugf("isLocalImage failed %w", err)
+		log.Entry(ctx).Debugf("isLocalImage failed %v", err)
 		return failed{err}
 	} else if isLocal {
 		return c.lookupLocal(ctx, hash, tag, entry)
@@ -128,7 +128,7 @@ func (c *cache) lookupRemote(ctx context.Context, hash, tag string, platforms []
 			return found{hash: hash}
 		}
 	} else {
-		log.Entry(ctx).Debugf("RemoteDigest error %w", err)
+		log.Entry(ctx).Debugf("RemoteDigest error %v", err)
 	}
 
 	// Image exists remotely with a different tag
@@ -162,7 +162,7 @@ func (c *cache) tryImport(ctx context.Context, a *latest.Artifact, tag string, h
 		_, err := c.client.ServerVersion(ctx)
 		load = err == nil
 		if !load {
-			log.Entry(ctx).Debugf("Docker client error, disabling image load as using buildx: %w", err)
+			log.Entry(ctx).Debugf("Docker client error, disabling image load as using buildx: %v", err)
 		}
 	}
 	if load {
diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go
@@ -143,6 +143,9 @@ func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, name string
 	var metadata *os.File
 	if b.buildx {
 		metadata, err = os.CreateTemp("", "metadata.json")
+		if err != nil {
+			return "", fmt.Errorf("unable create temp file: %w", err)
+		}
 		metadata.Close()
 		defer os.Remove(metadata.Name())
 		args = append(args, "--metadata-file", metadata.Name())
@@ -215,7 +218,7 @@ func (b *Builder) adjustCache(ctx context.Context, a *latest.Artifact, artifactT
 		tag, _ := config.GetCacheTag(b.cfg.GlobalConfig())
 		imgRef, err := docker.ParseReference(artifactTag)
 		if err != nil {
-			log.Entry(ctx).Errorf("couldn't parse image tag: %w", err)
+			log.Entry(ctx).Errorf("couldn't parse image tag: %v", err)
 		} else if tag != "" {
 			cacheTag = fmt.Sprintf("%s/%s:%s", imgRef.Repo, cacheRef, tag)
 		}
@@ -273,7 +276,7 @@ func getBuildxDigest(ctx context.Context, filename string) (string, error) {
 			return digest, nil
 		}
 	}
-	log.Entry(ctx).Warnf("No digest found in buildx metadata: %w", err)
+	log.Entry(ctx).Warnf("No digest found in buildx metadata: %v", err)
 	// if image is not pushed, it could not contain the digest log for debugging:
 	log.Entry(ctx).Debugf("Full buildx metadata: %s", data)
 	return "", err
diff --git a/pkg/skaffold/build/docker/docker_test.go b/pkg/skaffold/build/docker/docker_test.go
@@ -187,7 +187,7 @@ func TestDockerCLIBuild(t *testing.T) {
 			}
 			t.Override(&util.OSEnviron, func() []string { return []string{"KEY=VALUE"} })
 
-			builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv(test.extraEnv), test.cfg, test.localBuild.UseDockerCLI, test.localBuild.UseBuildkit, false, mockArtifactResolver{make(map[string]string)}, nil)
+			builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv(test.extraEnv), test.cfg, test.localBuild.UseDockerCLI, test.localBuild.UseBuildkit, false, false, mockArtifactResolver{make(map[string]string)}, nil)
 
 			artifact := &latest.Artifact{
 				Workspace: ".",
@@ -268,7 +268,7 @@ func TestDockerCLICheckCacheFromArgs(t *testing.T) {
 			)
 			t.Override(&util.DefaultExecCommand, mockCmd)
 
-			builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}), mockConfig{}, true, util.Ptr(false), false, mockArtifactResolver{make(map[string]string)}, nil)
+			builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}), mockConfig{}, true, util.Ptr(false), false, false, mockArtifactResolver{make(map[string]string)}, nil)
 			_, err := builder.Build(context.Background(), io.Discard, &a, test.tag, platform.Matcher{})
 			t.CheckNoError(err)
 		})
diff --git a/pkg/skaffold/build/docker/errors_test.go b/pkg/skaffold/build/docker/errors_test.go
@@ -60,7 +60,7 @@ Refer https://skaffold.dev/docs/references/yaml/#build-artifacts-docker for deta
 				"docker build . --file "+dockerfilePath+" -t tag",
 			))
 			t.Override(&docker.DefaultAuthHelper, stubAuth{})
-			builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}), mockConfig{}, true, nil, false, mockArtifactResolver{make(map[string]string)}, nil)
+			builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}), mockConfig{}, true, nil, false, false, mockArtifactResolver{make(map[string]string)}, nil)
 
 			artifact := &latest.Artifact{
 				ImageName: "test-image",
diff --git a/pkg/skaffold/build/local/types.go b/pkg/skaffold/build/local/types.go
@@ -106,7 +106,7 @@ func NewBuilder(ctx context.Context, bCtx BuilderContext, buildCfg *latest.Local
 	tryImportMissing := buildCfg.TryImportMissing
 
 	// for backward compatibility, extended build capabilities with BuildKit are disabled by default
-	buildx := bCtx.DetectBuildX()
+	buildx := bCtx.DetectBuildX() && docker.IsBuildXDetected()
 
 	return &Builder{
 		local:              *buildCfg,
diff --git a/pkg/skaffold/config/util.go b/pkg/skaffold/config/util.go
@@ -220,7 +220,7 @@ func GetDebugHelpersRegistry(configFile string) (string, error) {
 func GetCacheTag(configFile string) (string, error) {
 	cfg, err := GetConfigForCurrentKubectx(configFile)
 	if err != nil {
-		log.Entry(context.TODO()).Errorf("Cannot read cache-tag from config: %w", err)
+		log.Entry(context.TODO()).Errorf("Cannot read cache-tag from config: %v", err)
 		return "", err
 	}
 	if cfg.CacheTag != "" {
@@ -232,9 +232,9 @@ func GetCacheTag(configFile string) (string, error) {
 func GetDetectBuildX(configFile string) bool {
 	cfg, err := GetConfigForCurrentKubectx(configFile)
 	if err != nil {
-		log.Entry(context.TODO()).Errorf("Cannot read detect-buildx option from config: %w", err)
+		log.Entry(context.TODO()).Errorf("Cannot read detect-buildx option from config: %v", err)
 	} else if cfg.DetectBuildX != nil {
-		log.Entry(context.TODO()).Infof("Using detect-buildx=%s from config", *cfg.DetectBuildX)
+		log.Entry(context.TODO()).Infof("Using detect-buildx=%t from config", *cfg.DetectBuildX)
 		return *cfg.DetectBuildX
 	}
 	return false
diff --git a/pkg/skaffold/docker/auth.go b/pkg/skaffold/docker/auth.go
@@ -74,7 +74,7 @@ func LoadDockerConfig(access bool) (*configfile.ConfigFile, error) {
 			defer file.Close()
 		}
 		if err != nil {
-			log.Entry(context.TODO()).Warnf("cannot access docker config file %s: %w", configDir, err)
+			log.Entry(context.TODO()).Warnf("cannot access docker config file %s: %v", configDir, err)
 		}
 	}
 	return cf, err
diff --git a/pkg/skaffold/docker/buildx.go b/pkg/skaffold/docker/buildx.go
@@ -16,7 +16,7 @@ limitations under the License.
 
 package docker
 
-func DetectBuildX() bool {
+func IsBuildXDetected() bool {
 	// detect if buildx was installed (docker build then is an alias for docker buildx build):
 	// https://github.com/docker/buildx/blob/master/commands/install.go
 	cf, err := LoadDockerConfig(true)
diff --git a/pkg/skaffold/runner/runcontext/context.go b/pkg/skaffold/runner/runcontext/context.go
@@ -27,7 +27,6 @@ import (
 
 	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/config"
 	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/constants"
-	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/docker"
 	kubectx "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/kubernetes/context"
 	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output/log"
 	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest"
@@ -367,9 +366,8 @@ func (rc *RunContext) EnableGKEARMNodeTolerationInRenderedManifests() bool {
 
 func (rc *RunContext) DetectBuildX() bool {
 	if config.GetDetectBuildX(rc.GlobalConfig()) {
-		buildx := docker.DetectBuildX()
-		log.Entry(context.TODO()).Debugf("buildx detection result is %t", buildx)
-		return buildx
+		log.Entry(context.TODO()).Debugf("buildx detection is enabled")
+		return true
 	} else {
 		log.Entry(context.TODO()).Debugf("buildx detection is disabled")
 		return false
diff --git a/pkg/skaffold/schema/latest/config.go b/pkg/skaffold/schema/latest/config.go
@@ -1590,7 +1590,7 @@ type DockerArtifact struct {
 	CacheFrom []string `yaml:"cacheFrom,omitempty"`
 
 	// CacheTo lists the Docker images used as cache destination.
-	// If ommited, cacheFrom is used with max mode to export all layers
+	// If omitted, cacheFrom is used with max mode to export all layers.
 	// For example: `["type=registry,ref=gcr.io/k8s-skaffold/example:cache,mode=max"]`.
 	CacheTo []string `yaml:"cacheTo,omitempty"`
 

Original file line number	Diff line number	Diff line change
`@@ -415,7 +415,7 @@ func TestGetConfigStructWithIndex(t *testing.T) {`
`415`	`415`	`description: "survey flag set",`
`416`	`416`	`cfg: &config.ContextConfig{},`
`417`	`417`	`survey: true,`
`418`		`- expectedIdx: []int{7},`
	`418`	`+ expectedIdx: []int{9},`
`419`	`419`	`},`
`420`	`420`	`{`
`421`	`421`	`description: "no survey flag set",`
Original file line number	Diff line number	Diff line change
`@@ -121,7 +121,7 @@ func NewCache(ctx context.Context, cfg Config, isLocalImage func(imageName strin`
`121`	`121`	`}`
`122`	`122`
`123`	`123`	`// for backward compatibility, extended build capabilities with BuildKit are disabled by default`
`124`		`- buildx := cfg.DetectBuildX()`
	`124`	`+ buildx := cfg.DetectBuildX() && docker.IsBuildXDetected()`
`125`	`125`
`126`	`126`	`return &cache{`
`127`	`127`	`artifactCache: artifactCache,`
Original file line number	Diff line number	Diff line change
`@@ -87,7 +87,7 @@ func (c cache) lookup(ctx context.Context, out io.Writer, a latest.Artifact, t`
`87`	`87`	`}`
`88`	`88`
`89`	`89`	`if isLocal, err := c.isLocalImage(a.ImageName); err != nil {`
`90`		`- log.Entry(ctx).Debugf("isLocalImage failed %w", err)`
	`90`	`+ log.Entry(ctx).Debugf("isLocalImage failed %v", err)`
`91`	`91`	`return failed{err}`
`92`	`92`	`} else if isLocal {`
`93`	`93`	`return c.lookupLocal(ctx, hash, tag, entry)`
`@@ -128,7 +128,7 @@ func (c *cache) lookupRemote(ctx context.Context, hash, tag string, platforms []`
`128`	`128`	`return found{hash: hash}`
`129`	`129`	`}`
`130`	`130`	`} else {`
`131`		`- log.Entry(ctx).Debugf("RemoteDigest error %w", err)`
	`131`	`+ log.Entry(ctx).Debugf("RemoteDigest error %v", err)`
`132`	`132`	`}`
`133`	`133`
`134`	`134`	`// Image exists remotely with a different tag`
`@@ -162,7 +162,7 @@ func (c cache) tryImport(ctx context.Context, a latest.Artifact, tag string, h`
`162`	`162`	`_, err := c.client.ServerVersion(ctx)`
`163`	`163`	`load = err == nil`
`164`	`164`	`if !load {`
`165`		`- log.Entry(ctx).Debugf("Docker client error, disabling image load as using buildx: %w", err)`
	`165`	`+ log.Entry(ctx).Debugf("Docker client error, disabling image load as using buildx: %v", err)`
`166`	`166`	`}`
`167`	`167`	`}`
`168`	`168`	`if load {`
Original file line number	Diff line number	Diff line change
`@@ -187,7 +187,7 @@ func TestDockerCLIBuild(t *testing.T) {`
`187`	`187`	`}`
`188`	`188`	`t.Override(&util.OSEnviron, func() []string { return []string{"KEY=VALUE"} })`
`189`	`189`
`190`		`- builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv(test.extraEnv), test.cfg, test.localBuild.UseDockerCLI, test.localBuild.UseBuildkit, false, mockArtifactResolver{make(map[string]string)}, nil)`
	`190`	`+ builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv(test.extraEnv), test.cfg, test.localBuild.UseDockerCLI, test.localBuild.UseBuildkit, false, false, mockArtifactResolver{make(map[string]string)}, nil)`
`191`	`191`
`192`	`192`	`artifact := &latest.Artifact{`
`193`	`193`	`Workspace: ".",`
`@@ -268,7 +268,7 @@ func TestDockerCLICheckCacheFromArgs(t *testing.T) {`
`268`	`268`	`)`
`269`	`269`	`t.Override(&util.DefaultExecCommand, mockCmd)`
`270`	`270`
`271`		`- builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}), mockConfig{}, true, util.Ptr(false), false, mockArtifactResolver{make(map[string]string)}, nil)`
	`271`	`+ builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}), mockConfig{}, true, util.Ptr(false), false, false, mockArtifactResolver{make(map[string]string)}, nil)`
`272`	`272`	`_, err := builder.Build(context.Background(), io.Discard, &a, test.tag, platform.Matcher{})`
`273`	`273`	`t.CheckNoError(err)`
`274`	`274`	`})`
Original file line number	Diff line number	Diff line change
`@@ -74,7 +74,7 @@ func LoadDockerConfig(access bool) (*configfile.ConfigFile, error) {`
`74`	`74`	`defer file.Close()`
`75`	`75`	`}`
`76`	`76`	`if err != nil {`
`77`		`- log.Entry(context.TODO()).Warnf("cannot access docker config file %s: %w", configDir, err)`
	`77`	`+ log.Entry(context.TODO()).Warnf("cannot access docker config file %s: %v", configDir, err)`
`78`	`78`	`}`
`79`	`79`	`}`
`80`	`80`	`return cf, err`