Skip to content
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.

Sign up for GitHub

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

fix: Scope Issue with the 'entry' variable when looking up remote images and tests additions #9209

Closed
wants to merge 2 commits into from
Closed

fix: Scope Issue with the 'entry' variable when looking up remote images and tests additions #9209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
158695a
fix: new retrieved entry is not used in the lookup remote functionali…
cydan33 Dec 5, 2023
2fdb67a
fix: conventions inside the tests
cydan33 Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions pkg/skaffold/build/cache/lookup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,11 @@ func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tag string, plat
c.cacheMutex.RUnlock()

pls := platforms.GetPlatforms(a.ImageName)
// TODO (gaghosh): allow `tryImport` when the Docker daemon starts supporting multiarch images
// See https://github.com/docker/buildx/issues/1220#issuecomment-1189996403

if isLocal, err := c.isLocalImage(a.ImageName); err != nil {
return failed{err}
} else if isLocal {
// TODO (gaghosh): allow `tryImport` when the Docker daemon starts supporting multiarch images
// See https://github.com/docker/buildx/issues/1220#issuecomment-1189996403
if !cacheHit && !pls.IsMultiPlatform() {
var pl v1.Platform
if len(pls.Platforms) == 1 {
Expand All @@ -92,17 +91,20 @@ func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tag string, plat
return c.lookupLocal(ctx, hash, tag, entry)
}
if !cacheHit {
entry := ImageDetails{}
if digest, err := docker.RemoteDigest(tag, c.cfg, nil); err == nil {
log.Entry(ctx).Debugf("Added digest for %s to cache entry", tag)
entry.Digest = digest
entry.ID = ""
} else {
log.Entry(ctx).Debugf("Could not get remote digest from docker, building instead (%s)", err)
return needsBuilding{hash: hash}
}
log.Entry(ctx).Debugf("remote digest Error %s", err)

c.cacheMutex.Lock()
c.artifactCache[hash] = entry
c.cacheMutex.Unlock()

// No need to lookup remote if we have already found the image remotely
return found{hash: hash}
}
return c.lookupRemote(ctx, hash, tag, pls.Platforms, entry)
}
Expand Down
9 changes: 1 addition & 8 deletions pkg/skaffold/build/cache/lookup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,6 @@ func TestLookupRemote(t *testing.T) {
api *testutil.FakeAPIClient
expected cacheDetails
}{
{
description: "miss",
hasher: mockHasher{"hash"},
api: &testutil.FakeAPIClient{ErrImagePull: true},
cache: map[string]ImageDetails{},
expected: needsBuilding{hash: "hash"},
},
{
description: "hash failure",
hasher: failingHasher{errors.New("BUG")},
Expand Down Expand Up @@ -222,7 +215,7 @@ func TestLookupRemote(t *testing.T) {

// cmp.Diff cannot access unexported fields in *exec.Cmd, so use reflect.DeepEqual here directly
if !reflect.DeepEqual(test.expected, details[0]) {
t.Errorf("Expected result different from actual result. Expected: \n%v, \nActual: \n%v", test.expected, details)
t.Errorf("Expected result different from actual result. Expected: \n\"%v\", \nActual: \n\"%v\"", test.expected, details[0])
}
})
}
Expand Down
41 changes: 29 additions & 12 deletions pkg/skaffold/build/cache/retrieve_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,40 +206,49 @@ func TestCacheBuildRemote(t *testing.T) {
Write("dep1", "content1").
Write("dep2", "content2").
Write("dep3", "content3").
Write("dep4", "content4").
Write("dep5", "content5").
Chdir()

tags := map[string]string{
"artifact1": "artifact1:tag1",
"artifact2": "artifact2:tag2",
"exist_artifact1": "exist_artifact1:tag1",
"exist_artifact2": "exist_artifact2:tag2",
}
artifacts := []*latest.Artifact{
{ImageName: "artifact1", ArtifactType: latest.ArtifactType{DockerArtifact: &latest.DockerArtifact{}}},
{ImageName: "artifact2", ArtifactType: latest.ArtifactType{DockerArtifact: &latest.DockerArtifact{}}},
{ImageName: "exist_artifact1", ArtifactType: latest.ArtifactType{DockerArtifact: &latest.DockerArtifact{}}},
{ImageName: "exist_artifact2", ArtifactType: latest.ArtifactType{DockerArtifact: &latest.DockerArtifact{}}},
}
deps := depLister(map[string][]string{
"artifact1": {"dep1", "dep2"},
"artifact2": {"dep3"},
"exist_artifact1": {"dep4"},
"exist_artifact2": {"dep5"},
})
tagToDigest := map[string]string{
"exist_artifact1:tag1": "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165",
"exist_artifact2:tag2": "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539",
}

// Mock Docker
api := &testutil.FakeAPIClient{}
api = api.Add("artifact1:tag1", "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165")
api = api.Add("artifact2:tag2", "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539")
for tag, digest := range tagToDigest {
api = api.Add(tag, digest)
}

dockerDaemon := fakeLocalDaemon(api)
t.Override(&docker.NewAPIClient, func(context.Context, docker.Config) (docker.LocalDaemon, error) {
return dockerDaemon, nil
})
t.Override(&docker.DefaultAuthHelper, stubAuth{})
t.Override(&docker.RemoteDigest, func(ref string, _ docker.Config, _ []specs.Platform) (string, error) {
switch ref {
case "artifact1:tag1":
return "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165", nil
case "artifact2:tag2":
return "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539", nil
default:
return "", errors.New("unknown remote tag")
if digest, ok := tagToDigest[ref]; ok {
return digest, nil
}
return "", errors.New("unknown remote tag")
})

// Mock args builder
Expand All @@ -264,29 +273,37 @@ func TestCacheBuildRemote(t *testing.T) {

t.CheckNoError(err)
t.CheckDeepEqual(2, len(builder.built))
t.CheckDeepEqual(2, len(bRes))
t.CheckDeepEqual(4, len(bRes))
// Artifacts should always be returned in their original order
t.CheckDeepEqual("artifact1", bRes[0].ImageName)
t.CheckDeepEqual("artifact2", bRes[1].ImageName)

// Add the other tags to the remote cache
tagToDigest["artifact1:tag1"] = tagToDigest["exist_artifact1:tag1"]
tagToDigest["artifact2:tag2"] = tagToDigest["exist_artifact2:tag2"]
api.Add("artifact1:tag1", tagToDigest["artifact1:tag1"])
api.Add("artifact2:tag2", tagToDigest["artifact2:tag2"])

// Second build: both artifacts are read from cache
builder = &mockBuilder{dockerDaemon: dockerDaemon, push: true, cache: artifactCache}
bRes, err = artifactCache.Build(context.Background(), io.Discard, tags, artifacts, platform.Resolver{}, builder.Build)

t.CheckNoError(err)
t.CheckEmpty(builder.built)
t.CheckDeepEqual(2, len(bRes))
t.CheckDeepEqual(4, len(bRes))
t.CheckDeepEqual("artifact1", bRes[0].ImageName)
t.CheckDeepEqual("artifact2", bRes[1].ImageName)

// Third build: change one artifact's dependencies
tmpDir.Write("dep1", "new content")
tags["artifact1"] = "artifact1:new_tag1"

builder = &mockBuilder{dockerDaemon: dockerDaemon, push: true, cache: artifactCache}
bRes, err = artifactCache.Build(context.Background(), io.Discard, tags, artifacts, platform.Resolver{}, builder.Build)

t.CheckNoError(err)
t.CheckDeepEqual(1, len(builder.built))
t.CheckDeepEqual(2, len(bRes))
t.CheckDeepEqual(4, len(bRes))
t.CheckDeepEqual("artifact1", bRes[0].ImageName)
t.CheckDeepEqual("artifact2", bRes[1].ImageName)
})
Expand Down
Loading