From e43e46701873fd7b399d146d757560a28a6696dd Mon Sep 17 00:00:00 2001 From: David Gageot Date: Tue, 26 May 2020 11:29:46 +0200 Subject: [PATCH 1/4] Support project.toml Signed-off-by: David Gageot --- examples/buildpacks-node/project.toml | 3 + go.mod | 1 + .../examples/buildpacks-node/project.toml | 3 + pkg/skaffold/build/buildpacks/build_test.go | 80 ++++++++++++++++++- pkg/skaffold/build/buildpacks/lifecycle.go | 43 ++++++++-- pkg/skaffold/build/buildpacks/project.go | 80 +++++++++++++++++++ vendor/modules.txt | 1 + 7 files changed, 204 insertions(+), 7 deletions(-) create mode 100644 examples/buildpacks-node/project.toml create mode 100644 integration/examples/buildpacks-node/project.toml create mode 100644 pkg/skaffold/build/buildpacks/project.go diff --git a/examples/buildpacks-node/project.toml b/examples/buildpacks-node/project.toml new file mode 100644 index 00000000000..8b9b94983f8 --- /dev/null +++ b/examples/buildpacks-node/project.toml @@ -0,0 +1,3 @@ +[[build.env]] +name = "GOOGLE_RUNTIME_VERSION" +value = "14.3.0" diff --git a/go.mod b/go.mod index 98b680b0be2..28df3a3afa3 100644 --- a/go.mod +++ b/go.mod @@ -21,6 +21,7 @@ require ( contrib.go.opencensus.io/exporter/ocagent v0.6.0 // indirect contrib.go.opencensus.io/exporter/prometheus v0.1.0 // indirect contrib.go.opencensus.io/exporter/stackdriver v0.13.1 // indirect + github.com/BurntSushi/toml v0.3.1 github.com/blang/semver v3.5.1+incompatible github.com/bmatcuk/doublestar v1.2.4 github.com/buildpacks/lifecycle v0.7.1 diff --git a/integration/examples/buildpacks-node/project.toml b/integration/examples/buildpacks-node/project.toml new file mode 100644 index 00000000000..8b9b94983f8 --- /dev/null +++ b/integration/examples/buildpacks-node/project.toml @@ -0,0 +1,3 @@ +[[build.env]] +name = "GOOGLE_RUNTIME_VERSION" +value = "14.3.0" diff --git a/pkg/skaffold/build/buildpacks/build_test.go b/pkg/skaffold/build/buildpacks/build_test.go index 0ba0d0e5870..0c1d1627ece 100644 --- a/pkg/skaffold/build/buildpacks/build_test.go +++ b/pkg/skaffold/build/buildpacks/build_test.go @@ -44,6 +44,7 @@ func TestBuild(t *testing.T) { artifact *latest.Artifact tag string api *testutil.FakeAPIClient + files map[string]string pushImages bool devMode bool shouldErr bool @@ -76,6 +77,73 @@ func TestBuild(t *testing.T) { Image: "img:latest", }, }, + { + description: "project.toml", + artifact: buildpacksArtifact("my/builder2", "my/run2"), + tag: "img:tag", + api: &testutil.FakeAPIClient{}, + files: map[string]string{ + "project.toml": `[[build.env]] +name = "GOOGLE_RUNTIME_VERSION" +value = "14.3.0" +[[build.buildpacks]] +id = "my/buildpack" +[[build.buildpacks]] +id = "my/otherBuildpack" +`, + }, + expectedOptions: &pack.BuildOptions{ + AppPath: ".", + Builder: "my/builder2", + RunImage: "my/run2", + Buildpacks: []string{"my/buildpack", "my/otherBuildpack"}, + Env: map[string]string{ + "GOOGLE_RUNTIME_VERSION": "14.3.0", + }, + Image: "img:latest", + }, + }, + { + description: "Buildpacks in skaffold.yaml override those in project.toml", + artifact: withBuildpacks([]string{"my/buildpack", "my/otherBuildpack"}, buildpacksArtifact("my/builder3", "my/run3")), + tag: "img:tag", + api: &testutil.FakeAPIClient{}, + files: map[string]string{ + "project.toml": `[[build.buildpacks]] +id = "my/ignored" +`, + }, + expectedOptions: &pack.BuildOptions{ + AppPath: ".", + Builder: "my/builder3", + RunImage: "my/run3", + Buildpacks: []string{"my/buildpack", "my/otherBuildpack"}, + Env: map[string]string{}, + Image: "img:latest", + }, + }, + { + description: "Combine env from skaffold.yaml and project.toml", + artifact: withEnv([]string{"KEY1=VALUE1"}, buildpacksArtifact("my/builder4", "my/run4")), + tag: "img:tag", + api: &testutil.FakeAPIClient{}, + files: map[string]string{ + "project.toml": `[[build.env]] +name = "KEY2" +value = "VALUE2" +`, + }, + expectedOptions: &pack.BuildOptions{ + AppPath: ".", + Builder: "my/builder4", + RunImage: "my/run4", + Env: map[string]string{ + "KEY1": "VALUE1", + "KEY2": "VALUE2", + }, + Image: "img:latest", + }, + }, { description: "dev mode", artifact: withSync(&latest.Sync{Auto: &latest.Auto{}}, buildpacksArtifact("another/builder", "another/run")), @@ -130,10 +198,20 @@ func TestBuild(t *testing.T) { api: &testutil.FakeAPIClient{}, shouldErr: true, }, + { + description: "invalid project.toml", + artifact: buildpacksArtifact("my/builder2", "my/run2"), + tag: "img:tag", + api: &testutil.FakeAPIClient{}, + files: map[string]string{ + "project.toml": `INVALID`, + }, + shouldErr: true, + }, } for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - t.NewTempDir().Touch("file").Chdir() + t.NewTempDir().Touch("file").WriteFiles(test.files).Chdir() pack := &fakePack{} t.Override(&runPackBuildFunc, pack.runPack) diff --git a/pkg/skaffold/build/buildpacks/lifecycle.go b/pkg/skaffold/build/buildpacks/lifecycle.go index ea2eb27f22f..cacae05e3f7 100644 --- a/pkg/skaffold/build/buildpacks/lifecycle.go +++ b/pkg/skaffold/build/buildpacks/lifecycle.go @@ -21,18 +21,21 @@ import ( "errors" "fmt" "io" + "os" + "path/filepath" "strconv" "strings" lifecycle "github.com/buildpacks/lifecycle/cmd" "github.com/buildpacks/pack" - "github.com/sirupsen/logrus" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/misc" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" ) +const projectTOML = "project.toml" + // For testing var ( runPackBuildFunc = runPackBuild @@ -47,6 +50,18 @@ func (b *Builder) build(ctx context.Context, out io.Writer, a *latest.Artifact, artifact := a.BuildpackArtifact workspace := a.Workspace + // Read `project.toml` if it exists. + var projectDescriptor Descriptor + path := filepath.Join(a.Workspace, projectTOML) + if _, err := os.Stat(path); err == nil { + desc, err := ReadProjectDescriptor(path) + if err != nil { + return "", fmt.Errorf("failed to read project descriptor %q: %w", path, err) + } + + projectDescriptor = desc + } + // To improve caching, we always build the image with [:latest] tag // This way, the lifecycle is able to "bootstrap" from the previously built image. // The image will then be tagged as usual with the tag provided by the tag policy. @@ -56,24 +71,40 @@ func (b *Builder) build(ctx context.Context, out io.Writer, a *latest.Artifact, } latest := parsed.BaseName + ":latest" - logrus.Debugln("Evaluate env variables") - env, err := misc.EvaluateEnv(artifact.Env) + // Eveluate Env Vars. + envVars, err := misc.EvaluateEnv(artifact.Env) if err != nil { return "", fmt.Errorf("unable to evaluate env variables: %w", err) } if b.devMode && a.Sync != nil && a.Sync.Auto != nil { - env = append(env, "GOOGLE_DEVMODE=1") + envVars = append(envVars, "GOOGLE_DEVMODE=1") + } + + env := envMap(envVars) + for _, kv := range projectDescriptor.Build.Env { + env[kv.Name] = kv.Value + } + + // List buildpacks to be used for the build. + // Those specified in the skaffold.yaml override those in the project.toml. + buildpacks := artifact.Buildpacks + if len(buildpacks) == 0 { + for _, buildpack := range projectDescriptor.Build.Buildpacks { + // TODO(dgageot): Support version and URI. + buildpacks = append(buildpacks, buildpack.ID) + } } + // Does the builder image need to be pulled? alreadyPulled := images.AreAlreadyPulled(artifact.Builder, artifact.RunImage) if err := runPackBuildFunc(ctx, out, b.localDocker, pack.BuildOptions{ AppPath: workspace, Builder: artifact.Builder, RunImage: artifact.RunImage, - Buildpacks: artifact.Buildpacks, - Env: envMap(env), + Buildpacks: buildpacks, + Env: env, Image: latest, NoPull: alreadyPulled, }); err != nil { diff --git a/pkg/skaffold/build/buildpacks/project.go b/pkg/skaffold/build/buildpacks/project.go new file mode 100644 index 00000000000..3c6338c3587 --- /dev/null +++ b/pkg/skaffold/build/buildpacks/project.go @@ -0,0 +1,80 @@ +/* +Copyright 2019 The Skaffold Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// This whole file is copy/pasted from +// https://github.com/buildpacks/pack/blob/master/internal/project/project.go +package buildpacks + +import ( + "io/ioutil" + + "github.com/BurntSushi/toml" + "github.com/pkg/errors" +) + +type Buildpack struct { + ID string `toml:"id"` + Version string `toml:"version"` + URI string `toml:"uri"` +} + +type EnvVar struct { + Name string `toml:"name"` + Value string `toml:"value"` +} + +type Build struct { + Include []string `toml:"include"` + Exclude []string `toml:"exclude"` + Buildpacks []Buildpack `toml:"buildpacks"` + Env []EnvVar `toml:"env"` +} + +type Descriptor struct { + Build Build `toml:"build"` +} + +func ReadProjectDescriptor(pathToFile string) (Descriptor, error) { + projectTomlContents, err := ioutil.ReadFile(pathToFile) + if err != nil { + return Descriptor{}, err + } + + var descriptor Descriptor + _, err = toml.Decode(string(projectTomlContents), &descriptor) + if err != nil { + return Descriptor{}, err + } + + return descriptor, descriptor.validate() +} + +func (p Descriptor) validate() error { + if p.Build.Exclude != nil && p.Build.Include != nil { + return errors.New("project.toml: cannot have both include and exclude defined") + } + + for _, bp := range p.Build.Buildpacks { + if bp.ID == "" && bp.URI == "" { + return errors.New("project.toml: buildpacks must have an id or url defined") + } + if bp.URI != "" && bp.Version != "" { + return errors.New("project.toml: buildpacks cannot have both uri and version defined") + } + } + + return nil +} diff --git a/vendor/modules.txt b/vendor/modules.txt index c2e14d5107a..ea2a16b8598 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -35,6 +35,7 @@ github.com/Azure/go-autorest/logger # github.com/Azure/go-autorest/tracing v0.5.0 github.com/Azure/go-autorest/tracing # github.com/BurntSushi/toml v0.3.1 +## explicit github.com/BurntSushi/toml # github.com/MakeNowJust/heredoc v0.0.0-20170808103936-bb23615498cd github.com/MakeNowJust/heredoc From 41d5887bccd5a293d62ab57af1fdc32443c469a5 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Tue, 26 May 2020 16:35:15 +0200 Subject: [PATCH 2/4] `skaffold init` with `project.toml` Signed-off-by: David Gageot --- pkg/skaffold/build/buildpacks/init.go | 12 +++++------- pkg/skaffold/build/buildpacks/init_test.go | 7 +++++++ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/pkg/skaffold/build/buildpacks/init.go b/pkg/skaffold/build/buildpacks/init.go index 9511778c195..d45d27fe30e 100644 --- a/pkg/skaffold/build/buildpacks/init.go +++ b/pkg/skaffold/build/buildpacks/init.go @@ -71,14 +71,12 @@ func (c ArtifactConfig) Path() string { // validate checks if a file is a valid Buildpack configuration. func validate(path string) bool { - switch filepath.Base(path) { - case "package.json": - return !hasParent(path, "node_modules") - case "go.mod": - return !hasParent(path, "vendor") - default: - return false + file := filepath.Base(path) + if file == "package.json" || file == "go.mod" || file == "project.toml" { + return !hasParent(path, "node_modules") && !hasParent(path, "vendor") } + + return false } func hasParent(path, parent string) bool { diff --git a/pkg/skaffold/build/buildpacks/init_test.go b/pkg/skaffold/build/buildpacks/init_test.go index 3050b9d4eaa..ae36a06e30c 100644 --- a/pkg/skaffold/build/buildpacks/init_test.go +++ b/pkg/skaffold/build/buildpacks/init_test.go @@ -50,6 +50,11 @@ func TestValidate(t *testing.T) { path: filepath.Join("go.mod"), expectedValid: true, }, + { + description: "Buildpacks", + path: filepath.Join("project.toml"), + expectedValid: true, + }, { description: "Unknown language", path: filepath.Join("path", "to", "something.txt"), @@ -73,6 +78,8 @@ func TestValidateIgnored(t *testing.T) { filepath.Join("node_modules", "package.json"), filepath.Join("vendor", "go.mod"), filepath.Join("parent", "vendor", "go.mod"), + filepath.Join("parent", "vendor", "project.toml"), + filepath.Join("node_modules", "project.toml"), } for _, path := range paths { From 3604f0c79de2f8e827483511405643e09f8d4093 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Tue, 26 May 2020 16:39:46 +0200 Subject: [PATCH 3/4] Add a few TODOs Signed-off-by: David Gageot --- pkg/skaffold/build/buildpacks/dependencies.go | 1 + pkg/skaffold/build/buildpacks/lifecycle.go | 2 ++ 2 files changed, 3 insertions(+) diff --git a/pkg/skaffold/build/buildpacks/dependencies.go b/pkg/skaffold/build/buildpacks/dependencies.go index 0ba98be14a9..512bdb17a37 100644 --- a/pkg/skaffold/build/buildpacks/dependencies.go +++ b/pkg/skaffold/build/buildpacks/dependencies.go @@ -25,5 +25,6 @@ import ( // GetDependencies returns dependencies listed for a buildpack artifact func GetDependencies(ctx context.Context, workspace string, a *latest.BuildpackArtifact) ([]string, error) { + // TODO(dgageot): Support project.toml include/exclude. return list.Files(workspace, a.Dependencies.Paths, a.Dependencies.Ignore) } diff --git a/pkg/skaffold/build/buildpacks/lifecycle.go b/pkg/skaffold/build/buildpacks/lifecycle.go index cacae05e3f7..2f4ee2ccc4d 100644 --- a/pkg/skaffold/build/buildpacks/lifecycle.go +++ b/pkg/skaffold/build/buildpacks/lifecycle.go @@ -107,6 +107,8 @@ func (b *Builder) build(ctx context.Context, out io.Writer, a *latest.Artifact, Env: env, Image: latest, NoPull: alreadyPulled, + // TODO(dgageot): Support project.toml include/exclude. + // FileFilter: func(string) bool { return true }, }); err != nil { return "", err } From 47aace7498529d94509bd7e8abca2c612297af70 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 27 May 2020 12:19:07 +0200 Subject: [PATCH 4/4] Feedback Signed-off-by: David Gageot --- pkg/skaffold/build/buildpacks/lifecycle.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/pkg/skaffold/build/buildpacks/lifecycle.go b/pkg/skaffold/build/buildpacks/lifecycle.go index 2f4ee2ccc4d..f57e778fde9 100644 --- a/pkg/skaffold/build/buildpacks/lifecycle.go +++ b/pkg/skaffold/build/buildpacks/lifecycle.go @@ -51,15 +51,10 @@ func (b *Builder) build(ctx context.Context, out io.Writer, a *latest.Artifact, workspace := a.Workspace // Read `project.toml` if it exists. - var projectDescriptor Descriptor path := filepath.Join(a.Workspace, projectTOML) - if _, err := os.Stat(path); err == nil { - desc, err := ReadProjectDescriptor(path) - if err != nil { - return "", fmt.Errorf("failed to read project descriptor %q: %w", path, err) - } - - projectDescriptor = desc + projectDescriptor, err := ReadProjectDescriptor(path) + if err != nil && !os.IsNotExist(err) { + return "", fmt.Errorf("failed to read project descriptor %q: %w", path, err) } // To improve caching, we always build the image with [:latest] tag @@ -87,7 +82,7 @@ func (b *Builder) build(ctx context.Context, out io.Writer, a *latest.Artifact, } // List buildpacks to be used for the build. - // Those specified in the skaffold.yaml override those in the project.toml. + // Those specified in the skaffold.yaml replace those in the project.toml. buildpacks := artifact.Buildpacks if len(buildpacks) == 0 { for _, buildpack := range projectDescriptor.Build.Buildpacks {