Fix error message when the skaffold.yaml is not found (#1947)

* Fix the error message when loading the default `skaffold.yaml`
* Don't check for updates if the error is a missing config file
* Unit test `newRunner()`

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

cmd/skaffold/app/cmd/runner.go

@@ -17,6 +17,8 @@ limitations under the License.
 package cmd
 
 import (
+	"os"
+
 	configutil "github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/cmd/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
@@ -35,10 +37,13 @@ import (
 func newRunner(opts *config.SkaffoldOptions) (*runner.SkaffoldRunner, *latest.SkaffoldConfig, error) {
 	parsed, err := schema.ParseConfig(opts.ConfigurationFile, true)
 	if err != nil {
-		latest, current, versionErr := update.GetLatestAndCurrentVersion()
-		if versionErr == nil && latest.GT(current) {
-			logrus.Warnf("Your Skaffold version might be too old. Download the latest version (%s) at %s\n", latest, constants.LatestDownloadURL)
+		// If the error is NOT that the file doesn't exist, then we warn the user
+		// that maybe they are using an outdated version of Skaffold that's unable to read
+		// the configuration.
+		if !os.IsNotExist(err) {
+			warnIfUpdateIsAvailable()
 		}
+
 		return nil, nil, errors.Wrap(err, "parsing skaffold config")
 	}
 
@@ -71,6 +76,13 @@ func newRunner(opts *config.SkaffoldOptions) (*runner.SkaffoldRunner, *latest.Sk
 	return runner, config, nil
 }
 
+func warnIfUpdateIsAvailable() {
+	latest, current, versionErr := update.GetLatestAndCurrentVersion()
+	if versionErr == nil && latest.GT(current) {
+		logrus.Warnf("Your Skaffold version might be too old. Download the latest version (%s) at %s\n", latest, constants.LatestDownloadURL)
+	}
+}
+
 func applyDefaultRepoSubstitution(config *latest.SkaffoldConfig, defaultRepo string) {
 	if defaultRepo == "" {
 		// noop

cmd/skaffold/app/cmd/runner_test.go

@@ -0,0 +1,79 @@
+/*
+Copyright 2019 The Skaffold Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cmd
+
+import (
+	"fmt"
+	"os"
+	"testing"
+
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/testutil"
+	"github.com/pkg/errors"
+)
+
+func writeSkaffoldConfig(t *testing.T, content string) (string, func()) {
+	yaml := fmt.Sprintf("apiVersion: %s\nkind: Config\n%s", latest.Version, content)
+	return testutil.TempFile(t, "skaffold.yaml", []byte(yaml))
+}
+
+func TestNewRunner(t *testing.T) {
+	cfg, delete := writeSkaffoldConfig(t, "")
+	defer delete()
+
+	_, _, err := newRunner(&config.SkaffoldOptions{
+		ConfigurationFile: cfg,
+		Trigger:           "polling",
+	})
+
+	testutil.CheckError(t, false, err)
+}
+
+func TestNewRunnerMissingConfig(t *testing.T) {
+	_, _, err := newRunner(&config.SkaffoldOptions{
+		ConfigurationFile: "missing-skaffold.yaml",
+	})
+
+	testutil.CheckError(t, true, err)
+	if !os.IsNotExist(errors.Cause(err)) {
+		t.Error("error should say that file is missing")
+	}
+}
+
+func TestNewRunnerInvalidConfig(t *testing.T) {
+	cfg, delete := writeSkaffoldConfig(t, "invalid")
+	defer delete()
+
+	_, _, err := newRunner(&config.SkaffoldOptions{
+		ConfigurationFile: cfg,
+	})
+
+	testutil.CheckErrorContains(t, "parsing skaffold config", err)
+}
+
+func TestNewRunnerUnknownProfile(t *testing.T) {
+	cfg, delete := writeSkaffoldConfig(t, "")
+	defer delete()
+
+	_, _, err := newRunner(&config.SkaffoldOptions{
+		ConfigurationFile: cfg,
+		Profiles:          []string{"unknown-profile"},
+	})
+
+	testutil.CheckErrorContains(t, "applying profiles", err)
+}

pkg/skaffold/util/config.go

@@ -0,0 +1,57 @@
+/*
+Copyright 2019 The Skaffold Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package util
+
+import (
+	"io/ioutil"
+	"os"
+
+	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
+)
+
+// ReadConfiguration reads a `skaffold.yaml` configuration and
+// returns its content.
+func ReadConfiguration(filename string) ([]byte, error) {
+	switch {
+	case filename == "":
+		return nil, errors.New("filename not specified")
+	case filename == "-":
+		return ioutil.ReadAll(os.Stdin)
+	case IsURL(filename):
+		return Download(filename)
+	default:
+		contents, err := ioutil.ReadFile(filename)
+		if err != nil {
+			// If the config file is the default `skaffold.yaml`,
+			// then we also try to read `skaffold.yml`.
+			if filename == "skaffold.yaml" {
+				logrus.Infof("Could not open skaffold.yaml: \"%s\"", err)
+				logrus.Infof("Trying to read from skaffold.yml instead")
+				contents, errIgnored := ioutil.ReadFile("skaffold.yml")
+				if errIgnored != nil {
+					// Return original error because it's the one that matters
+					return nil, err
+				}
+
+				return contents, nil
+			}
+		}
+
+		return contents, err
+	}
+}

pkg/skaffold/util/config_test.go

@@ -0,0 +1,66 @@
+/*
+Copyright 2019 The Skaffold Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package util
+
+import (
+	"os"
+	"testing"
+
+	"github.com/GoogleContainerTools/skaffold/testutil"
+)
+
+func TestReadConfiguration(t *testing.T) {
+	localFile, delete := testutil.TempFile(t, "skaffold.yaml", []byte("some yaml"))
+	defer delete()
+
+	content, err := ReadConfiguration(localFile)
+
+	testutil.CheckErrorAndDeepEqual(t, false, err, []byte("some yaml"), content)
+}
+
+func TestReadConfigurationFallback(t *testing.T) {
+	tmpDir, cleanup := testutil.NewTempDir(t)
+	defer cleanup()
+
+	reset := testutil.Chdir(t, tmpDir.Root())
+	defer reset()
+
+	// skaffold.yaml doesn't exist but .yml does
+	tmpDir.Write("skaffold.yml", "some yaml")
+
+	content, err := ReadConfiguration("skaffold.yaml")
+
+	testutil.CheckErrorAndDeepEqual(t, false, err, []byte("some yaml"), content)
+}
+
+func TestReadConfigurationNotFound(t *testing.T) {
+	_, err := ReadConfiguration("unknown-config-file.yaml")
+
+	testutil.CheckError(t, true, err)
+	if !os.IsNotExist(err) {
+		t.Error("error should say that file doesn't exist")
+	}
+}
+
+func TestReadConfigurationRemote(t *testing.T) {
+	remoteFile, teardown := testutil.ServeFile(t, []byte("remote file"))
+	defer teardown()
+
+	content, err := ReadConfiguration(remoteFile)
+
+	testutil.CheckErrorAndDeepEqual(t, false, err, []byte("remote file"), content)
+}

pkg/skaffold/util/util.go

@@ -150,30 +150,6 @@ func StringPtr(s string) *string {
 	return &o
 }
 
-func ReadConfiguration(filename string) ([]byte, error) {
-	switch {
-	case filename == "":
-		return nil, errors.New("filename not specified")
-	case filename == "-":
-		return ioutil.ReadAll(os.Stdin)
-	case IsURL(filename):
-		return Download(filename)
-	default:
-		directory := filepath.Dir(filename)
-		baseName := filepath.Base(filename)
-		if baseName != "skaffold.yaml" {
-			return ioutil.ReadFile(filename)
-		}
-		contents, err := ioutil.ReadFile(filename)
-		if err != nil {
-			logrus.Infof("Could not open skaffold.yaml: \"%s\"", err)
-			logrus.Infof("Trying to read from skaffold.yml instead")
-			return ioutil.ReadFile(filepath.Join(directory, "skaffold.yml"))
-		}
-		return contents, err
-	}
-}
-
 func IsURL(s string) bool {
 	return strings.HasPrefix(s, "http://") || strings.HasPrefix(s, "https://")
 }

pkg/skaffold/util/util_test.go

@@ -101,17 +101,6 @@ func TestExpandPathsGlob(t *testing.T) {
 	}
 }
 
-func TestDefaultConfigFilenameAlternate(t *testing.T) {
-	tmpDir, cleanup := testutil.NewTempDir(t)
-	defer cleanup()
-
-	tmpDir.Write("skaffold.yml", "foo")
-
-	content, err := ReadConfiguration(tmpDir.Path("skaffold.yaml"))
-
-	testutil.CheckErrorAndDeepEqual(t, false, err, []byte("foo"), content)
-}
-
 func TestExpand(t *testing.T) {
 	var tests = []struct {
 		description string

testutil/util.go

@@ -80,6 +80,20 @@ func CheckError(t *testing.T, shouldErr bool, err error) {
 	}
 }
 
+// CheckErrorContains checks that an error is not nil and contains
+// a given message.
+func CheckErrorContains(t *testing.T, message string, err error) {
+	t.Helper()
+	if err == nil {
+		t.Error("expected error, but returned none")
+		return
+	}
+	if !strings.Contains(err.Error(), message) {
+		t.Errorf("expected message [%s] not found in error: %s", message, err.Error())
+		return
+	}
+}
+
 // Chdir changes current directory for a test
 func Chdir(t *testing.T, dir string) func() {
 	t.Helper()