From ec97f6cf1ae6ed78c4fd88440fa64965bafe269a Mon Sep 17 00:00:00 2001 From: Nick Kubala Date: Mon, 24 Feb 2020 16:09:25 -0800 Subject: [PATCH] defer manifest generation to build initializer --- integration/init_test.go | 12 ++++----- pkg/skaffold/initializer/build/builders.go | 15 +++-------- .../initializer/build/builders_test.go | 11 ++++---- pkg/skaffold/initializer/build/cli.go | 6 ++--- pkg/skaffold/initializer/build/init.go | 18 ++++++++----- pkg/skaffold/initializer/build/resolve.go | 4 +++ pkg/skaffold/initializer/config_test.go | 10 +++---- pkg/skaffold/initializer/deploy/init.go | 19 +++++--------- pkg/skaffold/initializer/deploy/kubectl.go | 26 +++---------------- pkg/skaffold/initializer/init.go | 9 +++++-- pkg/skaffold/initializer/prompt/prompt.go | 4 +-- 11 files changed, 57 insertions(+), 77 deletions(-) diff --git a/integration/init_test.go b/integration/init_test.go index a61fb74fff1..1c292fa9f45 100644 --- a/integration/init_test.go +++ b/integration/init_test.go @@ -75,12 +75,12 @@ func TestInitManifestGeneration(t *testing.T) { expectedManifestPaths: []string{"deployment.yaml"}, }, // TODO(nkubala): add this back when the --force flag is fixed - // { - // name: "microservices", - // dir: "testdata/init/microservices", - // args: []string{"--XXenableManifestGeneration"}, - // expectedManifestPaths: []string{"leeroy-web/deployment.yaml", "leeroy-app/deployment.yaml"}, - // }, + { + name: "microservices", + dir: "testdata/init/microservices", + args: []string{"--XXenableManifestGeneration"}, + expectedManifestPaths: []string{"leeroy-web/deployment.yaml", "leeroy-app/deployment.yaml"}, + }, } for _, test := range tests { diff --git a/pkg/skaffold/initializer/build/builders.go b/pkg/skaffold/initializer/build/builders.go index b7e40eb39f1..1ea6ee3465a 100644 --- a/pkg/skaffold/initializer/build/builders.go +++ b/pkg/skaffold/initializer/build/builders.go @@ -72,13 +72,8 @@ type Initializer interface { BuilderImagePairs() []BuilderImagePair // PrintAnalysis writes the project analysis to the provided out stream PrintAnalysis(io.Writer) error - // GeneratedPairs returns all builder/image pairs with images generated by skaffold. - // Each of these pairs contains a path at which a k8s manifest should be generated. - GeneratedPairs() []GeneratedBuilderImagePair - // Resolve should be called by the init controller once all generated pairs have - // been handled, to signal to the build initializer that all - // builder/image pairs have been resolved. - Resolve() + // GenerateManifests generates image names and manifests for all unresolved pairs + GenerateManifests() (map[GeneratedBuilderImagePair][]byte, error) } type emptyBuildInitializer struct { @@ -100,12 +95,10 @@ func (e *emptyBuildInitializer) PrintAnalysis(io.Writer) error { return nil } -func (e *emptyBuildInitializer) GeneratedPairs() []GeneratedBuilderImagePair { - return nil +func (e *emptyBuildInitializer) GenerateManifests() (map[GeneratedBuilderImagePair][]byte, error) { + return nil, nil } -func (e *emptyBuildInitializer) Resolve() {} - func NewInitializer(builders []InitBuilder, c config.Config) Initializer { switch { case c.SkipBuild: diff --git a/pkg/skaffold/initializer/build/builders_test.go b/pkg/skaffold/initializer/build/builders_test.go index 984bf2c65a9..d2948a8fd06 100644 --- a/pkg/skaffold/initializer/build/builders_test.go +++ b/pkg/skaffold/initializer/build/builders_test.go @@ -19,12 +19,13 @@ package build import ( "testing" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/initializer/prompt" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/warnings" + "github.com/google/go-cmp/cmp" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/buildpacks" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/jib" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/initializer/prompt" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/warnings" "github.com/GoogleContainerTools/skaffold/testutil" ) @@ -79,7 +80,7 @@ func TestResolveBuilderImages(t *testing.T) { Builder: jib.ArtifactConfig{BuilderName: "Jib Maven Plugin", File: "pom.xml", Project: "project"}, ImageName: "pom.xml-image", }, - ManifestPath: ".", + ManifestPath: "deployment.yaml", }, }, }, @@ -114,7 +115,7 @@ func TestResolveBuilderImages(t *testing.T) { Builder: docker.ArtifactConfig{File: "foo"}, ImageName: "foo-image", }, - ManifestPath: ".", + ManifestPath: "deployment.yaml", }, }, shouldMakeChoice: false, @@ -139,7 +140,7 @@ func TestResolveBuilderImages(t *testing.T) { } err := initializer.resolveBuilderImages() t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expectedPairs, initializer.BuilderImagePairs()) - t.CheckDeepEqual(test.expectedGeneratedPairs, initializer.GeneratedPairs()) + t.CheckDeepEqual(test.expectedGeneratedPairs, initializer.generatedBuilderImagePairs, cmp.AllowUnexported()) }) } } diff --git a/pkg/skaffold/initializer/build/cli.go b/pkg/skaffold/initializer/build/cli.go index 098abacd2e8..021b1baa9cc 100644 --- a/pkg/skaffold/initializer/build/cli.go +++ b/pkg/skaffold/initializer/build/cli.go @@ -62,12 +62,10 @@ func (c *cliBuildInitializer) PrintAnalysis(out io.Writer) error { return printAnalysis(out, c.enableNewFormat, c.skipBuild, c.builderImagePairs, c.builders, nil) } -func (c *cliBuildInitializer) GeneratedPairs() []GeneratedBuilderImagePair { - return nil +func (c *cliBuildInitializer) GenerateManifests() (map[GeneratedBuilderImagePair][]byte, error) { + return nil, nil } -func (c *cliBuildInitializer) Resolve() {} - func (c *cliBuildInitializer) processCliArtifacts() error { pairs, err := processCliArtifacts(c.cliArtifacts) if err != nil { diff --git a/pkg/skaffold/initializer/build/init.go b/pkg/skaffold/initializer/build/init.go index 5dc559e2382..51cd4361b04 100644 --- a/pkg/skaffold/initializer/build/init.go +++ b/pkg/skaffold/initializer/build/init.go @@ -19,6 +19,9 @@ package build import ( "io" + "github.com/pkg/errors" + + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/generator" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" ) @@ -63,14 +66,17 @@ func (d *defaultBuildInitializer) PrintAnalysis(out io.Writer) error { return printAnalysis(out, d.enableNewFormat, d.skipBuild, d.builderImagePairs, d.builders, d.unresolvedImages) } -func (d *defaultBuildInitializer) GeneratedPairs() []GeneratedBuilderImagePair { - return d.generatedBuilderImagePairs -} - -func (d *defaultBuildInitializer) Resolve() { +func (d *defaultBuildInitializer) GenerateManifests() (map[GeneratedBuilderImagePair][]byte, error) { + generatedManifests := map[GeneratedBuilderImagePair][]byte{} for _, pair := range d.generatedBuilderImagePairs { - d.builderImagePairs = append(d.builderImagePairs, pair.BuilderImagePair) + manifest, err := generator.Generate(pair.ImageName) + if err != nil { + return nil, errors.Wrap(err, "generating kubernetes manifest") + } + generatedManifests[pair] = manifest } + d.generatedBuilderImagePairs = nil + return generatedManifests, nil } // matchBuildersToImages takes a list of builders and images, checks if any of the builders' configured target diff --git a/pkg/skaffold/initializer/build/resolve.go b/pkg/skaffold/initializer/build/resolve.go index 66d2a6bba30..4849f2210fd 100644 --- a/pkg/skaffold/initializer/build/resolve.go +++ b/pkg/skaffold/initializer/build/resolve.go @@ -18,6 +18,7 @@ package build import ( "fmt" + "os" "path/filepath" "sort" "strings" @@ -109,11 +110,14 @@ func getGeneratedBuilderPair(b InitBuilder) GeneratedBuilderImagePair { imageName = fmt.Sprintf("%s-image", strings.ToLower(path)) path = "." } + path = filepath.Join(path, "deployment.yaml") + fmt.Fprintf(os.Stdout, "imageName: %s, path: %s\n", imageName, path) return GeneratedBuilderImagePair{ BuilderImagePair: BuilderImagePair{ Builder: b, ImageName: imageName, }, + // ManifestPath: filepath.Join(path, "deployment.yaml"), ManifestPath: path, } } diff --git a/pkg/skaffold/initializer/config_test.go b/pkg/skaffold/initializer/config_test.go index bd614e760c0..8b060cd349f 100644 --- a/pkg/skaffold/initializer/config_test.go +++ b/pkg/skaffold/initializer/config_test.go @@ -45,7 +45,7 @@ func (s stubDeploymentInitializer) Validate() error { panic("no thanks") } -func (s stubDeploymentInitializer) GenerateManifests([]build.GeneratedBuilderImagePair) (map[string][]byte, error) { +func (s stubDeploymentInitializer) AddManifestForImage(string, string) { panic("don't call me") } @@ -71,12 +71,8 @@ func (s stubBuildInitializer) BuildConfig() latest.BuildConfig { } } -func (s stubBuildInitializer) GeneratedPairs() []build.GeneratedBuilderImagePair { - panic("do not call me") -} - -func (s stubBuildInitializer) Resolve() { - panic("please don't") +func (s stubBuildInitializer) GenerateManifests() (map[build.GeneratedBuilderImagePair][]byte, error) { + panic("no thank you") } func TestGenerateSkaffoldConfig(t *testing.T) { diff --git a/pkg/skaffold/initializer/deploy/init.go b/pkg/skaffold/initializer/deploy/init.go index 79853432637..289d84c5aaa 100644 --- a/pkg/skaffold/initializer/deploy/init.go +++ b/pkg/skaffold/initializer/deploy/init.go @@ -17,7 +17,6 @@ limitations under the License. package deploy import ( - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/initializer/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/initializer/config" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" ) @@ -34,10 +33,10 @@ type Initializer interface { DeployConfig() latest.DeployConfig // GetImages fetches all the images defined in the manifest files. GetImages() []string - // GenerateManifests generates k8s manifests for each unresolved builder - GenerateManifests([]build.GeneratedBuilderImagePair) (map[string][]byte, error) // Validate ensures preconditions are met before generating a skaffold config Validate() error + // AddManifestForImage adds a provided manifest for a given image to the initializer + AddManifestForImage(string, string) } type cliDeployInit struct { @@ -64,28 +63,24 @@ func (c *cliDeployInit) Validate() error { return nil } -func (c *cliDeployInit) GenerateManifests([]build.GeneratedBuilderImagePair) (map[string][]byte, error) { - return nil, nil -} +func (c *cliDeployInit) AddManifestForImage(string, string) {} type emptyDeployInit struct { } -func (c *emptyDeployInit) DeployConfig() latest.DeployConfig { +func (e *emptyDeployInit) DeployConfig() latest.DeployConfig { return latest.DeployConfig{} } -func (c *emptyDeployInit) GetImages() []string { +func (e *emptyDeployInit) GetImages() []string { return nil } -func (c *emptyDeployInit) Validate() error { +func (e *emptyDeployInit) Validate() error { return nil } -func (c *emptyDeployInit) GenerateManifests([]build.GeneratedBuilderImagePair) (map[string][]byte, error) { - return nil, nil -} +func (e *emptyDeployInit) AddManifestForImage(string, string) {} func NewInitializer(manifests []string, c config.Config) Initializer { switch { diff --git a/pkg/skaffold/initializer/deploy/kubectl.go b/pkg/skaffold/initializer/deploy/kubectl.go index e720c3b486e..536775e40e5 100644 --- a/pkg/skaffold/initializer/deploy/kubectl.go +++ b/pkg/skaffold/initializer/deploy/kubectl.go @@ -17,13 +17,10 @@ limitations under the License. package deploy import ( - "path/filepath" + "fmt" + "os" - "github.com/pkg/errors" - - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/initializer/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/generator" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" ) @@ -76,23 +73,8 @@ func (k *kubectl) Validate() error { return nil } -// GenerateManifests implements the Initializer interface and -// generates manifests for each unresolved image -func (k *kubectl) GenerateManifests(unresolved []build.GeneratedBuilderImagePair) (map[string][]byte, error) { - generatedManifests := map[string][]byte{} - for _, pair := range unresolved { - manifest, err := generator.Generate(pair.ImageName) - if err != nil { - return nil, errors.Wrap(err, "generating kubernetes manifest") - } - path := filepath.Join(pair.ManifestPath, "deployment.yaml") - generatedManifests[path] = manifest - k.addManifestForImage(path, pair.ImageName) - } - return generatedManifests, nil -} - -func (k *kubectl) addManifestForImage(path, image string) { +func (k *kubectl) AddManifestForImage(path, image string) { + fmt.Fprintf(os.Stdout, "adding manifest path %s for image %s\n", path, image) k.configs = append(k.configs, path) k.images = append(k.images, image) } diff --git a/pkg/skaffold/initializer/init.go b/pkg/skaffold/initializer/init.go index 493b7b57c10..857b56658f1 100644 --- a/pkg/skaffold/initializer/init.go +++ b/pkg/skaffold/initializer/init.go @@ -61,13 +61,18 @@ func DoInit(ctx context.Context, out io.Writer, c config.Config) error { return buildInitializer.PrintAnalysis(out) } + // var generatedManifestPairs map[build.GeneratedBuilderImagePair][]byte var generatedManifests map[string][]byte if c.EnableManifestGeneration { - generatedManifests, err = deployInitializer.GenerateManifests(buildInitializer.GeneratedPairs()) + generatedManifestPairs, err := buildInitializer.GenerateManifests() if err != nil { return err } - buildInitializer.Resolve() + generatedManifests = make(map[string][]byte, len(generatedManifestPairs)) + for pair, manifest := range generatedManifestPairs { + deployInitializer.AddManifestForImage(pair.ManifestPath, pair.ImageName) + generatedManifests[pair.ManifestPath] = manifest + } } if err = deployInitializer.Validate(); err != nil { diff --git a/pkg/skaffold/initializer/prompt/prompt.go b/pkg/skaffold/initializer/prompt/prompt.go index 04704e89031..66b43b12591 100644 --- a/pkg/skaffold/initializer/prompt/prompt.go +++ b/pkg/skaffold/initializer/prompt/prompt.go @@ -50,8 +50,8 @@ func buildConfig(image string, choices []string) (string, error) { func WriteSkaffoldConfig(out io.Writer, pipeline []byte, generatedManifests map[string][]byte, filePath string) (bool, error) { fmt.Fprintln(out, string(pipeline)) - for f, m := range generatedManifests { - fmt.Fprintln(out, f, "-", string(m)) + for path, manifest := range generatedManifests { + fmt.Fprintln(out, path, "-", string(manifest)) } manifestString := ""