Check if an import path is valid when it's a local path

CHANGELOG.md

@@ -7,6 +7,7 @@ All notable changes to [Earthly](https://github.com/earthly/earthly) will be doc
 ### Added
 - A warning when a `COPY` destination includes a tilde (~). Related to [#1789](https://github.com/earthly/earthly/issues/1789).
 - A hint message to suggest the usage of `-i` flag to debug the build when a RUN command fails.
+- Local IMPORT paths will be validated before they are referenced [#3470](https://github.com/earthly/earthly/issues/3470).
 
 ### Fixed
 - Limit the number of deprecation warnings when using `COMMAND` instead of `FUNCTION` keyword.

domain/importtracker.go

@@ -2,13 +2,27 @@ package domain
 
 import (
 	"fmt"
+	"os"
+	"path/filepath"
 	"strings"
 
 	"github.com/earthly/earthly/conslogging"
+	"github.com/earthly/earthly/util/hint"
 
 	"github.com/pkg/errors"
 )
 
+type pathResult int
+
+const ( // iota is reset to 0
+	notExist pathResult = iota // c0 == 0
+	file                = iota // c1 == 1
+	dir                 = iota // c2 == 2
+)
+
+// used in unit tests
+type pathResultFunc func(path string) pathResult
+
 // ImportTrackerVal is used to resolve imports
 type ImportTrackerVal struct {
 	fullPath        string
@@ -17,9 +31,10 @@ type ImportTrackerVal struct {
 
 // ImportTracker is a resolver which also takes into account imports.
 type ImportTracker struct {
-	local   map[string]ImportTrackerVal // local name -> import details
-	global  map[string]ImportTrackerVal // local name -> import details
-	console conslogging.ConsoleLogger
+	local          map[string]ImportTrackerVal // local name -> import details
+	global         map[string]ImportTrackerVal // local name -> import details
+	console        conslogging.ConsoleLogger
+	pathResultFunc pathResultFunc // to help with unit testing
 }
 
 // NewImportTracker creates a new import resolver.
@@ -29,9 +44,10 @@ func NewImportTracker(console conslogging.ConsoleLogger, global map[string]Impor
 		gi[k] = v
 	}
 	return &ImportTracker{
-		local:   make(map[string]ImportTrackerVal),
-		global:  gi,
-		console: console,
+		local:          make(map[string]ImportTrackerVal),
+		global:         gi,
+		console:        console,
+		pathResultFunc: getPathResult,
 	}
 }
 
@@ -53,7 +69,7 @@ func (ir *ImportTracker) Add(importStr string, as string, global, currentlyPrivi
 	if importStr == "" {
 		return errors.New("IMPORTing empty string not supported")
 	}
-	aTarget := fmt.Sprintf("%s+none", importStr) // form a fictional target for parasing purposes
+	aTarget := fmt.Sprintf("%s+none", importStr) // form a fictional target for parsing purposes
 	parsedImport, err := ParseTarget(aTarget)
 	if err != nil {
 		return errors.Wrapf(err, "could not parse IMPORT %s", importStr)
@@ -68,6 +84,9 @@ func (ir *ImportTracker) Add(importStr string, as string, global, currentlyPrivi
 		allowPrivileged = allowPrivileged && allowPrivilegedFlag
 	} else if parsedImport.IsLocalExternal() {
 		path = parsedImport.GetLocalPath()
+		if pathErr := verifyLocalPath(path, ir.pathResultFunc); pathErr != nil {
+			return pathErr
+		}
 		if allowPrivilegedFlag {
 			ir.console.Printf("the --allow-privileged flag has no effect when referencing a local target\n")
 		}
@@ -158,3 +177,40 @@ func (ir *ImportTracker) Deref(ref Reference) (resolvedRef Reference, allowPrivi
 	}
 	return ref, false, false, nil
 }
+
+func verifyLocalPath(path string, pathResultF pathResultFunc) error {
+	earthlyFileName := "Earthfile"
+	res := pathResultF(path)
+	if res == notExist {
+		return hint.Wrapf(errors.Errorf("path %q does not exist", path), "Verify the path %q exists", path)
+	}
+	if res == file {
+		if filepath.Base(path) == earthlyFileName {
+			return hint.Wrapf(errors.Errorf("path %q is not a directory", path), "Did you mean to import %q?", filepath.Dir(path))
+		}
+		return hint.Wrap(errors.Errorf("path %q is not a directory", path), "Please use a directory when using a local IMPORT path")
+	}
+	res = pathResultF(filepath.Join(path, earthlyFileName))
+	if res == notExist {
+		if filepath.Base(path) == earthlyFileName {
+			return hint.Wrapf(errors.Errorf("path %q does not contain an %s", path, earthlyFileName), "The path %q ends with an %q which is a directory.\nDid you mean to create an %q as a file instead?", path, earthlyFileName, earthlyFileName)
+		}
+		return hint.Wrapf(errors.Errorf("path %q does not contain an %s", path, earthlyFileName), "Verify the path %q contains an %s", path, earthlyFileName)
+	}
+	if res == dir {
+		return hint.Wrapf(errors.Errorf("path %q does contains an %s which is not a file", path, earthlyFileName), "The local IMPORT path %q contains an %q directory and not a file", path, earthlyFileName)
+	}
+	return nil
+}
+
+// getPathResult checks whether the given path does not exist, a directory or a file
+func getPathResult(path string) pathResult {
+	info, err := os.Stat(path)
+	if os.IsNotExist(err) {
+		return notExist
+	}
+	if info.IsDir() {
+		return dir
+	}
+	return file
+}

domain/importtracker_test.go

@@ -1,56 +1,143 @@
 package domain
 
 import (
+	"errors"
+	"path/filepath"
 	"testing"
 
 	"github.com/earthly/earthly/conslogging"
+	"github.com/earthly/earthly/util/hint"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestImports(t *testing.T) {
-	var tests = []struct {
+	var tests = map[string]struct {
+		importStr                   string
+		as                          string
+		ref                         string
+		expected                    string
+		expectedPathResultFuncCalls int
+		ok                          bool
+	}{
+		"remote ref no alias":                     {"github.com/foo/bar", "", "bar+abc", "github.com/foo/bar+abc", 0, true},
+		"remote ref with alias":                   {"github.com/foo/bar", "buz", "buz+abc", "github.com/foo/bar+abc", 0, true},
+		"remote ref with alias fail":              {"github.com/foo/bar", "buz", "bar+abc", "", 0, false},
+		"remote ref with version no alias":        {"github.com/foo/bar:v1.2.3", "", "bar+abc", "github.com/foo/bar:v1.2.3+abc", 0, true},
+		"remote ref with version with alias":      {"github.com/foo/bar:v1.2.3", "buz", "buz+abc", "github.com/foo/bar:v1.2.3+abc", 0, true},
+		"remote ref with version with alias fail": {"github.com/foo/bar:v1.2.3", "buz", "bar+abc", "", 0, false},
+		"local relative ref no alias":             {"./foo/bar", "", "bar+abc", "./foo/bar+abc", 2, true},
+		"local relative ref with alias":           {"./foo/bar", "buz", "buz+abc", "./foo/bar+abc", 2, true},
+		"local relative ref with alias fail":      {"./foo/bar", "buz", "bar+abc", "", 2, false},
+		"local up directory ref no alias":         {"../foo/bar", "", "bar+abc", "../foo/bar+abc", 2, true},
+		"local up directory ref with alias":       {"../foo/bar", "buz", "buz+abc", "../foo/bar+abc", 2, true},
+		"local up directory ref with alias fail":  {"../foo/bar", "buz", "bar+abc", "", 2, false},
+		"local absolute directory ref no alias":   {"/foo/bar", "", "bar+abc", "/foo/bar+abc", 2, true},
+		"local absolute directory ref with alias": {"/foo/bar", "buz", "buz+abc", "/foo/bar+abc", 2, true},
+	}
+
+	var console conslogging.ConsoleLogger
+
+	for name, tt := range tests {
+		name, tt := name, tt
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+			ir := NewImportTracker(console, nil)
+			pathResultFuncCounter := 0
+			ir.pathResultFunc = func(path string) pathResult {
+				pathResultFuncCounter++
+				if filepath.Base(path) == "Earthfile" {
+					return file
+				}
+				return dir
+			}
+			err := ir.Add(tt.importStr, tt.as, false, false, false)
+			require.NoError(t, err, "add import error")
+			require.Equalf(t, tt.expectedPathResultFuncCalls, pathResultFuncCounter, "pathResultFunc was called an unexpected number of times")
+
+			ref, err := ParseTarget(tt.ref)
+			require.NoError(t, err, "parse test case ref") // check that the test data is good
+			require.Equal(t, tt.ref, ref.String())         // sanity check
+
+			ref2, _, _, err := ir.Deref(ref)
+			if tt.ok {
+				assert.NoError(t, err, "deref import")
+				assert.Equal(t, tt.expected, ref2.StringCanonical()) // StringCanonical shows its resolved form
+				assert.Equal(t, tt.ref, ref2.String())               // String shows its import form
+			} else {
+				assert.Error(t, err, "deref should have error'd")
+			}
+		})
+	}
+}
+
+func TestImportAdd(t *testing.T) {
+	var tests = map[string]struct {
 		importStr string
-		as        string
-		ref       string
-		expected  string
-		ok        bool
+		f         pathResultFunc
+		expected  error
 	}{
-		{"github.com/foo/bar", "", "bar+abc", "github.com/foo/bar+abc", true},
-		{"github.com/foo/bar", "buz", "buz+abc", "github.com/foo/bar+abc", true},
-		{"github.com/foo/bar", "buz", "bar+abc", "", false},
-		{"github.com/foo/bar:v1.2.3", "", "bar+abc", "github.com/foo/bar:v1.2.3+abc", true},
-		{"github.com/foo/bar:v1.2.3", "buz", "buz+abc", "github.com/foo/bar:v1.2.3+abc", true},
-		{"github.com/foo/bar:v1.2.3", "buz", "bar+abc", "", false},
-		{"./foo/bar", "", "bar+abc", "./foo/bar+abc", true},
-		{"./foo/bar", "buz", "buz+abc", "./foo/bar+abc", true},
-		{"./foo/bar", "buz", "bar+abc", "", false},
-		{"../foo/bar", "", "bar+abc", "../foo/bar+abc", true},
-		{"../foo/bar", "buz", "buz+abc", "../foo/bar+abc", true},
-		{"../foo/bar", "buz", "bar+abc", "", false},
-		{"/foo/bar", "", "bar+abc", "/foo/bar+abc", true},
-		{"/foo/bar", "buz", "buz+abc", "/foo/bar+abc", true},
-		{"/foo/bar", "buz", "bar+abc", "", false},
+		"path does not exist": {
+			importStr: "./foo/bar",
+			f: func(path string) pathResult {
+				return notExist
+			},
+			expected: hint.Wrapf(errors.New(`path "./foo/bar" does not exist`), `Verify the path "./foo/bar" exists`),
+		},
+		"not a directory (ends with Earthfile)": {
+			importStr: "./foo/bar/Earthfile",
+			f: func(path string) pathResult {
+				return file
+			},
+			expected: hint.Wrap(errors.New(`path "./foo/bar/Earthfile" is not a directory`), `Did you mean to import "./foo/bar"?`),
+		},
+		"not a directory": {
+			importStr: "./foo/bar",
+			f: func(path string) pathResult {
+				return file
+			},
+			expected: hint.Wrap(errors.New(`path "./foo/bar/Earthfile" is not a directory`), "Please use a directory when using a local IMPORT path"),
+		},
+		"path ends with an Earthfile directory": {
+			importStr: "./foo/bar/Earthfile",
+			f: func(path string) pathResult {
+				if filepath.Base(filepath.Dir(path)) == "Earthfile" {
+					return dir
+				}
+				return notExist
+			},
+			expected: hint.Wrap(errors.New(`path "./foo/bar" does not contain an Earthfile`), `The path "./foo/bar" ends with an "Earthfile" which is a directory.\nDid you mean to create an "Earthfile" as a file instead?`),
+		},
+		"Earthfile does not exist": {
+			importStr: "./foo/bar",
+			f: func(path string) pathResult {
+				if filepath.Base(path) == "Earthfile" {
+					return notExist
+				}
+				return dir
+			},
+			expected: hint.Wrap(errors.New(`path "./foo/bar" does not contain an "Earthfile"`), `Verify the path "./foo/bar" contains an Earthfile`),
+		},
+		"Earthfile exists but is a directory": {
+			importStr: "./foo/bar",
+			f: func(path string) pathResult {
+				return dir
+			},
+			expected: hint.Wrap(errors.New(`path "./foo/bar" does contains an "Earthfile" which is not a file`), `The local IMPORT path "./foo/bar" contains an "Earthfile" directory and not a file`),
+		},
 	}
 
 	var console conslogging.ConsoleLogger
 
-	for _, tt := range tests {
-		ir := NewImportTracker(console, nil)
-		err := ir.Add(tt.importStr, tt.as, false, false, false)
-		assert.NoError(t, err, "add import error")
-
-		ref, err := ParseTarget(tt.ref)
-		assert.NoError(t, err, "parse test case ref") // check that the test data is good
-		assert.Equal(t, tt.ref, ref.String())         // sanity check
-
-		ref2, _, _, err := ir.Deref(ref)
-		if tt.ok {
-			assert.NoError(t, err, "deref import")
-			assert.Equal(t, tt.expected, ref2.StringCanonical()) // StringCanonical shows its resolved form
-			assert.Equal(t, tt.ref, ref2.String())               // String shows its import form
-		} else {
-			assert.Error(t, err, "deref should have error'd")
-		}
+	for name, tt := range tests {
+		name, tt := name, tt
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+			ir := NewImportTracker(console, nil)
+			ir.pathResultFunc = tt.f
+			err := ir.Add(tt.importStr, "alias", false, false, false)
+			assert.Error(t, err, "add import did not error")
+		})
 	}
 }

features/features_test.go

@@ -120,6 +120,7 @@ func TestAvailableFlags(t *testing.T) {
 		{"arg-scope-and-set", "ArgScopeSet"},
 		{"earthly-ci-runner-arg", "EarthlyCIRunnerArg"},
 		{"use-docker-ignore", "UseDockerIgnore"},
+		{"use-function-keyword", "UseFunctionKeyword"},
 	} {
 		tt := tt
 		t.Run(tt.flag, func(t *testing.T) {