-
Notifications
You must be signed in to change notification settings - Fork 397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check if an import path is valid when it's a local path #3592
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this work when this is in a remote context? E.g. I call the target There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting... it might be nice to throw an error for that case too. But that could be a separate PR. |
||
|
||
// 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 | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The diff isn't great here, it might be easier to view commit by commit: |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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") | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unrelated, but forgotten in previous PR. |
||
} { | ||
tt := tt | ||
t.Run(tt.flag, func(t *testing.T) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.