From d767708246eb1a67a627aa7cecdc2e9b714a2902 Mon Sep 17 00:00:00 2001 From: Jason Hall Date: Sat, 31 Oct 2020 12:55:28 -0400 Subject: [PATCH] IsSupportedReference returns descriptive error (#233) This can be useful to determine what they need to do to make a ko publish work. --- pkg/build/build.go | 7 ++++--- pkg/build/gobuild.go | 11 +++++++---- pkg/build/gobuild_test.go | 16 ++++++++-------- pkg/build/limit.go | 2 +- pkg/build/limit_test.go | 4 ++-- pkg/build/recorder.go | 2 +- pkg/build/recorder_test.go | 14 +++++--------- pkg/build/shared.go | 2 +- pkg/build/shared_test.go | 8 +++----- pkg/commands/publisher.go | 4 ++-- pkg/internal/testing/fixed.go | 9 ++++++--- pkg/internal/testing/fixed_test.go | 8 ++++---- pkg/resolve/resolve.go | 2 +- 13 files changed, 45 insertions(+), 44 deletions(-) diff --git a/pkg/build/build.go b/pkg/build/build.go index 692ceda570..3df5c605d0 100644 --- a/pkg/build/build.go +++ b/pkg/build/build.go @@ -24,10 +24,11 @@ import ( // Interface abstracts different methods for turning a supported importpath // reference into a v1.Image. type Interface interface { - // IsSupportedReference determines whether the given reference is to an importpath reference - // that Ko supports building. + // IsSupportedReference determines whether the given reference is to an + // importpath reference that Ko supports building, returning an error + // if it is not. // TODO(mattmoor): Verify that some base repo: foo.io/bar can be suffixed with this reference and parsed. - IsSupportedReference(string) bool + IsSupportedReference(string) error // Build turns the given importpath reference into a v1.Image containing the Go binary // (or a set of images as a v1.ImageIndex). diff --git a/pkg/build/gobuild.go b/pkg/build/gobuild.go index 7f6c33dce3..545a2a30b4 100644 --- a/pkg/build/gobuild.go +++ b/pkg/build/gobuild.go @@ -172,16 +172,19 @@ func NewGo(options ...Option) (Interface, error) { // // Only valid importpaths that provide commands (i.e., are "package main") are // supported. -func (g *gobuild) IsSupportedReference(s string) bool { +func (g *gobuild) IsSupportedReference(s string) error { ref := newRef(s) if !ref.IsStrict() { - return false + return errors.New("importpath does not start with ko://") } p, err := g.importPackage(ref) if err != nil { - return false + return err + } + if !p.IsCommand() { + return errors.New("importpath is not `package main`") } - return p.IsCommand() + return nil } // importPackage wraps go/build.Import to handle go modules. diff --git a/pkg/build/gobuild_test.go b/pkg/build/gobuild_test.go index 2eff3dc83d..5ccbc2faaa 100644 --- a/pkg/build/gobuild_test.go +++ b/pkg/build/gobuild_test.go @@ -49,8 +49,8 @@ func TestGoBuildIsSupportedRef(t *testing.T) { "ko://github.com/google/ko/cmd/ko", // ko can build itself. } { t.Run(importpath, func(t *testing.T) { - if !ng.IsSupportedReference(importpath) { - t.Errorf("IsSupportedReference(%q) = false, want true", importpath) + if err := ng.IsSupportedReference(importpath); err != nil { + t.Errorf("IsSupportedReference(%q) = (%v), want nil", importpath, err) } }) } @@ -61,8 +61,8 @@ func TestGoBuildIsSupportedRef(t *testing.T) { "ko://github.com/google/ko/pkg/nonexistent", // does not exist. } { t.Run(importpath, func(t *testing.T) { - if ng.IsSupportedReference(importpath) { - t.Errorf("IsSupportedReference(%v) = true, want false", importpath) + if err := ng.IsSupportedReference(importpath); err == nil { + t.Errorf("IsSupportedReference(%v) = nil, want error", importpath) } }) } @@ -110,8 +110,8 @@ func TestGoBuildIsSupportedRefWithModules(t *testing.T) { "ko://github.com/some/module/cmd", // ko can build commands in dependent modules } { t.Run(importpath, func(t *testing.T) { - if !ng.IsSupportedReference(importpath) { - t.Errorf("IsSupportedReference(%q) = false, want true", importpath) + if err := ng.IsSupportedReference(importpath); err != nil { + t.Errorf("IsSupportedReference(%q) = (%v), want nil", err, importpath) } }) } @@ -123,8 +123,8 @@ func TestGoBuildIsSupportedRefWithModules(t *testing.T) { "ko://github.com/google/ko/cmd/ko", // not in this module. } { t.Run(importpath, func(t *testing.T) { - if ng.IsSupportedReference(importpath) { - t.Errorf("IsSupportedReference(%v) = true, want false", importpath) + if err := ng.IsSupportedReference(importpath); err == nil { + t.Errorf("IsSupportedReference(%v) = nil, want error", importpath) } }) } diff --git a/pkg/build/limit.go b/pkg/build/limit.go index 432cf68074..8d67ec3ab5 100644 --- a/pkg/build/limit.go +++ b/pkg/build/limit.go @@ -30,7 +30,7 @@ type Limiter struct { var _ Interface = (*Recorder)(nil) // IsSupportedReference implements Interface -func (l *Limiter) IsSupportedReference(ip string) bool { +func (l *Limiter) IsSupportedReference(ip string) error { return l.Builder.IsSupportedReference(ip) } diff --git a/pkg/build/limit_test.go b/pkg/build/limit_test.go index 1ec41ee7d0..d17a6ecddc 100644 --- a/pkg/build/limit_test.go +++ b/pkg/build/limit_test.go @@ -27,8 +27,8 @@ type sleeper struct{} var _ Interface = (*sleeper)(nil) // IsSupportedReference implements Interface -func (r *sleeper) IsSupportedReference(ip string) bool { - return true +func (r *sleeper) IsSupportedReference(ip string) error { + return nil } // Build implements Interface diff --git a/pkg/build/recorder.go b/pkg/build/recorder.go index 1a82de3b8a..847a0712d8 100644 --- a/pkg/build/recorder.go +++ b/pkg/build/recorder.go @@ -30,7 +30,7 @@ type Recorder struct { var _ Interface = (*Recorder)(nil) // IsSupportedReference implements Interface -func (r *Recorder) IsSupportedReference(ip string) bool { +func (r *Recorder) IsSupportedReference(ip string) error { return r.Builder.IsSupportedReference(ip) } diff --git a/pkg/build/recorder_test.go b/pkg/build/recorder_test.go index 3ffcd841b3..b778ecc566 100644 --- a/pkg/build/recorder_test.go +++ b/pkg/build/recorder_test.go @@ -22,21 +22,17 @@ import ( ) type fake struct { - isr func(string) bool + isr func(string) error b func(string) (Result, error) } var _ Interface = (*fake)(nil) // IsSupportedReference implements Interface -func (r *fake) IsSupportedReference(ip string) bool { - return r.isr(ip) -} +func (r *fake) IsSupportedReference(ip string) error { return r.isr(ip) } // Build implements Interface -func (r *fake) Build(_ context.Context, ip string) (Result, error) { - return r.b(ip) -} +func (r *fake) Build(_ context.Context, ip string) (Result, error) { return r.b(ip) } func TestISRPassThrough(t *testing.T) { tests := []struct { @@ -53,12 +49,12 @@ func TestISRPassThrough(t *testing.T) { t.Run(test.name, func(t *testing.T) { called := false inner := &fake{ - isr: func(ip string) bool { + isr: func(ip string) error { called = true if ip != test.input { t.Errorf("ISR = %v, wanted %v", ip, test.input) } - return true + return nil }, } rec := &Recorder{ diff --git a/pkg/build/shared.go b/pkg/build/shared.go index c6e70af57c..6fab02023e 100644 --- a/pkg/build/shared.go +++ b/pkg/build/shared.go @@ -65,7 +65,7 @@ func (c *Caching) Build(ctx context.Context, ip string) (Result, error) { } // IsSupportedReference implements Interface -func (c *Caching) IsSupportedReference(ip string) bool { +func (c *Caching) IsSupportedReference(ip string) error { return c.inner.IsSupportedReference(ip) } diff --git a/pkg/build/shared_test.go b/pkg/build/shared_test.go index 5c9577b22e..d8e917dcd5 100644 --- a/pkg/build/shared_test.go +++ b/pkg/build/shared_test.go @@ -29,9 +29,7 @@ type slowbuild struct { // slowbuild implements Interface var _ Interface = (*slowbuild)(nil) -func (sb *slowbuild) IsSupportedReference(string) bool { - return true -} +func (sb *slowbuild) IsSupportedReference(string) error { return nil } func (sb *slowbuild) Build(context.Context, string) (Result, error) { time.Sleep(sb.sleep) @@ -45,8 +43,8 @@ func TestCaching(t *testing.T) { sb := &slowbuild{duration} cb, _ := NewCaching(sb) - if !cb.IsSupportedReference(ip) { - t.Errorf("ISR(%q) = false, wanted true", ip) + if err := cb.IsSupportedReference(ip); err != nil { + t.Errorf("ISR(%q) = (%v), wanted nil", err, ip) } previousDigest := "not-a-digest" diff --git a/pkg/commands/publisher.go b/pkg/commands/publisher.go index 9b611d9290..b40dadf560 100644 --- a/pkg/commands/publisher.go +++ b/pkg/commands/publisher.go @@ -55,8 +55,8 @@ func publishImages(ctx context.Context, importpaths []string, pub publish.Interf importpath = build.StrictScheme + importpath } - if !b.IsSupportedReference(importpath) { - return nil, fmt.Errorf("importpath %q is not supported", importpath) + if err := b.IsSupportedReference(importpath); err != nil { + return nil, fmt.Errorf("importpath %q is not supported: %v", importpath, err) } img, err := b.Build(ctx, importpath) diff --git a/pkg/internal/testing/fixed.go b/pkg/internal/testing/fixed.go index 0cad00d96c..054caee494 100644 --- a/pkg/internal/testing/fixed.go +++ b/pkg/internal/testing/fixed.go @@ -16,6 +16,7 @@ package testing import ( "context" + "errors" "fmt" "strings" @@ -36,10 +37,12 @@ func NewFixedBuild(entries map[string]build.Result) build.Interface { } // IsSupportedReference implements build.Interface -func (f *fixedBuild) IsSupportedReference(s string) bool { +func (f *fixedBuild) IsSupportedReference(s string) error { s = strings.TrimPrefix(s, build.StrictScheme) - _, ok := f.entries[s] - return ok + if _, ok := f.entries[s]; !ok { + return errors.New("importpath is not supported") + } + return nil } // Build implements build.Interface diff --git a/pkg/internal/testing/fixed_test.go b/pkg/internal/testing/fixed_test.go index 4a0b1df9d6..4c7556a368 100644 --- a/pkg/internal/testing/fixed_test.go +++ b/pkg/internal/testing/fixed_test.go @@ -67,8 +67,8 @@ func TestFixedBuild(t *testing.T) { "asdf": testImage, }) - if got, want := f.IsSupportedReference("asdf"), true; got != want { - t.Errorf("IsSupportedReference(asdf) = %v, want %v", got, want) + if got := f.IsSupportedReference("asdf"); got != nil { + t.Errorf("IsSupportedReference(asdf) = (%v), want nil", got) } if got, err := f.Build(context.Background(), "asdf"); err != nil { t.Errorf("Build(asdf) = %v, want %v", err, testImage) @@ -76,8 +76,8 @@ func TestFixedBuild(t *testing.T) { t.Errorf("Build(asdf) = %v, want %v", got, testImage) } - if got, want := f.IsSupportedReference("blah"), false; got != want { - t.Errorf("IsSupportedReference(blah) = %v, want %v", got, want) + if got := f.IsSupportedReference("blah"); got == nil { + t.Error("IsSupportedReference(blah) = nil, want error") } if got, err := f.Build(context.Background(), "blah"); err == nil { t.Errorf("Build(blah) = %v, want error", got) diff --git a/pkg/resolve/resolve.go b/pkg/resolve/resolve.go index 20c5e04ecc..d605c41684 100644 --- a/pkg/resolve/resolve.go +++ b/pkg/resolve/resolve.go @@ -41,7 +41,7 @@ func ImageReferences(ctx context.Context, docs []*yaml.Node, strict bool, builde for node, ok := it(); ok; node, ok = it() { ref := strings.TrimSpace(node.Value) - if builder.IsSupportedReference(ref) { + if err := builder.IsSupportedReference(ref); err == nil { refs[ref] = append(refs[ref], node) } else if strict { return fmt.Errorf("found strict reference but %s is not a valid import path", ref)