Skip to content
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

tk tool importers: Handle deleted files #872

Merged
merged 2 commits into from
Jul 5, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion cmd/tk/tool.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,10 @@ func importsCmd() *cli.Command {

func importersCmd() *cli.Command {
cmd := &cli.Command{
Use: "importers <file> <file...>",
Use: "importers <file> <file...> <deleted:file...>",
Short: "list all environments that either directly or transitively import the given files",
Long: `list all environments that either directly or transitively import the given files
If the file being looked up was deleted, it should be prefixed with "deleted:".
As optimization,
if the file is not a vendored (located at <tk-root>/vendor/) or a lib file (located at <tk-root>/lib/), we assume:
Expand All @@ -162,6 +163,9 @@ if the file is not a vendored (located at <tk-root>/vendor/) or a lib file (loca
}

for _, f := range args {
if strings.HasPrefix(f, "deleted:") {
continue
}
if _, err := os.Stat(f); os.IsNotExist(err) {
return fmt.Errorf("file %q does not exist", f)
}
Expand Down
50 changes: 43 additions & 7 deletions pkg/jsonnet/find_importers.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package jsonnet

import (
"fmt"
"os"
"path/filepath"
"sort"
Expand Down Expand Up @@ -34,12 +35,33 @@ func FindImporterForFiles(root string, files []string) ([]string, error) {

importers := map[string]struct{}{}

if files, err = expandSymlinksInFiles(root, files); err != nil {
// Handle files prefixed with `deleted:`. They need to be made absolute and we shouldn't try to find symlinks for them
var filesToCheck, existingFiles []string
for _, file := range files {
if strings.HasPrefix(file, "deleted:") {
file = strings.TrimPrefix(file, "deleted:")
julienduchesne marked this conversation as resolved.
Show resolved Hide resolved
// Try with both the absolute path and the path relative to the root
if !filepath.IsAbs(file) {
absFilePath, err := filepath.Abs(file)
if err != nil {
return nil, err
}
filesToCheck = append(filesToCheck, absFilePath)
filesToCheck = append(filesToCheck, filepath.Clean(filepath.Join(root, file)))
}
continue
}

existingFiles = append(existingFiles, file)
}

if existingFiles, err = expandSymlinksInFiles(root, existingFiles); err != nil {
return nil, err
}
filesToCheck = append(filesToCheck, existingFiles...)

// Loop through all given files and add their importers to the list
for _, file := range files {
for _, file := range filesToCheck {
if filepath.Base(file) == jpath.DefaultEntrypoint {
importers[file] = struct{}{}
}
Expand Down Expand Up @@ -171,15 +193,14 @@ func findImporters(root string, searchForFile string, chain map[string]struct{})
searchedFileIsLibOrVendored := isFileLibOrVendored(searchForFile)
if !searchedFileIsLibOrVendored {
searchedDir := filepath.Dir(searchForFile)
searchedFileEntrypoint, err := jpath.Entrypoint(searchedDir)
if err == nil && searchedFileEntrypoint != "" {
if entrypoint := findEntrypoint(searchedDir); entrypoint != "" {
// Found the main file for the searched file, add it as an importer
importers = append(importers, searchedFileEntrypoint)
} else {
importers = append(importers, entrypoint)
} else if _, err := os.Stat(searchedDir); err == nil {
// No main file found, add all main files in child dirs as importers
files, err := FindFiles(searchedDir, nil)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to find files in %s: %w", searchedDir, err)
}
for _, file := range files {
if filepath.Base(file) == jpath.DefaultEntrypoint {
Expand Down Expand Up @@ -317,6 +338,21 @@ func createJsonnetFileCache(root string) (map[string]*cachedJsonnetFile, error)
return jsonnetFilesCache[root], nil
}

// findEntrypoint finds the nearest main.jsonnet file in the given file's directory or parent directories
func findEntrypoint(searchedDir string) string {
for {
if _, err := os.Stat(searchedDir); err == nil {
break
}
searchedDir = filepath.Dir(searchedDir)
}
searchedFileEntrypoint, err := jpath.Entrypoint(searchedDir)
if err != nil {
return ""
}
return searchedFileEntrypoint
}

func pathMatches(path1, path2 string) bool {
if path1 == path2 {
return true
Expand Down
59 changes: 59 additions & 0 deletions pkg/jsonnet/find_importers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package jsonnet
import (
"fmt"
"path/filepath"
"strings"
"testing"

"github.com/grafana/tanka/pkg/jsonnet/jpath"
Expand Down Expand Up @@ -143,6 +144,59 @@ func findImportersTestCases(t testing.TB) []findImportersTestCase {
absPath(t, "testdata/findImporters/environments/lib-import-relative-to-env/folder1/folder2/main.jsonnet"),
},
},
{
name: "unused deleted file",
files: []string{
"deleted:testdata/findImporters/vendor/deleted-vendor/main.libsonnet",
},
expectedImporters: nil,
},
{
name: "deleted local path that is still potentially imported",
files: []string{
"deleted:testdata/findImporters/environments/using-deleted-stuff/my-import-dir/main.libsonnet",
},
expectedImporters: []string{
absPath(t, "testdata/findImporters/environments/using-deleted-stuff/main.jsonnet"),
},
},
{
name: "deleted lib that is still potentially imported",
files: []string{
"deleted:testdata/findImporters/lib/my-import-dir/main.libsonnet",
},
expectedImporters: []string{
absPath(t, "testdata/findImporters/environments/using-deleted-stuff/main.jsonnet"),
},
},
{
name: "deleted vendor that is still potentially imported",
files: []string{
"deleted:testdata/findImporters/vendor/my-import-dir/main.libsonnet",
},
expectedImporters: []string{
absPath(t, "testdata/findImporters/environments/using-deleted-stuff/main.jsonnet"),
},
},
{
name: "deleted lib that is still potentially imported, relative path from root",
files: []string{
"deleted:lib/my-import-dir/main.libsonnet",
},
expectedImporters: []string{
absPath(t, "testdata/findImporters/environments/using-deleted-stuff/main.jsonnet"),
},
},
{
// All files in an environment are considered to be imported by the main file, so the same should apply for deleted files
name: "deleted dir in environment",
files: []string{
"deleted:testdata/findImporters/environments/no-imports/deleted-dir/deleted-file.libsonnet",
},
expectedImporters: []string{
absPath(t, "testdata/findImporters/environments/no-imports/main.jsonnet"),
},
},
}
}

Expand All @@ -154,6 +208,11 @@ func TestFindImportersForFiles(t *testing.T) {
require.NoError(t, err)
require.NotEmpty(t, files)
for _, file := range files {
// This project is known to be invalid (as the name suggests)
if strings.Contains(file, "using-deleted-stuff") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be part of findImportersTestCase? That way you can have next to the test information if you should skip here or rather, expect an error or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I don't think so. It's a sanity check. The command to find importers does not eval jsonnet (because that would be too expensive), and for that that reason, it does not necessarily throw errors on invalid jsonnet (it's not its purpose). So this eval is there to check that the test cases are actually valid

continue
}

// Skip non-main files
if filepath.Base(file) != jpath.DefaultEntrypoint {
continue
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
myimport: import 'my-import-dir/main.libsonnet',
}