Simplify skaffold init code (#3406)

* Remove duplication

Signed-off-by: David Gageot <[email protected]>

* Remove some more duplication

Signed-off-by: David Gageot <[email protected]>

* Simplify code

Signed-off-by: David Gageot <[email protected]>

* Remove duplication

Signed-off-by: David Gageot <[email protected]>

* Fix #3405

Signed-off-by: David Gageot <[email protected]>

* Simplify code

Signed-off-by: David Gageot <[email protected]>

* Use similar code for every artifact type

Signed-off-by: David Gageot <[email protected]>

* Fix typo

Signed-off-by: David Gageot <[email protected]>

* Make sure we generate the same yaml as we used to

Signed-off-by: David Gageot <[email protected]>

Author: dgageot

pkg/skaffold/build/buildpacks/init.go

@@ -25,58 +25,48 @@ import (
 
 // For testing
 var (
-	ValidateConfig = validateConfig
+	Validate = validate
 )
 
 // Name is the name of the Buildpack builder
 var Name = "Buildpacks"
 
-// Buildpack holds information about a Buildpack project
-type Buildpacks struct {
+// ArtifactConfig holds information about a Buildpack project
+type ArtifactConfig struct {
 	File string `json:"path,omitempty"`
 }
 
 // Name returns the name of the builder
-func (b Buildpacks) Name() string {
+func (c ArtifactConfig) Name() string {
 	return Name
 }
 
 // Describe returns the initBuilder's string representation, used when prompting the user to choose a builder.
-func (b Buildpacks) Describe() string {
-	return fmt.Sprintf("%s (%s)", b.Name(), b.File)
+func (c ArtifactConfig) Describe() string {
+	return fmt.Sprintf("%s (%s)", c.Name(), c.File)
 }
 
 // CreateArtifact creates an Artifact to be included in the generated Build Config
-func (b Buildpacks) CreateArtifact(manifestImage string) *latest.Artifact {
-	a := &latest.Artifact{
-		ImageName: manifestImage,
-		ArtifactType: latest.ArtifactType{
-			BuildpackArtifact: &latest.BuildpackArtifact{
-				Builder: "heroku/buildpacks",
-			},
+func (c ArtifactConfig) UpdateArtifact(a *latest.Artifact) {
+	a.ArtifactType = latest.ArtifactType{
+		BuildpackArtifact: &latest.BuildpackArtifact{
+			Builder: "heroku/buildpacks",
 		},
 	}
-
-	workspace := filepath.Dir(b.File)
-	if workspace != "." {
-		a.Workspace = workspace
-	}
-
-	return a
 }
 
 // ConfiguredImage returns the target image configured by the builder, or empty string if no image is configured
-func (b Buildpacks) ConfiguredImage() string {
-	// Target image is not configured in dockerfiles
+func (c ArtifactConfig) ConfiguredImage() string {
+	// Target image is not configured in buildpacks
 	return ""
 }
 
 // Path returns the path to the build definition
-func (b Buildpacks) Path() string {
-	return b.File
+func (c ArtifactConfig) Path() string {
+	return c.File
 }
 
-// validateConfig checks if a file is a valid Buildpack configuration.
-func validateConfig(path string) bool {
+// validate checks if a file is a valid Buildpack configuration.
+func validate(path string) bool {
 	return filepath.Base(path) == "package.json"
 }

pkg/skaffold/build/buildpacks/init_test.go

@@ -24,7 +24,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
-func TestValidateConfig(t *testing.T) {
+func TestValidate(t *testing.T) {
 	var tests = []struct {
 		description   string
 		path          string
@@ -45,7 +45,7 @@ func TestValidateConfig(t *testing.T) {
 		testutil.Run(t, test.description, func(t *testutil.T) {
 			tmpDir := t.NewTempDir().Touch(test.path)
 
-			isValid := ValidateConfig(tmpDir.Path(test.path))
+			isValid := Validate(tmpDir.Path(test.path))
 
 			t.CheckDeepEqual(test.expectedValid, isValid)
 		})
@@ -55,12 +55,12 @@ func TestValidateConfig(t *testing.T) {
 func TestDescribe(t *testing.T) {
 	var tests = []struct {
 		description    string
-		config         Buildpacks
+		config         ArtifactConfig
 		expectedPrompt string
 	}{
 		{
 			description:    "buildpacks",
-			config:         Buildpacks{File: "/path/to/package.json"},
+			config:         ArtifactConfig{File: "/path/to/package.json"},
 			expectedPrompt: "Buildpacks (/path/to/package.json)",
 		},
 	}
@@ -71,32 +71,16 @@ func TestDescribe(t *testing.T) {
 	}
 }
 
-func TestCreateArtifact(t *testing.T) {
+func TestUpdateArtifact(t *testing.T) {
 	var tests = []struct {
 		description      string
-		config           Buildpacks
-		manifestImage    string
+		config           ArtifactConfig
 		expectedArtifact latest.Artifact
-		expectedImage    string
 	}{
 		{
-			description:   "buildpacks",
-			config:        Buildpacks{File: filepath.Join("path", "to", "package.json")},
-			manifestImage: "image",
+			description: "buildpacks",
+			config:      ArtifactConfig{File: filepath.Join("path", "to", "package.json")},
 			expectedArtifact: latest.Artifact{
-				ImageName: "image",
-				Workspace: filepath.Join("path", "to"),
-				ArtifactType: latest.ArtifactType{BuildpackArtifact: &latest.BuildpackArtifact{
-					Builder: "heroku/buildpacks",
-				}},
-			},
-		},
-		{
-			description:   "ignore workspace",
-			config:        Buildpacks{File: "build.gradle"},
-			manifestImage: "other-image",
-			expectedArtifact: latest.Artifact{
-				ImageName: "other-image",
 				ArtifactType: latest.ArtifactType{BuildpackArtifact: &latest.BuildpackArtifact{
 					Builder: "heroku/buildpacks",
 				}},
@@ -105,7 +89,9 @@ func TestCreateArtifact(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			artifact := test.config.CreateArtifact(test.manifestImage)
+			artifact := &latest.Artifact{}
+
+			test.config.UpdateArtifact(artifact)
 
 			t.CheckDeepEqual(test.expectedArtifact, *artifact)
 		})

pkg/skaffold/build/jib/init.go

@@ -34,57 +34,47 @@ import (
 
 // For testing
 var (
-	ValidateJibConfigFunc = ValidateJibConfig
+	Validate = validate
 )
 
-// Jib holds information about a Jib project
-type Jib struct {
+// ArtifactConfig holds information about a Jib project
+type ArtifactConfig struct {
 	BuilderName string `json:"-"`
 	Image       string `json:"image,omitempty"`
-	FilePath    string `json:"path,omitempty"`
+	File        string `json:"path,omitempty"`
 	Project     string `json:"project,omitempty"`
 }
 
 // Name returns the name of the builder
-func (j Jib) Name() string {
-	return j.BuilderName
+func (c ArtifactConfig) Name() string {
+	return c.BuilderName
 }
 
 // Describe returns the initBuilder's string representation, used when prompting the user to choose a builder.
-func (j Jib) Describe() string {
-	if j.Project != "" {
-		return fmt.Sprintf("%s (%s, %s)", j.BuilderName, j.Project, j.FilePath)
+func (c ArtifactConfig) Describe() string {
+	if c.Project != "" {
+		return fmt.Sprintf("%s (%s, %s)", c.BuilderName, c.Project, c.File)
 	}
-	return fmt.Sprintf("%s (%s)", j.BuilderName, j.FilePath)
+	return fmt.Sprintf("%s (%s)", c.BuilderName, c.File)
 }
 
 // CreateArtifact creates an Artifact to be included in the generated Build Config
-func (j Jib) CreateArtifact(manifestImage string) *latest.Artifact {
-	workspace := filepath.Dir(j.FilePath)
-
-	a := &latest.Artifact{ImageName: manifestImage}
-
-	if workspace != "." {
-		a.Workspace = workspace
+func (c ArtifactConfig) UpdateArtifact(a *latest.Artifact) {
+	a.ArtifactType = latest.ArtifactType{
+		JibArtifact: &latest.JibArtifact{
+			Project: c.Project,
+		},
 	}
-
-	jib := &latest.JibArtifact{}
-	if j.Project != "" {
-		jib.Project = j.Project
-	}
-	a.ArtifactType = latest.ArtifactType{JibArtifact: jib}
-
-	return a
 }
 
 // ConfiguredImage returns the target image configured by the builder, or empty string if no image is configured
-func (j Jib) ConfiguredImage() string {
-	return j.Image
+func (c ArtifactConfig) ConfiguredImage() string {
+	return c.Image
 }
 
 // Path returns the path to the build definition
-func (j Jib) Path() string {
-	return j.FilePath
+func (c ArtifactConfig) Path() string {
+	return c.File
 }
 
 // BuilderConfig contains information about inferred Jib configurations
@@ -93,8 +83,8 @@ type jibJSON struct {
 	Project string `json:"project"`
 }
 
-// ValidateJibConfig checks if a file is a valid Jib configuration. Returns the list of Config objects corresponding to each Jib project built by the file, or nil if Jib is not configured.
-func ValidateJibConfig(path string) []Jib {
+// validate checks if a file is a valid Jib configuration. Returns the list of Config objects corresponding to each Jib project built by the file, or nil if Jib is not configured.
+func validate(path string) []ArtifactConfig {
 	// Determine whether maven or gradle
 	var builderType PluginType
 	var executable, wrapper, taskName, searchString string
@@ -137,8 +127,8 @@ func ValidateJibConfig(path string) []Jib {
 		return nil
 	}
 
-	results := make([]Jib, len(matches))
-	for i, match := range matches {
+	var results []ArtifactConfig
+	for _, match := range matches {
 		// Escape windows path separators
 		line := bytes.Replace(match[1], []byte(`\`), []byte(`\\`), -1)
 		parsedJSON := jibJSON{}
@@ -147,7 +137,12 @@ func ValidateJibConfig(path string) []Jib {
 			return nil
 		}
 
-		results[i] = Jib{BuilderName: PluginName(builderType), Image: parsedJSON.Image, FilePath: path, Project: parsedJSON.Project}
+		results = append(results, ArtifactConfig{
+			BuilderName: PluginName(builderType),
+			Image:       parsedJSON.Image,
+			File:        path,
+			Project:     parsedJSON.Project,
+		})
 	}
 	return results
 }