Merge branch 'master' into debug-iterations

Author: tejal29

DEVELOPMENT.md

@@ -103,6 +103,58 @@ For more details behind the logic of config changes see [the Skaffold config man
 
 We build the API directly through gRPC, which gets translated into REST API through a reverse proxy gateway library. If you make changes to the [proto/skaffold.proto](https://github.com/GoogleContainerTools/skaffold/blob/master/proto/skaffold.proto) file you can run `./hack/generate-proto.sh` to generate the equivalent Go code. 
 
+## Adding actionable error messages to code.
+Skaffold has a built-in framework to provide actionable error messages for user to help bootstrap skaffold errors.
+
+Also, [v1.19.0](https://github.com/GoogleContainerTools/skaffold/releases/tag/v1.19.0) onwards, skaffold is collecting failure error codes to help the team get more insights into common failure scenarios.
+
+To take advantage of this framework, contributors can simply use the [`ErrDef`](https://github.com/GoogleContainerTools/skaffold/blob/master/pkg/skaffold/errors/err_def.go#L32) struct to throw meaningful actionable error messages and 
+improve user experience.
+
+e.g In this example [PR](https://github.com/GoogleContainerTools/skaffold/pull/5273), 
+1. The contributor created 3 distinct error codes in [skaffold.proto](https://github.com/GoogleContainerTools/skaffold/pull/5088/files#diff-3883fe4549a47ae73a7a3a0afc00896b197d5ba8570906ba423769cf5a93a26f)
+   ```
+    // Docker build error when listing containers.
+    INIT_DOCKER_NETWORK_LISTING_CONTAINERS = 122;
+    // Docker build error indicating an invalid container name (or id).
+    INIT_DOCKER_NETWORK_INVALID_CONTAINER_NAME = 123;
+    // Docker build error indicating the container referenced does not exists in the docker context used.
+    INIT_DOCKER_NETWORK_CONTAINER_DOES_NOT_EXIST = 124
+   ```
+   The `INIT` in this case stands for skaffold `INIT` phase which includes, parsing of skaffold config and creating a skaffold runner. 
+   The other valid phases are `BUILD`, `DEPLOY`, `STATUSCHECK`. Complete list [here](https://skaffold.dev/docs/references/api/grpc/#statuscode)
+2. Run `hack/generate_proto.sh`. These will generate go code and structs for the newly added proto fields.
+   ```shell script
+    git status
+	   modified:   docs/content/en/api/skaffold.swagger.json
+	   modified:   docs/content/en/docs/references/api/grpc.md
+	   modified:   proto/skaffold.pb.go
+	   modified:   proto/skaffold.proto
+   ```
+3. The contributor then used these error codes when creating an error in their [proposed code change](https://github.com/GoogleContainerTools/skaffold/pull/5088/files#diff-3fc5246574bf7367a232c6d682b22a4e22795d52eb1c81fe2c27ff052939d507R220).
+   They used the constructor `sErrors.NewError` in [pkg/skaffold/errors](https://github.com/GoogleContainerTools/skaffold/blob/54466ff6983e9fcf977d6e549119b4c1c4dc9e2b/pkg/skaffold/errors/err_def.go#L57) to inantiate an object of struct `ErrDef`. 
+   `ErrDef` implements the golang `error` interface.
+   ```
+    err :=  sErrors.NewError(fmt.Errorf(errMsg), 
+      proto.ActionableErr{
+	    Message: errMsg,
+	    ErrCode: proto.StatusCode_INIT_DOCKER_NETWORK_INVALID_CONTAINER_NAME,
+		  Suggestions: []*proto.Suggestion{
+	  	    {
+			  SuggestionCode: proto.SuggestionCode_FIX_DOCKER_NETWORK_CONTAINER_NAME,
+			  Action:         "Please fix the docker network container name and try again",
+		    },
+		  },
+	  }))
+   ```
+
+With above two changes, skaffold will now show a meaning full error message when this error condition is met. 
+```shell script
+skaffold dev 
+invalid skaffold config: container 'does-not-exits' not found, required by image 'skaffold-example' for docker network stack sharing.
+Please fix the docker network container name and try again.
+```
+
 ## Building skaffold
 
 To build with your local changes you have two options:

cmd/skaffold/app/cmd/flags.go

@@ -120,6 +120,14 @@ var flagRegistry = []Flag{
 		FlagAddMethod: "StringVar",
 		DefinedOn:     []string{"dev", "build", "run", "debug"},
 	},
+	{
+		Name:          "remote-cache-dir",
+		Usage:         "Specify the location of the git repositories cache (default $HOME/.skaffold/repos)",
+		Value:         &opts.RepoCacheDir,
+		DefValue:      "",
+		FlagAddMethod: "StringVar",
+		DefinedOn:     []string{"all"},
+	},
 	{
 		Name:          "insecure-registry",
 		Usage:         "Target registries for built images which are not secure",

cmd/skaffold/app/cmd/parse_config.go

@@ -24,7 +24,10 @@ import (
 	"sort"
 	"strings"
 
+	"github.com/sirupsen/logrus"
+
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/git"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/defaults"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
@@ -45,9 +48,20 @@ type configOpts struct {
 	isDependency bool
 }
 
+// record captures the state of referenced configs.
+type record struct {
+	appliedProfiles  map[string]string      // config -> list of applied profiles
+	configNameToFile map[string]string      // configName -> file path
+	cachedRepos      map[string]interface{} // git repo -> cache path or error
+}
+
+func newRecord() *record {
+	return &record{appliedProfiles: make(map[string]string), configNameToFile: make(map[string]string), cachedRepos: make(map[string]interface{})}
+}
+
 func getAllConfigs(opts config.SkaffoldOptions) ([]*latest.SkaffoldConfig, error) {
 	cOpts := configOpts{file: opts.ConfigurationFile, selection: nil, profiles: opts.Profiles, isRequired: false, isDependency: false}
-	cfgs, err := getConfigs(cOpts, opts, make(map[string]string), make(map[string]string))
+	cfgs, err := getConfigs(cOpts, opts, newRecord())
 	if err != nil {
 		return nil, err
 	}
@@ -61,7 +75,7 @@ func getAllConfigs(opts config.SkaffoldOptions) ([]*latest.SkaffoldConfig, error
 }
 
 // getConfigs recursively parses all configs and their dependencies in the specified `skaffold.yaml`
-func getConfigs(cfgOpts configOpts, opts config.SkaffoldOptions, appliedProfiles map[string]string, configNameToFile map[string]string) ([]*latest.SkaffoldConfig, error) {
+func getConfigs(cfgOpts configOpts, opts config.SkaffoldOptions, r *record) ([]*latest.SkaffoldConfig, error) {
 	parsed, err := schema.ParseConfigAndUpgrade(cfgOpts.file, latest.Version)
 	if err != nil {
 		return nil, err
@@ -80,7 +94,7 @@ func getConfigs(cfgOpts configOpts, opts config.SkaffoldOptions, appliedProfiles
 	var configs []*latest.SkaffoldConfig
 	for i, cfg := range parsed {
 		config := cfg.(*latest.SkaffoldConfig)
-		processed, err := processEachConfig(config, cfgOpts, opts, appliedProfiles, configNameToFile, i)
+		processed, err := processEachConfig(config, cfgOpts, opts, r, i)
 		if err != nil {
 			return nil, err
 		}
@@ -90,14 +104,14 @@ func getConfigs(cfgOpts configOpts, opts config.SkaffoldOptions, appliedProfiles
 }
 
 // processEachConfig processes each parsed config by applying profiles and recursively processing it's dependencies
-func processEachConfig(config *latest.SkaffoldConfig, cfgOpts configOpts, opts config.SkaffoldOptions, appliedProfiles map[string]string, configNameToFile map[string]string, index int) ([]*latest.SkaffoldConfig, error) {
+func processEachConfig(config *latest.SkaffoldConfig, cfgOpts configOpts, opts config.SkaffoldOptions, r *record, index int) ([]*latest.SkaffoldConfig, error) {
 	// check that the same config name isn't repeated in multiple files.
 	if config.Metadata.Name != "" {
-		prevConfig, found := configNameToFile[config.Metadata.Name]
+		prevConfig, found := r.configNameToFile[config.Metadata.Name]
 		if found && prevConfig != cfgOpts.file {
 			return nil, fmt.Errorf("skaffold config named %q found in multiple files: %q and %q", config.Metadata.Name, prevConfig, cfgOpts.file)
 		}
-		configNameToFile[config.Metadata.Name] = cfgOpts.file
+		r.configNameToFile[config.Metadata.Name] = cfgOpts.file
 	}
 
 	// configSelection specifies the exact required configs in this file. Empty configSelection means that all configs are required.
@@ -124,13 +138,13 @@ func processEachConfig(config *latest.SkaffoldConfig, cfgOpts configOpts, opts c
 	}
 
 	sort.Strings(profiles)
-	if revisit, err := checkRevisit(config, profiles, appliedProfiles, cfgOpts.file, required, index); revisit {
+	if revisit, err := checkRevisit(config, profiles, r.appliedProfiles, cfgOpts.file, required, index); revisit {
 		return nil, err
 	}
 
 	var configs []*latest.SkaffoldConfig
 	for _, d := range config.Dependencies {
-		depConfigs, err := processEachDependency(d, cfgOpts.file, required, profiles, opts, appliedProfiles, configNameToFile)
+		depConfigs, err := processEachDependency(d, cfgOpts.file, required, profiles, opts, r)
 		if err != nil {
 			return nil, err
 		}
@@ -144,7 +158,7 @@ func processEachConfig(config *latest.SkaffoldConfig, cfgOpts configOpts, opts c
 }
 
 // processEachDependency parses a config dependency with the calculated set of activated profiles.
-func processEachDependency(d latest.ConfigDependency, fPath string, required bool, profiles []string, opts config.SkaffoldOptions, appliedProfiles, configNameToFile map[string]string) ([]*latest.SkaffoldConfig, error) {
+func processEachDependency(d latest.ConfigDependency, fPath string, required bool, profiles []string, opts config.SkaffoldOptions, r *record) ([]*latest.SkaffoldConfig, error) {
 	var depProfiles []string
 	for _, ap := range d.ActiveProfiles {
 		if len(ap.ActivatedBy) == 0 {
@@ -159,27 +173,60 @@ func processEachDependency(d latest.ConfigDependency, fPath string, required boo
 		}
 	}
 	path := d.Path
+
+	if d.GitRepo != nil {
+		cachePath, err := cacheRepo(*d.GitRepo, opts, r)
+		if err != nil {
+			return nil, fmt.Errorf("caching remote dependency %s: %w", d.GitRepo.Repo, err)
+		}
+		path = cachePath
+	}
+
 	if path == "" {
 		// empty path means configs in the same file
 		path = fPath
 	}
 	fi, err := os.Stat(path)
 	if err != nil {
 		if os.IsNotExist(errors.Unwrap(err)) {
-			return nil, fmt.Errorf("could not find skaffold config %s that is referenced as a dependency in config %s", d.Path, fPath)
+			return nil, fmt.Errorf("could not find skaffold config %s that is referenced as a dependency in config %s", path, fPath)
 		}
 		return nil, fmt.Errorf("parsing dependencies for skaffold config %s: %w", fPath, err)
 	}
 	if fi.IsDir() {
 		path = filepath.Join(path, "skaffold.yaml")
 	}
-	depConfigs, err := getConfigs(configOpts{file: path, selection: d.Names, profiles: depProfiles, isRequired: required, isDependency: path != fPath}, opts, appliedProfiles, configNameToFile)
+	depConfigs, err := getConfigs(configOpts{file: path, selection: d.Names, profiles: depProfiles, isRequired: required, isDependency: path != fPath}, opts, r)
 	if err != nil {
 		return nil, err
 	}
 	return depConfigs, nil
 }
 
+// cacheRepo downloads the referenced git repository to skaffold's cache if required and returns the path to the target configuration file in that repository.
+func cacheRepo(g latest.GitInfo, opts config.SkaffoldOptions, r *record) (string, error) {
+	key := fmt.Sprintf("%s@%s", g.Repo, g.Ref)
+	if p, found := r.cachedRepos[key]; found {
+		switch v := p.(type) {
+		case string:
+			return filepath.Join(v, g.Path), nil
+		case error:
+			return "", v
+		default:
+			logrus.Fatalf("unable to check download status of repo %s at ref %s", g.Repo, g.Ref)
+			return "", nil
+		}
+	} else {
+		p, err := git.SyncRepo(g, opts)
+		if err != nil {
+			r.cachedRepos[key] = err
+			return "", err
+		}
+		r.cachedRepos[key] = p
+		return filepath.Join(p, g.Path), nil
+	}
+}
+
 // checkRevisit ensures that each config is activated with the same set of active profiles
 // It returns true if this config was visited once before. It additionally returns an error if the previous visit was with a different set of active profiles.
 func checkRevisit(config *latest.SkaffoldConfig, profiles []string, appliedProfiles map[string]string, file string, required bool, index int) (bool, error) {

cmd/skaffold/app/cmd/parse_config_test.go

@@ -26,14 +26,15 @@ import (
 	"github.com/google/go-cmp/cmp"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/git"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
 const (
 	template = `
-apiVersion: skaffold/v2beta11
+apiVersion: %s
 kind: Config
 metadata:
   name: %s
@@ -329,6 +330,37 @@ requires:
 			},
 			err: errors.New("did not find any configs matching selection [cfg3]"),
 		},
+		{
+			description: "remote dependencies",
+			documents: []document{
+				{path: "skaffold.yaml", configs: []mockCfg{{name: "cfg00", requiresStanza: `
+requires:
+  - path: doc1
+`}, {name: "cfg01", requiresStanza: ""}}},
+				{path: "doc1/skaffold.yaml", configs: []mockCfg{{name: "cfg10", requiresStanza: `
+requires:
+  - git:
+      repo: doc2
+      path: skaffold.yaml
+      ref: main
+    configs: [cfg21]
+`}, {name: "cfg11", requiresStanza: `
+requires:
+  - git:
+      repo: doc2
+      ref: main
+    configs: [cfg21]
+`}}},
+				{path: "doc2/skaffold.yaml", configs: []mockCfg{{name: "cfg20", requiresStanza: ""}, {name: "cfg21", requiresStanza: ""}}},
+			},
+			expected: []*latest.SkaffoldConfig{
+				createCfg("cfg21", "image21", "doc2", nil),
+				createCfg("cfg10", "image10", "doc1", []latest.ConfigDependency{{GitRepo: &latest.GitInfo{Repo: "doc2", Path: "skaffold.yaml", Ref: "main"}, Names: []string{"cfg21"}}}),
+				createCfg("cfg11", "image11", "doc1", []latest.ConfigDependency{{GitRepo: &latest.GitInfo{Repo: "doc2", Ref: "main"}, Names: []string{"cfg21"}}}),
+				createCfg("cfg00", "image00", ".", []latest.ConfigDependency{{Path: "doc1"}}),
+				createCfg("cfg01", "image01", ".", nil),
+			},
+		},
 	}
 
 	for _, test := range tests {
@@ -338,7 +370,7 @@ requires:
 				var cfgs []string
 				for j, c := range d.configs {
 					id := fmt.Sprintf("%d%d", i, j)
-					s := fmt.Sprintf(template, c.name, c.requiresStanza, id, id, id)
+					s := fmt.Sprintf(template, latest.Version, c.name, c.requiresStanza, id, id, id)
 					cfgs = append(cfgs, s)
 				}
 				tmpDir.Write(d.path, strings.Join(cfgs, "\n---\n"))
@@ -355,10 +387,13 @@ requires:
 				wd, _ := util.RealWorkDir()
 				c.Build.Artifacts[0].Workspace = filepath.Join(wd, dir)
 				for i := range c.Dependencies {
+					if c.Dependencies[i].Path == "" {
+						continue
+					}
 					c.Dependencies[i].Path = filepath.Join(wd, dir, c.Dependencies[i].Path)
 				}
 			}
-
+			t.Override(&git.SyncRepo, func(g latest.GitInfo, _ config.SkaffoldOptions) (string, error) { return g.Repo, nil })
 			cfgs, err := getAllConfigs(config.SkaffoldOptions{
 				Command:             "dev",
 				ConfigurationFile:   test.documents[0].path,