cleanup on analyzers and moving things into a single package (#3538)

Author: balopat

pkg/skaffold/initializer/analyze.go

@@ -17,14 +17,11 @@ limitations under the License.
 package initializer
 
 import (
-	"fmt"
 	"path/filepath"
 	"sort"
 
 	"github.com/karrick/godirwalk"
-	"github.com/sirupsen/logrus"
 
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/initializer/kubectl"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 )
 
@@ -34,86 +31,9 @@ type analysis struct {
 	builderAnalyzer  *builderAnalyzer
 }
 
-// analyzer is a generic Visitor that is called on every file in the directory
-// It can manage state and react to walking events assuming a bread first search
-type analyzer interface {
-	enterDir(dir string)
-	analyzeFile(file string) error
-	exitDir(dir string)
-}
-
-type directoryAnalyzer struct {
-	currentDir string
-}
-
-func (a *directoryAnalyzer) analyzeFile(filePath string) error {
-	return nil
-}
-
-func (a *directoryAnalyzer) enterDir(dir string) {
-	a.currentDir = dir
-}
-
-func (a *directoryAnalyzer) exitDir(dir string) {
-	//pass
-}
-
-type kubectlAnalyzer struct {
-	directoryAnalyzer
-	kubernetesManifests []string
-}
-
-func (a *kubectlAnalyzer) analyzeFile(filePath string) error {
-	if kubectl.IsKubernetesManifest(filePath) && !IsSkaffoldConfig(filePath) {
-		a.kubernetesManifests = append(a.kubernetesManifests, filePath)
-	}
-	return nil
-}
-
-type skaffoldConfigAnalyzer struct {
-	directoryAnalyzer
-	force bool
-}
-
-func (a *skaffoldConfigAnalyzer) analyzeFile(filePath string) error {
-	if !IsSkaffoldConfig(filePath) {
-		return nil
-	}
-	if !a.force {
-		return fmt.Errorf("pre-existing %s found (you may continue with --force)", filePath)
-	}
-	logrus.Debugf("%s is a valid skaffold configuration: continuing since --force=true", filePath)
-	return nil
-}
-
-type builderAnalyzer struct {
-	directoryAnalyzer
-	enableJibInit       bool
-	enableBuildpackInit bool
-	findBuilders        bool
-	foundBuilders       []InitBuilder
-
-	parentDirToStopFindBuilders string
-}
-
-func (a *builderAnalyzer) analyzeFile(filePath string) error {
-	if a.findBuilders && (a.parentDirToStopFindBuilders == "" || a.parentDirToStopFindBuilders == a.currentDir) {
-		builderConfigs, continueSearchingBuilders := detectBuilders(a.enableJibInit, a.enableBuildpackInit, filePath)
-		a.foundBuilders = append(a.foundBuilders, builderConfigs...)
-		if !continueSearchingBuilders {
-			a.parentDirToStopFindBuilders = a.currentDir
-		}
-	}
-	return nil
-}
-
-func (a *builderAnalyzer) exitDir(dir string) {
-	if a.parentDirToStopFindBuilders == dir {
-		a.parentDirToStopFindBuilders = ""
-	}
-}
-
-// analyze recursively walks a directory and returns the k8s configs and builder configs that it finds
+// analyze recursively walks a directory and notifies the analyzers of files and enterDir and exitDir events
+// at the end of the analyze function the analysis struct's analyzers should contain the state that we can
+// use to do further computation.
 func (a *analysis) analyze(dir string) error {
 	for _, analyzer := range a.analyzers() {
 		analyzer.enterDir(dir)
@@ -168,3 +88,31 @@ func (a *analysis) analyzers() []analyzer {
 		a.builderAnalyzer,
 	}
 }
+
+// analyzer is following the visitor pattern. It is called on every file
+// as the analysis.analyze function walks the directory structure recursively.
+// It can manage state and react to walking events assuming a breadth first search.
+type analyzer interface {
+	enterDir(dir string)
+	analyzeFile(file string) error
+	exitDir(dir string)
+}
+
+// directoryAnalyzer is a base analyzer that can be included in every analyzer as a convenience
+// it saves the current directory on enterDir events. Benefits to include this into other analyzers is that
+// they can rely on the current directory var, but also they don't have to implement enterDir and exitDir.
+type directoryAnalyzer struct {
+	currentDir string
+}
+
+func (a *directoryAnalyzer) analyzeFile(filePath string) error {
+	return nil
+}
+
+func (a *directoryAnalyzer) enterDir(dir string) {
+	a.currentDir = dir
+}
+
+func (a *directoryAnalyzer) exitDir(dir string) {
+	//pass
+}

pkg/skaffold/initializer/builders.go

@@ -32,6 +32,33 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 )
 
+type builderAnalyzer struct {
+	directoryAnalyzer
+	enableJibInit       bool
+	enableBuildpackInit bool
+	findBuilders        bool
+	foundBuilders       []InitBuilder
+
+	parentDirToStopFindBuilders string
+}
+
+func (a *builderAnalyzer) analyzeFile(filePath string) error {
+	if a.findBuilders && (a.parentDirToStopFindBuilders == "" || a.parentDirToStopFindBuilders == a.currentDir) {
+		builderConfigs, continueSearchingBuilders := detectBuilders(a.enableJibInit, a.enableBuildpackInit, filePath)
+		a.foundBuilders = append(a.foundBuilders, builderConfigs...)
+		if !continueSearchingBuilders {
+			a.parentDirToStopFindBuilders = a.currentDir
+		}
+	}
+	return nil
+}
+
+func (a *builderAnalyzer) exitDir(dir string) {
+	if a.parentDirToStopFindBuilders == dir {
+		a.parentDirToStopFindBuilders = ""
+	}
+}
+
 // autoSelectBuilders takes a list of builders and images, checks if any of the builders' configured target
 // images match an image in the image list, and returns a list of the matching builder/image pairs. Also
 // separately returns the builder configs and images that didn't have any matches.

pkg/skaffold/initializer/config.go

@@ -17,6 +17,7 @@ limitations under the License.
 package initializer
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -33,7 +34,23 @@ var (
 	getWd = os.Getwd
 )
 
-func generateSkaffoldConfig(k DeploymentInitializer, buildConfigPairs []builderImagePair) *latest.SkaffoldConfig {
+type skaffoldConfigAnalyzer struct {
+	directoryAnalyzer
+	force bool
+}
+
+func (a *skaffoldConfigAnalyzer) analyzeFile(filePath string) error {
+	if !isSkaffoldConfig(filePath) {
+		return nil
+	}
+	if !a.force {
+		return fmt.Errorf("pre-existing %s found (you may continue with --force)", filePath)
+	}
+	logrus.Debugf("%s is a valid skaffold configuration: continuing since --force=true", filePath)
+	return nil
+}
+
+func generateSkaffoldConfig(k deploymentInitializer, buildConfigPairs []builderImagePair) *latest.SkaffoldConfig {
 	// if we're here, the user has no skaffold yaml so we need to generate one
 	// if the user doesn't have any k8s yamls, generate one for each dockerfile
 	logrus.Info("generating skaffold config")
@@ -53,7 +70,7 @@ func generateSkaffoldConfig(k DeploymentInitializer, buildConfigPairs []builderI
 			Build: latest.BuildConfig{
 				Artifacts: artifacts(buildConfigPairs),
 			},
-			Deploy: k.GenerateDeployConfig(),
+			Deploy: k.deployConfig(),
 		},
 	}
 }

pkg/skaffold/initializer/config_test.go

@@ -28,11 +28,11 @@ import (
 )
 
 type stubDeploymentInitializer struct {
-	deployConfig latest.DeployConfig
+	config latest.DeployConfig
 }
 
-func (s stubDeploymentInitializer) GenerateDeployConfig() latest.DeployConfig {
-	return s.deployConfig
+func (s stubDeploymentInitializer) deployConfig() latest.DeployConfig {
+	return s.config
 }
 
 func (s stubDeploymentInitializer) GetImages() []string {

pkg/skaffold/initializer/init.go

@@ -31,7 +31,6 @@ import (
 	"github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/tips"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/initializer/kubectl"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/warnings"
 )
@@ -40,10 +39,10 @@ import (
 // an image we parse out from a Kubernetes manifest
 const NoBuilder = "None (image not built from these sources)"
 
-// DeploymentInitializer detects a deployment type and is able to extract image names from it
-type DeploymentInitializer interface {
-	// GenerateDeployConfig generates Deploy Config for skaffold configuration.
-	GenerateDeployConfig() latest.DeployConfig
+// deploymentInitializer detects a deployment type and is able to extract image names from it
+type deploymentInitializer interface {
+	// deployConfig generates Deploy Config for skaffold configuration.
+	deployConfig() latest.DeployConfig
 	// GetImages fetches all the images defined in the manifest files.
 	GetImages() []string
 }
@@ -108,7 +107,7 @@ func DoInit(ctx context.Context, out io.Writer, c Config) error {
 		return err
 	}
 
-	k, err := kubectl.New(a.kubectlAnalyzer.kubernetesManifests)
+	k, err := newKubectlInitializer(a.kubectlAnalyzer.kubernetesManifests)
 	if err != nil {
 		return err
 	}

pkg/skaffold/initializer/kubectl/kubectl.go renamed to pkg/skaffold/initializer/kubectl.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package kubectl
+package initializer
 
 import (
 	"bufio"
@@ -32,29 +32,39 @@ import (
 
 var requiredFields = []string{"apiVersion", "kind", "metadata"}
 
-// Kubectl holds parameters to run kubectl.
-type Kubectl struct {
+// kubectl implements deploymentInitializer for the kubectl deployer.
+type kubectl struct {
 	configs []string
 	images  []string
 }
 
-// New returns a Kubectl skaffold generator.
-func New(potentialConfigs []string) (*Kubectl, error) {
+// kubectlAnalyzer is a Visitor during the directory analysis that collects kubernetes manifests
+type kubectlAnalyzer struct {
+	directoryAnalyzer
+	kubernetesManifests []string
+}
+
+func (a *kubectlAnalyzer) analyzeFile(filePath string) error {
+	if IsKubernetesManifest(filePath) && !isSkaffoldConfig(filePath) {
+		a.kubernetesManifests = append(a.kubernetesManifests, filePath)
+	}
+	return nil
+}
+
+// newKubectlInitializer returns a kubectl skaffold generator.
+func newKubectlInitializer(potentialConfigs []string) (*kubectl, error) {
 	var k8sConfigs, images []string
 	for _, file := range potentialConfigs {
 		imgs, err := parseImagesFromKubernetesYaml(file)
 		if err == nil {
-			logrus.Infof("found valid k8s yaml: %s", file)
 			k8sConfigs = append(k8sConfigs, file)
 			images = append(images, imgs...)
-		} else {
-			logrus.Infof("invalid k8s yaml %s: %s", file, err.Error())
 		}
 	}
 	if len(k8sConfigs) == 0 {
 		return nil, errors.New("one or more valid Kubernetes manifests is required to run skaffold")
 	}
-	return &Kubectl{
+	return &kubectl{
 		configs: k8sConfigs,
 		images:  images,
 	}, nil
@@ -70,9 +80,9 @@ func IsKubernetesManifest(file string) bool {
 	return err == nil
 }
 
-// GenerateDeployConfig implements the Initializer interface and generates
+// deployConfig implements the Initializer interface and generates
 // skaffold kubectl deployment config.
-func (k *Kubectl) GenerateDeployConfig() latest.DeployConfig {
+func (k *kubectl) deployConfig() latest.DeployConfig {
 	return latest.DeployConfig{
 		DeployType: latest.DeployType{
 			KubectlDeploy: &latest.KubectlDeploy{
@@ -84,7 +94,7 @@ func (k *Kubectl) GenerateDeployConfig() latest.DeployConfig {
 
 // GetImages implements the Initializer interface and lists all the
 // images present in the k8 manifest files.
-func (k *Kubectl) GetImages() []string {
+func (k *kubectl) GetImages() []string {
 	return k.images
 }
 

pkg/skaffold/initializer/kubectl/kubectl_test.go renamed to pkg/skaffold/initializer/kubectl_test.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package kubectl
+package initializer
 
 import (
 	"testing"
@@ -38,7 +38,7 @@ spec:
 `)
 	filename := tmpDir.Path("deployment.yaml")
 
-	k, err := New([]string{filename})
+	k, err := newKubectlInitializer([]string{filename})
 	if err != nil {
 		t.Fatal("failed to create a pipeline")
 	}
@@ -50,7 +50,7 @@ spec:
 			},
 		},
 	}
-	testutil.CheckDeepEqual(t, expectedConfig, k.GenerateDeployConfig())
+	testutil.CheckDeepEqual(t, expectedConfig, k.deployConfig())
 }
 
 func TestParseImagesFromKubernetesYaml(t *testing.T) {

pkg/skaffold/initializer/util.go

@@ -18,8 +18,8 @@ package initializer
 
 import "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema"
 
-// IsSkaffoldConfig is for determining if a file is skaffold config file.
-func IsSkaffoldConfig(file string) bool {
+// isSkaffoldConfig is for determining if a file is skaffold config file.
+func isSkaffoldConfig(file string) bool {
 	if config, err := schema.ParseConfig(file, false); err == nil && config != nil {
 		return true
 	}

pkg/skaffold/initializer/util_test.go

@@ -55,7 +55,7 @@ deploy:
 			tmpDir := t.NewTempDir().
 				Write("skaffold.yaml", test.contents)
 
-			isValid := IsSkaffoldConfig(tmpDir.Path("skaffold.yaml"))
+			isValid := isSkaffoldConfig(tmpDir.Path("skaffold.yaml"))
 
 			t.CheckDeepEqual(test.isValid, isValid)
 		})