From 7f7dbaa04b6a24057bec415af2718b02d12fc783 Mon Sep 17 00:00:00 2001 From: Mariano Reingart Date: Mon, 13 Jan 2025 19:09:04 -0600 Subject: [PATCH] fix warnings and import cycle (UT/integration) --- cmd/skaffold/app/cmd/config/set_test.go | 2 +- docs-v2/content/en/schemas/v4beta12.json | 13 +++++++++++++ pkg/skaffold/build/cache/cache.go | 2 +- pkg/skaffold/build/cache/lookup.go | 6 +++--- pkg/skaffold/build/docker/docker.go | 7 +++++-- pkg/skaffold/build/docker/docker_test.go | 4 ++-- pkg/skaffold/build/docker/errors_test.go | 2 +- pkg/skaffold/build/local/types.go | 2 +- pkg/skaffold/config/util.go | 6 +++--- pkg/skaffold/docker/auth.go | 2 +- pkg/skaffold/docker/buildx.go | 2 +- pkg/skaffold/runner/runcontext/context.go | 6 ++---- pkg/skaffold/schema/latest/config.go | 2 +- 13 files changed, 35 insertions(+), 21 deletions(-) diff --git a/cmd/skaffold/app/cmd/config/set_test.go b/cmd/skaffold/app/cmd/config/set_test.go index b34845cfd59..48510a57ebb 100644 --- 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 index b8bad3b0334..c30694a2983 100755 --- 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 index d29a4a6a995..a05c88aae99 100644 --- 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 index 346cd99f66d..d7f2fc0d952 100644 --- 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 index caca5e9b371..f7994eed0fa 100644 --- 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 index 509e3a1804f..cfa739128c1 100644 --- 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 index 32686d38117..1138142c00c 100644 --- 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 index 1a4f07407ab..89ddf0f8b7e 100644 --- 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 index e020ee376a3..42ae8ffe083 100644 --- 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 index fec519a6489..d9b90b7de9c 100644 --- 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 index 29498322fda..784cc9f65ce 100644 --- 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 index b90558df3ff..544f48cf494 100644 --- 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 index 3c2cc71867b..b102715ec1f 100644 --- 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"`