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

IsSupportedReference returns descriptive error #233

Merged
merged 1 commit into from
Oct 31, 2020
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: 4 additions & 3 deletions pkg/build/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
11 changes: 7 additions & 4 deletions pkg/build/gobuild.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
16 changes: 8 additions & 8 deletions pkg/build/gobuild_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
}
Expand All @@ -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)
}
})
}
Expand Down Expand Up @@ -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)
}
})
}
Expand All @@ -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)
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/build/limit.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/build/limit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/build/recorder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
14 changes: 5 additions & 9 deletions pkg/build/recorder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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{
Expand Down
2 changes: 1 addition & 1 deletion pkg/build/shared.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
8 changes: 3 additions & 5 deletions pkg/build/shared_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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"
Expand Down
4 changes: 2 additions & 2 deletions pkg/commands/publisher.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
9 changes: 6 additions & 3 deletions pkg/internal/testing/fixed.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package testing

import (
"context"
"errors"
"fmt"
"strings"

Expand All @@ -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
Expand Down
8 changes: 4 additions & 4 deletions pkg/internal/testing/fixed_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,17 +67,17 @@ 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)
} else if got != testImage {
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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/resolve/resolve.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down