diff --git a/.nextchanges/air/7643.md b/.nextchanges/air/7643.md new file mode 100644 index 00000000000..908a8c81994 --- /dev/null +++ b/.nextchanges/air/7643.md @@ -0,0 +1 @@ +* `air run` now rejects a `code_source.include_paths` path that selects no files instead of uploading a snapshot without it; glob patterns and pathspec magic are left to git. ([#7643](https://github.com/databricks/cli/pull/7643)) diff --git a/cmd/air/snapshot_package.go b/cmd/air/snapshot_package.go index c1dc9c5f89d..152a7a4e648 100644 --- a/cmd/air/snapshot_package.go +++ b/cmd/air/snapshot_package.go @@ -8,6 +8,7 @@ import ( "io" "os" "os/exec" + "path" "path/filepath" "strings" @@ -168,6 +169,10 @@ func snapshotFiles(ctx context.Context, repoPath string, includePaths []string, if name == ".git" || strings.HasPrefix(name, ".git/") || strings.HasPrefix(base, "._") { continue } + // Pathspec magic such as ":(top)x" or ":/x" can select files above root_path. + if !filepath.IsLocal(filepath.FromSlash(name)) { + return nil, fmt.Errorf("code_source include_paths select %q, which is outside root_path %s", name, filepath.ToSlash(repoPath)) + } info, err := os.Lstat(filepath.Join(repoPath, filepath.FromSlash(name))) if errors.Is(err, os.ErrNotExist) { continue @@ -175,6 +180,12 @@ func snapshotFiles(ctx context.Context, repoPath string, includePaths []string, if err != nil { return nil, fmt.Errorf("failed to inspect snapshot path %q: %w", name, err) } + // git lists a nested repository or a submodule as a directory, which tarpack + // does not pack; keep the listing to what lands in the archive so the empty + // and include_paths checks below see the same thing. + if !info.Mode().IsRegular() && info.Mode()&os.ModeSymlink == 0 { + continue + } linkTarget := "" if info.Mode()&os.ModeSymlink != 0 { linkTarget, err = os.Readlink(filepath.Join(repoPath, filepath.FromSlash(name))) @@ -193,9 +204,45 @@ func snapshotFiles(ctx context.Context, repoPath string, includePaths []string, if len(files) == 0 { return nil, emptySnapshotError(ctx, repoPath, includePaths, isGitRepo) } + if unmatched := unmatchedIncludePaths(includePaths, files); len(unmatched) > 0 { + return nil, fmt.Errorf("code_source include_paths select no files under %s: %s. Check that these paths exist under root_path and hold files git does not ignore%s; empty directories, nested git repositories and submodules add no files", filepath.ToSlash(repoPath), strings.Join(unmatched, ", "), ignoreRuleHint(isGitRepo)) + } return files, nil } +// unmatchedIncludePaths returns the plain-path include_paths entries that select none +// of files. git ls-files drops a pathspec that matches nothing, so a mistyped entry +// would silently be missing from the snapshot. Entries git reads as patterns are left +// to git, since a path-prefix comparison cannot evaluate them. +func unmatchedIncludePaths(includePaths []string, files []snapshotFile) []string { + var unmatched []string + for _, p := range includePaths { + if isPathspecPattern(p) { + continue + } + prefix := path.Clean(filepath.ToSlash(p)) + matched := prefix == "." + for _, file := range files { + rel := filepath.ToSlash(file.rel) + if rel == prefix || strings.HasPrefix(rel, prefix+"/") { + matched = true + break + } + } + if !matched { + unmatched = append(unmatched, p) + } + } + return unmatched +} + +// isPathspecPattern reports whether git matches p as a pattern rather than as a +// literal path: pathspec magic (":!x", ":(exclude)x", ":/x", ...) or a glob. git +// treats a backslash as an escape; on Windows ToSlash turns it into a separator first. +func isPathspecPattern(p string) bool { + return strings.HasPrefix(p, ":") || strings.ContainsAny(filepath.ToSlash(p), `*?[\`) +} + // emptySnapshotError explains why no file was selected for the snapshot. An empty // tarball is accepted by the upload but fails on the node, after compute was // provisioned, so it is rejected here with the most likely cause. @@ -211,7 +258,16 @@ func emptySnapshotError(ctx context.Context, repoPath string, includePaths []str } } if len(includePaths) > 0 { - return fmt.Errorf("%s: no file under include_paths (%s) is selected. Check that the paths exist under root_path and are not ignored by git (`git check-ignore -v `)", prefix, strings.Join(includePaths, ", ")) + return fmt.Errorf("%s: no file under include_paths (%s) is selected. Check that the paths exist under root_path and hold files git does not ignore%s; empty directories, nested git repositories and submodules add no files", prefix, strings.Join(includePaths, ", "), ignoreRuleHint(isGitRepo)) + } + return fmt.Errorf("%s: the directory is empty, holds only nested git repositories or submodules, or all of its files are excluded by .gitignore rules%s. Check code_source.root_path", prefix, ignoreRuleHint(isGitRepo)) +} + +// ignoreRuleHint points at git's ignore-rule lookup, which only works inside a git +// repository. +func ignoreRuleHint(isGitRepo bool) string { + if !isGitRepo { + return "" } - return fmt.Errorf("%s: the directory is empty or all of its files are excluded by .gitignore rules (`git check-ignore -v ` shows the rule for a file). Check code_source.root_path", prefix) + return " (`git check-ignore -v ` shows the rule for a file)" } diff --git a/cmd/air/snapshot_package_test.go b/cmd/air/snapshot_package_test.go index 068997e2671..f8b6d775ac2 100644 --- a/cmd/air/snapshot_package_test.go +++ b/cmd/air/snapshot_package_test.go @@ -6,6 +6,7 @@ import ( "context" "os" "path/filepath" + "runtime" "slices" "strings" "testing" @@ -324,3 +325,136 @@ func TestSnapshotFiles_RejectsIncludePathsMatchingNothing(t *testing.T) { assert.Contains(t, err.Error(), "contains no files") assert.Contains(t, err.Error(), "include_paths") } + +func TestSnapshotFiles_RejectsIncludePathMatchingNothingAlongsideOthers(t *testing.T) { + repo := t.TempDir() + writeRepoFile(t, repo, "ray_bootstrap.sh", "#!/bin/bash\n") + writeRepoFile(t, repo, "my-job/main.py", "print(1)\n") + + _, err := snapshotFiles(t.Context(), repo, []string{"ray_bootstrap.sh", "my_job"}, false) + require.Error(t, err) + assert.Contains(t, err.Error(), "include_paths select no files") + assert.Contains(t, err.Error(), ": my_job.") + assert.NotContains(t, err.Error(), "ray_bootstrap.sh") +} + +func TestSnapshotFiles_RejectsIncludePathWhoseFilesAreIgnored(t *testing.T) { + repo := newTestRepo(t) + writeRepoFile(t, repo, ".gitignore", "build/\n") + writeRepoFile(t, repo, "main.py", "print(1)\n") + writeRepoFile(t, repo, "build/out.bin", "x") + + _, err := snapshotFiles(t.Context(), repo, []string{"main.py", "build"}, true) + require.Error(t, err) + assert.Contains(t, err.Error(), ": build.") + assert.Contains(t, err.Error(), "check-ignore -v") +} + +func TestSnapshotFiles_AcceptsIncludePathForms(t *testing.T) { + repo := t.TempDir() + writeRepoFile(t, repo, "src/model.py", "print()") + writeRepoFile(t, repo, "README.md", "readme") + writeRepoFile(t, repo, "notes.txt", "notes") + + files, err := snapshotFiles(t.Context(), repo, []string{"./src/", "README.md", "*.txt"}, false) + require.NoError(t, err) + var rels []string + for _, file := range files { + rels = append(rels, filepath.ToSlash(file.rel)) + } + assert.ElementsMatch(t, []string{"src/model.py", "README.md", "notes.txt"}, rels) +} + +func TestSnapshotFiles_IncludePathMatching(t *testing.T) { + tests := []struct { + name string + includePaths []string + want []string // selected files; nil when an error is expected + wantErr string + }{ + {name: "root", includePaths: []string{"."}, want: []string{"README.md", "b.txt", "data_v2/a.csv", "notes.txt", "src/model.py", "src/tests/test_model.py"}}, + {name: "root with slash", includePaths: []string{"./"}, want: []string{"README.md", "b.txt", "data_v2/a.csv", "notes.txt", "src/model.py", "src/tests/test_model.py"}}, + {name: "root and a file", includePaths: []string{".", "README.md"}, want: []string{"README.md", "b.txt", "data_v2/a.csv", "notes.txt", "src/model.py", "src/tests/test_model.py"}}, + {name: "question mark glob", includePaths: []string{"note?.txt"}, want: []string{"notes.txt"}}, + {name: "bracket glob", includePaths: []string{"[ab].txt"}, want: []string{"b.txt"}}, + {name: "exclude magic", includePaths: []string{"src", ":!src/tests"}, want: []string{"src/model.py"}}, + {name: "long exclude magic", includePaths: []string{"src", ":(exclude)src/tests"}, want: []string{"src/model.py"}}, + {name: "prefix of another directory", includePaths: []string{"data_v2", "data"}, wantErr: ": data."}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repo := t.TempDir() + for _, f := range []string{"README.md", "b.txt", "data_v2/a.csv", "notes.txt", "src/model.py", "src/tests/test_model.py"} { + writeRepoFile(t, repo, f, "x") + } + + files, err := snapshotFiles(t.Context(), repo, tt.includePaths, false) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), "include_paths select no files") + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + var rels []string + for _, file := range files { + rels = append(rels, filepath.ToSlash(file.rel)) + } + assert.ElementsMatch(t, tt.want, rels) + }) + } +} + +// git lists a nested repository as a single directory entry, which the tarball +// writer skips, so it must not count as a match or as a file. +func TestSnapshotFiles_NestedRepoSelectsNoFiles(t *testing.T) { + repo := t.TempDir() + writeRepoFile(t, repo, "train.py", "print(1)\n") + nested := filepath.Join(repo, "my_lib") + writeRepoFile(t, nested, "lib.py", "x = 1\n") + runGit(t, nested, "init", "-q") + + _, err := snapshotFiles(t.Context(), repo, []string{"train.py", "my_lib"}, false) + require.Error(t, err) + assert.Contains(t, err.Error(), ": my_lib.") + + _, err = snapshotFiles(t.Context(), repo, []string{"my_lib"}, false) + require.Error(t, err) + assert.Contains(t, err.Error(), "contains no files") +} + +func TestSnapshotFiles_RejectsPathspecOutsideRoot(t *testing.T) { + repo := newTestRepo(t) + writeRepoFile(t, repo, "secret/key.txt", "s") + writeRepoFile(t, repo, "proj/main.py", "print(1)\n") + commitAll(t, repo, "init") + + _, err := snapshotFiles(t.Context(), filepath.Join(repo, "proj"), []string{"main.py", ":(top)secret"}, true) + require.Error(t, err) + assert.Contains(t, err.Error(), "outside root_path") +} + +// git reads a backslash in a pathspec as an escape, so the entry is a pattern and is +// left to git. On Windows a backslash is a path separator instead. +func TestSnapshotFiles_AcceptsBackslashEscapedIncludePath(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("a backslash is a path separator on Windows") + } + repo := t.TempDir() + writeRepoFile(t, repo, "my file.txt", "x") + writeRepoFile(t, repo, "other.txt", "x") + + files, err := snapshotFiles(t.Context(), repo, []string{`my\ file.txt`}, false) + require.NoError(t, err) + require.Len(t, files, 1) + assert.Equal(t, "my file.txt", filepath.ToSlash(files[0].rel)) +} + +func TestSnapshotFiles_OmitsIgnoreRuleHintOutsideGit(t *testing.T) { + repo := t.TempDir() + writeRepoFile(t, repo, "main.py", "print(1)\n") + + _, err := snapshotFiles(t.Context(), repo, []string{"main.py", "missing"}, false) + require.Error(t, err) + assert.NotContains(t, err.Error(), "check-ignore") +}