Skip to content

Commit

Permalink
add source file and module to config parsing error description (#6087)
Browse files Browse the repository at this point in the history
  • Loading branch information
@gsquared94
gsquared94 authored Jun 28, 2021
1 parent 7dad6fa commit 9cd80d3
Show file tree
Hide file tree
Showing 5 changed files with 170 additions and 128 deletions.
8 changes: 6 additions & 2 deletions cmd/skaffold/app/cmd/fix.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/spf13/cobra"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/output"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/parser"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema"
latestV1 "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest/v1"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/validation"
Expand Down Expand Up @@ -81,9 +82,12 @@ func fix(out io.Writer, configFile string, toVersion string, overwrite bool) err
// TODO(dgageot): We should be able run validations on any schema version
// but that's not the case. They can only run on the latest version for now.
if toVersion == latestV1.Version {
var cfgs []*latestV1.SkaffoldConfig
var cfgs parser.SkaffoldConfigSet
for _, cfg := range versionedCfgs {
cfgs = append(cfgs, cfg.(*latestV1.SkaffoldConfig))
cfgs = append(cfgs, &parser.SkaffoldConfigEntry{
SkaffoldConfig: cfg.(*latestV1.SkaffoldConfig),
SourceFile: configFile,
IsRootConfig: true})
}
if err := validation.Process(cfgs); err != nil {
return fmt.Errorf("validating upgraded config: %w", err)
Expand Down
21 changes: 13 additions & 8 deletions cmd/skaffold/app/cmd/runner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,16 +73,19 @@ func createNewRunner(out io.Writer, opts config.SkaffoldOptions) (runner.Runner,
}

func runContext(out io.Writer, opts config.SkaffoldOptions) (*runcontext.RunContext, []*latestV1.SkaffoldConfig, error) {
configs, err := withFallbackConfig(out, opts, parser.GetAllConfigs)
cfgSet, err := withFallbackConfig(out, opts, parser.GetConfigSet)
if err != nil {
return nil, nil, err
}
setDefaultDeployer(configs)
setDefaultDeployer(cfgSet)

if err := validation.Process(configs); err != nil {
if err := validation.Process(cfgSet); err != nil {
return nil, nil, fmt.Errorf("invalid skaffold config: %w", err)
}

var configs []*latestV1.SkaffoldConfig
for _, cfg := range cfgSet {
configs = append(configs, cfg.SkaffoldConfig)
}
runCtx, err := runcontext.GetRunContext(opts, configs)
if err != nil {
return nil, nil, fmt.Errorf("getting run context: %w", err)
Expand All @@ -96,7 +99,7 @@ func runContext(out io.Writer, opts config.SkaffoldOptions) (*runcontext.RunCont
}

// withFallbackConfig will try to automatically generate a config if root `skaffold.yaml` file does not exist.
func withFallbackConfig(out io.Writer, opts config.SkaffoldOptions, getCfgs func(opts config.SkaffoldOptions) ([]*latestV1.SkaffoldConfig, error)) ([]*latestV1.SkaffoldConfig, error) {
func withFallbackConfig(out io.Writer, opts config.SkaffoldOptions, getCfgs func(opts config.SkaffoldOptions) (parser.SkaffoldConfigSet, error)) (parser.SkaffoldConfigSet, error) {
configs, err := getCfgs(opts)
if err == nil {
return configs, nil
Expand All @@ -115,7 +118,9 @@ func withFallbackConfig(out io.Writer, opts config.SkaffoldOptions, getCfgs func

defaults.Set(config)

return []*latestV1.SkaffoldConfig{config}, nil
return parser.SkaffoldConfigSet{
&parser.SkaffoldConfigEntry{SkaffoldConfig: config, IsRootConfig: true},
}, nil
}

return nil, fmt.Errorf("skaffold config file %s not found - check your current working directory, or try running `skaffold init`", opts.ConfigurationFile)
Expand All @@ -128,13 +133,13 @@ func withFallbackConfig(out io.Writer, opts config.SkaffoldOptions, getCfgs func
return nil, fmt.Errorf("parsing skaffold config: %w", err)
}

func setDefaultDeployer(configs []*latestV1.SkaffoldConfig) {
func setDefaultDeployer(configs parser.SkaffoldConfigSet) {
// do not set a default deployer in a multi-config application.
if len(configs) > 1 {
return
}
// there always exists at least one config
defaults.SetDefaultDeployer(configs[0])
defaults.SetDefaultDeployer(configs[0].SkaffoldConfig)
}

func warnIfUpdateIsAvailable() {
Expand Down
25 changes: 13 additions & 12 deletions pkg/skaffold/schema/samples_test.go → ...kaffold/schema/validation/samples_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package schema
package validation

import (
"bytes"
Expand All @@ -24,16 +24,17 @@ import (
"strings"
"testing"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/parser"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/defaults"
latestV1 "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest/v1"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/validation"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/walk"
"github.com/GoogleContainerTools/skaffold/testutil"
)

const (
samplesRoot = "../../../docs/content/en/samples"
samplesRoot = "../../../../docs/content/en/samples"
)

var (
Expand All @@ -43,9 +44,9 @@ var (
// Test that every example can be parsed and produces a valid
// Skaffold configuration.
func TestParseExamples(t *testing.T) {
parseConfigFiles(t, "../../../examples")
parseConfigFiles(t, "../../../integration/examples")
parseConfigFiles(t, "../../../integration/testdata/regressions")
parseConfigFiles(t, "../../../../examples")
parseConfigFiles(t, "../../../../integration/examples")
parseConfigFiles(t, "../../../../integration/testdata/regressions")
}

// Samples are skaffold.yaml fragments that are used
Expand Down Expand Up @@ -77,17 +78,17 @@ func TestParseSamples(t *testing.T) {

func checkSkaffoldConfig(t *testutil.T, yaml []byte) {
configFile := t.TempFile("skaffold.yaml", yaml)
parsed, err := ParseConfigAndUpgrade(configFile)
parsed, err := schema.ParseConfigAndUpgrade(configFile)
t.CheckNoError(err)
var cfgs []*latestV1.SkaffoldConfig
var cfgs parser.SkaffoldConfigSet
for _, p := range parsed {
cfg := p.(*latestV1.SkaffoldConfig)
err = defaults.Set(cfg)
defaults.SetDefaultDeployer(cfg)
cfg := &parser.SkaffoldConfigEntry{SkaffoldConfig: p.(*latestV1.SkaffoldConfig)}
err = defaults.Set(cfg.SkaffoldConfig)
defaults.SetDefaultDeployer(cfg.SkaffoldConfig)
t.CheckNoError(err)
cfgs = append(cfgs, cfg)
}
err = validation.Process(cfgs)
err = Process(cfgs)
t.CheckNoError(err)
}

Expand Down
60 changes: 38 additions & 22 deletions pkg/skaffold/schema/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,19 @@ package validation

import (
"context"
"errors"
"fmt"
"reflect"
"regexp"
"strings"
"time"

"github.com/docker/docker/api/types"
"github.com/pkg/errors"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/misc"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
sErrors "github.com/GoogleContainerTools/skaffold/pkg/skaffold/errors"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/parser"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext"
latestV1 "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest/v1"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
Expand All @@ -44,19 +45,21 @@ var (
)

// Process checks if the Skaffold pipeline is valid and returns all encountered errors as a concatenated string
func Process(configs []*latestV1.SkaffoldConfig) error {
func Process(configs parser.SkaffoldConfigSet) error {
var errs = validateImageNames(configs)
for _, config := range configs {
errs = append(errs, visitStructs(config, validateYamltags)...)
errs = append(errs, validateDockerNetworkMode(config.Build.Artifacts)...)
errs = append(errs, validateCustomDependencies(config.Build.Artifacts)...)
errs = append(errs, validateSyncRules(config.Build.Artifacts)...)
errs = append(errs, validatePortForwardResources(config.PortForward)...)
errs = append(errs, validateJibPluginTypes(config.Build.Artifacts)...)
errs = append(errs, validateLogPrefix(config.Deploy.Logs)...)
errs = append(errs, validateArtifactTypes(config.Build)...)
errs = append(errs, validateTaggingPolicy(config.Build)...)
errs = append(errs, validateCustomTest(config.Test)...)
var cfgErrs []error
cfgErrs = append(cfgErrs, visitStructs(config.SkaffoldConfig, validateYamltags)...)
cfgErrs = append(cfgErrs, validateDockerNetworkMode(config.Build.Artifacts)...)
cfgErrs = append(cfgErrs, validateCustomDependencies(config.Build.Artifacts)...)
cfgErrs = append(cfgErrs, validateSyncRules(config.Build.Artifacts)...)
cfgErrs = append(cfgErrs, validatePortForwardResources(config.PortForward)...)
cfgErrs = append(cfgErrs, validateJibPluginTypes(config.Build.Artifacts)...)
cfgErrs = append(cfgErrs, validateLogPrefix(config.Deploy.Logs)...)
cfgErrs = append(cfgErrs, validateArtifactTypes(config.Build)...)
cfgErrs = append(cfgErrs, validateTaggingPolicy(config.Build)...)
cfgErrs = append(cfgErrs, validateCustomTest(config.Test)...)
errs = append(errs, wrapWithContext(config, cfgErrs...)...)
}
errs = append(errs, validateArtifactDependencies(configs)...)
errs = append(errs, validateSingleKubeContext(configs)...)
Expand Down Expand Up @@ -100,35 +103,35 @@ func validateTaggingPolicy(bc latestV1.BuildConfig) (errs []error) {

// validateImageNames makes sure the artifact image names are unique and valid base names,
// without tags nor digests.
func validateImageNames(configs []*latestV1.SkaffoldConfig) (errs []error) {
seen := make(map[string]bool)
func validateImageNames(configs parser.SkaffoldConfigSet) (errs []error) {
seen := make(map[string]string)
for _, c := range configs {
for _, a := range c.Build.Artifacts {
if seen[a.ImageName] {
errs = append(errs, fmt.Errorf("found duplicate images %q: artifact image names must be unique across all configurations", a.ImageName))
if prevSource, found := seen[a.ImageName]; found {
errs = append(errs, fmt.Errorf("duplicate image %q found in sources %s and %s: artifact image names must be unique across all configurations", a.ImageName, prevSource, c.SourceFile))
continue
}

seen[a.ImageName] = true
seen[a.ImageName] = c.SourceFile
parsed, err := docker.ParseReference(a.ImageName)
if err != nil {
errs = append(errs, fmt.Errorf("invalid image %q: %w", a.ImageName, err))
errs = append(errs, wrapWithContext(c, fmt.Errorf("invalid image %q: %w", a.ImageName, err))...)
continue
}

if parsed.Tag != "" {
errs = append(errs, fmt.Errorf("invalid image %q: no tag should be specified. Use taggers instead: https://skaffold.dev/docs/how-tos/taggers/", a.ImageName))
errs = append(errs, wrapWithContext(c, fmt.Errorf("invalid image %q: no tag should be specified. Use taggers instead: https://skaffold.dev/docs/how-tos/taggers/", a.ImageName))...)
}

if parsed.Digest != "" {
errs = append(errs, fmt.Errorf("invalid image %q: no digest should be specified. Use taggers instead: https://skaffold.dev/docs/how-tos/taggers/", a.ImageName))
errs = append(errs, wrapWithContext(c, fmt.Errorf("invalid image %q: no digest should be specified. Use taggers instead: https://skaffold.dev/docs/how-tos/taggers/", a.ImageName))...)
}
}
}
return
}

func validateArtifactDependencies(configs []*latestV1.SkaffoldConfig) (errs []error) {
func validateArtifactDependencies(configs parser.SkaffoldConfigSet) (errs []error) {
var artifacts []*latestV1.Artifact
for _, c := range configs {
artifacts = append(artifacts, c.Build.Artifacts...)
Expand Down Expand Up @@ -528,7 +531,7 @@ func validateLogPrefix(lc latestV1.LogsConfig) []error {
return nil
}

func validateSingleKubeContext(configs []*latestV1.SkaffoldConfig) []error {
func validateSingleKubeContext(configs parser.SkaffoldConfigSet) []error {
if len(configs) < 2 {
return nil
}
Expand Down Expand Up @@ -565,3 +568,16 @@ func validateCustomTest(tcs []*latestV1.TestCase) (errs []error) {
}
return
}

func wrapWithContext(config *parser.SkaffoldConfigEntry, errs ...error) []error {
var id string
if config.Metadata.Name != "" {
id = fmt.Sprintf("module %q", config.Metadata.Name)
} else {
id = fmt.Sprintf("unnamed config at index %d", config.SourceIndex)
}
for i := range errs {
errs[i] = errors.Wrapf(errs[i], "source: %s, %s", config.SourceFile, id)
}
return errs
}
Loading

0 comments on commit 9cd80d3

Please sign in to comment.