Skip to content

Commit

Permalink
Remove strictness checks from build, into resolve
Browse files Browse the repository at this point in the history
Strictness has nothing to do with building, and is independent of how
images are built (fixed builder, some future exotic builder type, etc.)
  • Loading branch information
imjasonh committed Aug 15, 2019
1 parent 4342cef commit 3315663
Show file tree
Hide file tree
Showing 11 changed files with 42 additions and 75 deletions.
7 changes: 0 additions & 7 deletions pkg/build/gobuild.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ type gobuild struct {
build builder
disableOptimizations bool
mod *modInfo
strict bool
}

// Option is a functional option for NewGo.
Expand All @@ -61,7 +60,6 @@ type gobuildOpener struct {
build builder
disableOptimizations bool
mod *modInfo
strict bool
}

func (gbo *gobuildOpener) Open() (Interface, error) {
Expand All @@ -74,7 +72,6 @@ func (gbo *gobuildOpener) Open() (Interface, error) {
build: gbo.build,
disableOptimizations: gbo.disableOptimizations,
mod: gbo.mod,
strict: gbo.strict,
}, nil
}

Expand Down Expand Up @@ -122,10 +119,6 @@ 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 {
if g.strict && !strings.HasPrefix(s, "ko://") {
return false
}
s = strings.TrimPrefix(s, "ko://")
p, err := g.importPackage(s)
if err != nil {
return false
Expand Down
38 changes: 1 addition & 37 deletions pkg/build/gobuild_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ func TestGoBuildIsSupportedRef(t *testing.T) {

// Supported import paths.
for _, importpath := range []string{
filepath.FromSlash("github.com/google/ko/cmd/ko"), // ko can build itself.
filepath.FromSlash("ko://github.com/google/ko/cmd/ko"), // ko:// prefix is ignored in loose mode.
filepath.FromSlash("github.com/google/ko/cmd/ko"), // ko can build itself.
} {
t.Run(importpath, func(t *testing.T) {
if !ng.IsSupportedReference(importpath) {
Expand All @@ -62,41 +61,6 @@ func TestGoBuildIsSupportedRef(t *testing.T) {
}
}

func TestGoBuildIsSupportedReference_Strict(t *testing.T) {
base, err := random.Image(1024, 3)
if err != nil {
t.Fatalf("random.Image() = %v", err)
}
ng, err := NewGo(WithBaseImages(func(string) (v1.Image, error) { return base, nil }), WithStrict())
if err != nil {
t.Fatalf("NewGo() = %v", err)
}

// Supported import paths.
for _, importpath := range []string{
filepath.FromSlash("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)
}
})
}

// Unsupported import paths.
for _, importpath := range []string{
filepath.FromSlash("github.com/google/ko/cmd/ko"), // In strict mode, without ko://, it's not supported.
filepath.FromSlash("github.com/google/ko/pkg/build"), // not a command.
filepath.FromSlash("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)
}
})
}
}

func TestGoBuildIsSupportedRefWithModules(t *testing.T) {
base, err := random.Image(1024, 3)
if err != nil {
Expand Down
9 changes: 0 additions & 9 deletions pkg/build/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,3 @@ func withModuleInfo(mi *modInfo) Option {
return nil
}
}

// WithStrict is a functional option for requiring that package references are
// explicitly noted.
func WithStrict() Option {
return func(g *gobuildOpener) error {
g.strict = true
return nil
}
}
2 changes: 1 addition & 1 deletion pkg/commands/apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func addApply(topLevel *cobra.Command) {
cat config.yaml | ko apply -f -`,
Args: cobra.NoArgs,
Run: func(cmd *cobra.Command, args []string) {
builder, err := makeBuilder(do, sto)
builder, err := makeBuilder(do)
if err != nil {
log.Fatalf("error creating builder: %v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/commands/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func addCreate(topLevel *cobra.Command) {
cat config.yaml | ko create -f -`,
Args: cobra.NoArgs,
Run: func(cmd *cobra.Command, args []string) {
builder, err := makeBuilder(do, sto)
builder, err := makeBuilder(do)
if err != nil {
log.Fatalf("error creating builder: %v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/commands/publish.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func addPublish(topLevel *cobra.Command) {
ko publish --local github.com/foo/bar/cmd/baz github.com/foo/bar/cmd/blah`,
Args: cobra.MinimumNArgs(1),
Run: func(_ *cobra.Command, args []string) {
builder, err := makeBuilder(do, &options.StrictOptions{})
builder, err := makeBuilder(do)
if err != nil {
log.Fatalf("error creating builder: %v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/commands/resolve.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func addResolve(topLevel *cobra.Command) {
ko resolve --local -f config/`,
Args: cobra.NoArgs,
Run: func(cmd *cobra.Command, args []string) {
builder, err := makeBuilder(do, sto)
builder, err := makeBuilder(do)
if err != nil {
log.Fatalf("error creating builder: %v", err)
}
Expand Down
9 changes: 3 additions & 6 deletions pkg/commands/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import (
"github.com/mattmoor/dep-notify/pkg/graph"
)

func gobuildOptions(do *options.DebugOptions, so *options.StrictOptions) ([]build.Option, error) {
func gobuildOptions(do *options.DebugOptions) ([]build.Option, error) {
creationTime, err := getCreationTime()
if err != nil {
return nil, err
Expand All @@ -46,14 +46,11 @@ func gobuildOptions(do *options.DebugOptions, so *options.StrictOptions) ([]buil
if do.DisableOptimizations {
opts = append(opts, build.WithDisabledOptimizations())
}
if so.Strict {
opts = append(opts, build.WithStrict())
}
return opts, nil
}

func makeBuilder(do *options.DebugOptions, so *options.StrictOptions) (*build.Caching, error) {
opt, err := gobuildOptions(do, so)
func makeBuilder(do *options.DebugOptions) (*build.Caching, error) {
opt, err := gobuildOptions(do)
if err != nil {
log.Fatalf("error setting up builder options: %v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/commands/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func addRun(topLevel *cobra.Command) {
# This supports relative import paths as well.
ko run foo --image=./cmd/baz`,
Run: func(cmd *cobra.Command, args []string) {
builder, err := makeBuilder(do, &options.StrictOptions{})
builder, err := makeBuilder(do)
if err != nil {
log.Fatalf("error creating builder: %v", err)
}
Expand Down
9 changes: 6 additions & 3 deletions pkg/resolve/resolve.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,12 @@ func ImageReferences(input []byte, strict bool, builder build.Interface, publish
}
// This simply returns the replaced object, which we discard during the gathering phase.
if _, err := replaceRecursive(obj, func(ref string) (string, error) {
if builder.IsSupportedReference(ref) {
ref = strings.TrimPrefix(ref, "ko://")
refs[ref] = struct{}{}
if strict && !strings.HasPrefix(ref, "ko://") {
return ref, nil
}
tref := strings.TrimPrefix(ref, "ko://")
if builder.IsSupportedReference(tref) {
refs[tref] = struct{}{}
} else if strict && strings.HasPrefix(ref, "ko://") {
return "", fmt.Errorf("Strict reference %q is not supported", ref)
}
Expand Down
35 changes: 27 additions & 8 deletions pkg/resolve/resolve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func TestYAMLArrays(t *testing.T) {
t.Fatalf("yaml.Marshal(%v) = %v", inputStructured, err)
}

outYAML, err := ImageReferences(inputYAML, testBuilder, newFixedPublish(test.base, testHashes))
outYAML, err := ImageReferences(inputYAML, false, testBuilder, newFixedPublish(test.base, testHashes))
if err != nil {
t.Fatalf("ImageReferences(%v) = %v", string(inputYAML), err)
}
Expand Down Expand Up @@ -158,7 +158,7 @@ func TestYAMLMaps(t *testing.T) {
t.Fatalf("yaml.Marshal(%v) = %v", inputStructured, err)
}

outYAML, err := ImageReferences(inputYAML, testBuilder, newFixedPublish(base, testHashes))
outYAML, err := ImageReferences(inputYAML, false, testBuilder, newFixedPublish(base, testHashes))
if err != nil {
t.Fatalf("ImageReferences(%v) = %v", string(inputYAML), err)
}
Expand Down Expand Up @@ -226,7 +226,7 @@ func TestYAMLObject(t *testing.T) {
t.Fatalf("yaml.Marshal(%v) = %v", inputStructured, err)
}

outYAML, err := ImageReferences(inputYAML, testBuilder, newFixedPublish(base, testHashes))
outYAML, err := ImageReferences(inputYAML, false, testBuilder, newFixedPublish(base, testHashes))
if err != nil {
t.Fatalf("ImageReferences(%v) = %v", string(inputYAML), err)
}
Expand All @@ -242,8 +242,29 @@ func TestYAMLObject(t *testing.T) {
}
}

func TestStrict(t *testing.T) {
refs := []string{
"ko://" + fooRef,
"ko://" + barRef,
}
buf := bytes.NewBuffer(nil)
encoder := yaml.NewEncoder(buf)
for _, input := range refs {
if err := encoder.Encode(input); err != nil {
t.Fatalf("Encode(%v) = %v", input, err)
}
}
inputYAML := buf.Bytes()
base := mustRepository("gcr.io/multi-pass")
outYAML, err := ImageReferences(inputYAML, true, testBuilder, newFixedPublish(base, testHashes))
if err != nil {
t.Fatalf("ImageReferences: %v", err)
}
t.Log(string(outYAML))
}

func TestMultiDocumentYAMLs(t *testing.T) {
tests := []struct {
for _, test := range []struct {
desc string
refs []string
hashes []v1.Hash
Expand All @@ -253,9 +274,7 @@ func TestMultiDocumentYAMLs(t *testing.T) {
refs: []string{fooRef, barRef},
hashes: []v1.Hash{fooHash, barHash},
base: mustRepository("gcr.io/multi-pass"),
}}

for _, test := range tests {
}} {
t.Run(test.desc, func(t *testing.T) {
buf := bytes.NewBuffer(nil)
encoder := yaml.NewEncoder(buf)
Expand All @@ -266,7 +285,7 @@ func TestMultiDocumentYAMLs(t *testing.T) {
}
inputYAML := buf.Bytes()

outYAML, err := ImageReferences(inputYAML, testBuilder, newFixedPublish(test.base, testHashes))
outYAML, err := ImageReferences(inputYAML, false, testBuilder, newFixedPublish(test.base, testHashes))
if err != nil {
t.Fatalf("ImageReferences(%v) = %v", string(inputYAML), err)
}
Expand Down

0 comments on commit 3315663

Please sign in to comment.