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.

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

Ignore cache-from pull errors #1604

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 5 additions & 2 deletions pkg/skaffold/build/gcb/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
package gcb

import (
"fmt"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
cloudbuild "google.golang.org/api/cloudbuild/v1"
Expand All @@ -27,8 +29,9 @@ func (b *Builder) dockerBuildSteps(artifact *latest.DockerArtifact, tag string)

for _, cacheFrom := range artifact.CacheFrom {
steps = append(steps, &cloudbuild.BuildStep{
Name: b.DockerImage,
Args: []string{"pull", cacheFrom},
Name: b.DockerImage,
Entrypoint: "sh",
Args: []string{"-c", fmt.Sprintf("docker pull %s || true", cacheFrom)},
})
}

Expand Down
10 changes: 6 additions & 4 deletions pkg/skaffold/build/gcb/docker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,13 @@ func TestPullCacheFrom(t *testing.T) {
steps := builder.dockerBuildSteps(artifact, "nginx2")

expected := []*cloudbuild.BuildStep{{
Name: "docker/docker",
Args: []string{"pull", "from/image1"},
Name: "docker/docker",
Entrypoint: "sh",
Args: []string{"-c", "docker pull from/image1 || true"},
}, {
Name: "docker/docker",
Args: []string{"pull", "from/image2"},
Name: "docker/docker",
Entrypoint: "sh",
Args: []string{"-c", "docker pull from/image2 || true"},
}, {
Name: "docker/docker",
Args: []string{"build", "--tag", "nginx2", "-f", "Dockerfile", "--cache-from", "from/image1", "--cache-from", "from/image2", "."},
Expand Down
3 changes: 2 additions & 1 deletion pkg/skaffold/build/local/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/warnings"
"github.com/pkg/errors"
)

Expand Down Expand Up @@ -90,7 +91,7 @@ func (b *Builder) pullCacheFromImages(ctx context.Context, out io.Writer, a *lat
}

if err := b.localDocker.Pull(ctx, out, image); err != nil {
return errors.Wrapf(err, "pulling image %s", image)
warnings.Printf("Cache-From image couldn't be pulled: %s\n", image)
}
}

Expand Down
31 changes: 21 additions & 10 deletions pkg/skaffold/build/local/local_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/warnings"
"github.com/GoogleContainerTools/skaffold/testutil"
"github.com/docker/docker/api/types"
)
Expand Down Expand Up @@ -55,13 +56,14 @@ func TestLocalRun(t *testing.T) {
docker.DefaultAuthHelper = testAuthHelper{}

var tests = []struct {
description string
api testutil.FakeAPIClient
tagger tag.Tagger
artifacts []*latest.Artifact
expected []build.Artifact
pushImages bool
shouldErr bool
description string
api testutil.FakeAPIClient
tagger tag.Tagger
artifacts []*latest.Artifact
expected []build.Artifact
expectedWarnings []string
pushImages bool
shouldErr bool
}{
{
description: "single build (local)",
Expand Down Expand Up @@ -165,7 +167,7 @@ func TestLocalRun(t *testing.T) {
}},
},
{
description: "pull error",
description: "ignore cache-from pull error",
artifacts: []*latest.Artifact{{
ImageName: "gcr.io/test/image",
ArtifactType: latest.ArtifactType{
Expand All @@ -177,8 +179,12 @@ func TestLocalRun(t *testing.T) {
api: testutil.FakeAPIClient{
ErrImagePull: true,
},
tagger: &FakeTagger{Out: "gcr.io/test/image:tag"},
shouldErr: true,
tagger: &FakeTagger{Out: "gcr.io/test/image:tag"},
expected: []build.Artifact{{
ImageName: "gcr.io/test/image",
Tag: "gcr.io/test/image:1",
}},
expectedWarnings: []string{"Cache-From image couldn't be pulled: pull1\n"},
},
{
description: "inspect error",
Expand All @@ -200,6 +206,10 @@ func TestLocalRun(t *testing.T) {

for _, test := range tests {
t.Run(test.description, func(t *testing.T) {
defer func(w warnings.Warner) { warnings.Printf = w }(warnings.Printf)
fakeWarner := &warnings.Collect{}
warnings.Printf = fakeWarner.Warnf

l := Builder{
cfg: &latest.LocalBuild{},
localDocker: docker.NewLocalDaemon(&test.api, nil),
Expand All @@ -209,6 +219,7 @@ func TestLocalRun(t *testing.T) {
res, err := l.Build(context.Background(), ioutil.Discard, test.tagger, test.artifacts)

testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, res)
testutil.CheckDeepEqual(t, test.expectedWarnings, fakeWarner.Warnings)
})
}
}
18 changes: 2 additions & 16 deletions pkg/skaffold/build/tag/env_template.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,24 +22,10 @@ import (

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/warnings"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
)

// for testing
var warner Warner = &logrusWarner{}

// Warner shows warnings.
type Warner interface {
Warnf(format string, args ...interface{})
}

type logrusWarner struct{}

func (l *logrusWarner) Warnf(format string, args ...interface{}) {
logrus.Warnf(format, args...)
}

// envTemplateTagger implements Tagger
type envTemplateTagger struct {
Template *template.Template
Expand Down Expand Up @@ -78,7 +64,7 @@ func (t *envTemplateTagger) GenerateFullyQualifiedImageName(workingDir, imageNam
if strings.Contains(tag, "_DEPRECATED_DIGEST_") ||
strings.Contains(tag, "_DEPRECATED_DIGEST_ALGO_") ||
strings.Contains(tag, "_DEPRECATED_DIGEST_HEX_") {
warner.Warnf("{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be appended to image tags")
warnings.Printf("{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be appended to image tags")

switch {
case strings.HasSuffix(tag, "@_DEPRECATED_DIGEST_"):
Expand Down
20 changes: 5 additions & 15 deletions pkg/skaffold/build/tag/env_template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,13 @@ limitations under the License.
package tag

import (
"fmt"
"sort"
"testing"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/warnings"
"github.com/GoogleContainerTools/skaffold/testutil"
)

type fakeWarner struct {
warnings []string
}

func (l *fakeWarner) Warnf(format string, args ...interface{}) {
l.warnings = append(l.warnings, fmt.Sprintf(format, args...))
sort.Strings(l.warnings)
}

func TestEnvTemplateTagger_GenerateFullyQualifiedImageName(t *testing.T) {
tests := []struct {
name string
Expand Down Expand Up @@ -105,17 +95,17 @@ func TestEnvTemplateTagger_GenerateFullyQualifiedImageName(t *testing.T) {
return test.env
}

defer func(w Warner) { warner = w }(warner)
fakeWarner := &fakeWarner{}
warner = fakeWarner
defer func(w warnings.Warner) { warnings.Printf = w }(warnings.Printf)
fakeWarner := &warnings.Collect{}
warnings.Printf = fakeWarner.Warnf

c, err := NewEnvTemplateTagger(test.template)
testutil.CheckError(t, false, err)

got, err := c.GenerateFullyQualifiedImageName("", test.imageName)

testutil.CheckErrorAndDeepEqual(t, false, err, test.expected, got)
testutil.CheckDeepEqual(t, test.expectedWarnings, fakeWarner.warnings)
testutil.CheckDeepEqual(t, test.expectedWarnings, fakeWarner.Warnings)
})
}
}
Expand Down
8 changes: 3 additions & 5 deletions pkg/skaffold/deploy/kubectl/images.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,9 @@ import (
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/warnings"
)

// for testing
var warner Warner = &logrusWarner{}

// ReplaceImages replaces image names in a list of manifests.
func (l *ManifestList) ReplaceImages(builds []build.Artifact, defaultRepo string) (ManifestList, error) {
replacer := newImageReplacer(builds, defaultRepo)
Expand Down Expand Up @@ -87,7 +85,7 @@ func (r *imageReplacer) NewValue(old interface{}) (bool, interface{}) {
func (r *imageReplacer) parseAndReplace(image string) (bool, interface{}) {
parsed, err := docker.ParseReference(image)
if err != nil {
warner.Warnf("Couldn't parse image: %s", image)
warnings.Printf("Couldn't parse image: %s", image)
return false, nil
}

Expand All @@ -107,7 +105,7 @@ func (r *imageReplacer) parseAndReplace(image string) (bool, interface{}) {
func (r *imageReplacer) Check() {
for imageName := range r.tagsByImageName {
if !r.found[imageName] {
warner.Warnf("image [%s] is not used by the deployment", imageName)
warnings.Printf("image [%s] is not used by the deployment", imageName)
}
}
}
Expand Down
20 changes: 5 additions & 15 deletions pkg/skaffold/deploy/kubectl/images_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,13 @@ limitations under the License.
package kubectl

import (
"fmt"
"sort"
"testing"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/warnings"
"github.com/GoogleContainerTools/skaffold/testutil"
)

type fakeWarner struct {
warnings []string
}

func (l *fakeWarner) Warnf(format string, args ...interface{}) {
l.warnings = append(l.warnings, fmt.Sprintf(format, args...))
sort.Strings(l.warnings)
}

func TestReplaceImages(t *testing.T) {
manifests := ManifestList{[]byte(`
apiVersion: v1
Expand Down Expand Up @@ -96,9 +86,9 @@ spec:
- image: in valid
`)}

defer func(w Warner) { warner = w }(warner)
fakeWarner := &fakeWarner{}
warner = fakeWarner
defer func(w warnings.Warner) { warnings.Printf = w }(warnings.Printf)
fakeWarner := &warnings.Collect{}
warnings.Printf = fakeWarner.Warnf

resultManifest, err := manifests.ReplaceImages(builds, "")

Expand All @@ -107,7 +97,7 @@ spec:
"Couldn't parse image: in valid",
"image [skaffold/unused] is not used by the deployment",
"image [skaffold/usedwrongfqn] is not used by the deployment",
}, fakeWarner.warnings)
}, fakeWarner.Warnings)
}

func TestReplaceEmptyManifest(t *testing.T) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,29 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package kubectl
package warnings

import "github.com/sirupsen/logrus"
import (
"fmt"
"sort"

// Warner shows warnings.
type Warner interface {
Warnf(format string, args ...interface{})
}
"github.com/sirupsen/logrus"
)

// Warner prints warnings
type Warner func(format string, args ...interface{})

type logrusWarner struct{}
// Printf can be overridden for testing
var Printf = logrus.Warnf

// Collect is used for testing to collect warnings
// instead of printing them
type Collect struct {
Warnings []string
}

func (l *logrusWarner) Warnf(format string, args ...interface{}) {
logrus.Warnf(format, args...)
// Warnf collects all the warnings for unit tests
func (l *Collect) Warnf(format string, args ...interface{}) {
l.Warnings = append(l.Warnings, fmt.Sprintf(format, args...))
sort.Strings(l.Warnings)
}