From 484ad68999ad84767355fd40e9609e638e1a4795 Mon Sep 17 00:00:00 2001 From: Daniel Banck Date: Wed, 3 Nov 2021 17:43:35 +0100 Subject: [PATCH 01/10] add new IgnoreDirectoryNames setting --- docs/SETTINGS.md | 12 ++++++++++++ internal/settings/settings.go | 7 ++++--- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/docs/SETTINGS.md b/docs/SETTINGS.md index 8f3b04ac..a875c87f 100644 --- a/docs/SETTINGS.md +++ b/docs/SETTINGS.md @@ -68,6 +68,18 @@ Or if left empty This setting should be deprecated once the language server supports multiple workspaces, as this arises in VS code because a server instance is started per VS Code workspace. +## `ignoreDirectoryNames` (`[]string`) + +This allows excluding directories from being checked by the walker by passing a static list of directory names. + +The following list of directories will always be ignored: + +- `.git` +- `.idea` +- `.vscode` +- `terraform.tfstate.d` +- `.terragrunt-cache` + ## `experimentalFeatures` (object) This object contains inner settings used to opt into experimental features not yet ready to be on by default. diff --git a/internal/settings/settings.go b/internal/settings/settings.go index 5e6c03e8..9fde4406 100644 --- a/internal/settings/settings.go +++ b/internal/settings/settings.go @@ -15,9 +15,10 @@ type ExperimentalFeatures struct { type Options struct { // ModulePaths describes a list of absolute paths to modules to load - ModulePaths []string `mapstructure:"rootModulePaths"` - ExcludeModulePaths []string `mapstructure:"excludeModulePaths"` - CommandPrefix string `mapstructure:"commandPrefix"` + ModulePaths []string `mapstructure:"rootModulePaths"` + ExcludeModulePaths []string `mapstructure:"excludeModulePaths"` + CommandPrefix string `mapstructure:"commandPrefix"` + IgnoreDirectoryNames []string `mapstructure:"ignoreDirectoryNames"` // ExperimentalFeatures encapsulates experimental features users can opt into. ExperimentalFeatures ExperimentalFeatures `mapstructure:"experimentalFeatures"` From ce3ee69ca568d772863f33bf8330aba4043ec313 Mon Sep 17 00:00:00 2001 From: Daniel Banck Date: Thu, 4 Nov 2021 11:22:43 +0100 Subject: [PATCH 02/10] add test for ignoreDirectoryNames --- .../langserver/handlers/initialize_test.go | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/internal/langserver/handlers/initialize_test.go b/internal/langserver/handlers/initialize_test.go index 7c661c5e..1b54de94 100644 --- a/internal/langserver/handlers/initialize_test.go +++ b/internal/langserver/handlers/initialize_test.go @@ -2,6 +2,7 @@ package handlers import ( "fmt" + "path/filepath" "testing" "github.com/creachadair/jrpc2/code" @@ -119,3 +120,42 @@ func TestInitialize_multipleFolders(t *testing.T) { ] }`, rootDir.URI(), rootDir.URI())}) } + +func TestInitialize_ignoreDirectoryNames(t *testing.T) { + tmpDir := TempDir(t, "plugin", "ignore") + pluginDir := filepath.Join(tmpDir.Dir(), "plugin") + emptyDir := filepath.Join(tmpDir.Dir(), "ignore") + + InitPluginCache(t, pluginDir) + InitPluginCache(t, emptyDir) + + ls := langserver.NewLangServerMock(t, NewMockSession(&MockSessionInput{ + TerraformCalls: &exec.TerraformMockCalls{ + PerWorkDir: map[string][]*mock.Call{ + pluginDir: validTfMockCalls(), + emptyDir: { + // TODO? can this be empty? + { + Method: "GetExecPath", + Repeatability: 1, + ReturnArguments: []interface{}{ + "", + }, + }, + }, + }, + }})) + stop := ls.Start(t) + defer stop() + + ls.Call(t, &langserver.CallRequest{ + Method: "initialize", + ReqParams: fmt.Sprintf(`{ + "capabilities": {}, + "rootUri": %q, + "processId": 12345, + "initializationOptions": { + "ignoreDirectoryNames": [%q] + } + }`, tmpDir.URI(), "ignore")}) +} From a6602e42ef64b2cfbec145d66ad594ae1b721f31 Mon Sep 17 00:00:00 2001 From: Daniel Banck Date: Thu, 4 Nov 2021 11:23:42 +0100 Subject: [PATCH 03/10] implement ignoreDirectoryNames --- internal/langserver/handlers/initialize.go | 4 ++++ internal/terraform/module/walker.go | 16 +++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/internal/langserver/handlers/initialize.go b/internal/langserver/handlers/initialize.go index 5af34ab0..1d799355 100644 --- a/internal/langserver/handlers/initialize.go +++ b/internal/langserver/handlers/initialize.go @@ -59,6 +59,7 @@ func (svc *service) Initialize(ctx context.Context, params lsp.InitializeParams) "options.rootModulePaths": false, "options.excludeModulePaths": false, "options.commandPrefix": false, + "options.ignoreDirectoryNames": false, "options.experimentalFeatures.validateOnSave": false, "options.terraformExecPath": false, "options.terraformExecTimeout": "", @@ -123,6 +124,7 @@ func (svc *service) Initialize(ctx context.Context, params lsp.InitializeParams) properties["options.rootModulePaths"] = len(out.Options.ModulePaths) > 0 properties["options.excludeModulePaths"] = len(out.Options.ExcludeModulePaths) > 0 properties["options.commandPrefix"] = len(out.Options.CommandPrefix) > 0 + properties["options.ignoreDirectoryNames"] = len(out.Options.IgnoreDirectoryNames) > 0 properties["options.experimentalFeatures.prefillRequiredFields"] = out.Options.ExperimentalFeatures.PrefillRequiredFields properties["options.experimentalFeatures.validateOnSave"] = out.Options.ExperimentalFeatures.ValidateOnSave properties["options.terraformExecPath"] = len(out.Options.TerraformExecPath) > 0 @@ -192,6 +194,8 @@ func (svc *service) Initialize(ctx context.Context, params lsp.InitializeParams) excludeModulePaths = append(excludeModulePaths, modPath) } + // TODO? do we need to validate the input here? + svc.walker.SetIgnoreDirectoryNames(cfgOpts.IgnoreDirectoryNames) svc.walker.SetExcludeModulePaths(excludeModulePaths) svc.walker.EnqueuePath(fh.Dir()) diff --git a/internal/terraform/module/walker.go b/internal/terraform/module/walker.go index 38bbf4ff..8d26d07c 100644 --- a/internal/terraform/module/walker.go +++ b/internal/terraform/module/walker.go @@ -48,7 +48,8 @@ type Walker struct { cancelFunc context.CancelFunc doneCh <-chan struct{} - excludeModulePaths map[string]bool + excludeModulePaths map[string]bool + ignoreDirectoryNames map[string]bool } // queueCap represents channel buffer size @@ -84,6 +85,13 @@ func (w *Walker) SetExcludeModulePaths(excludeModulePaths []string) { } } +func (w *Walker) SetIgnoreDirectoryNames(ignoreDirectoryNames []string) { + w.ignoreDirectoryNames = make(map[string]bool) + for _, path := range ignoreDirectoryNames { + w.ignoreDirectoryNames[path] = true + } +} + func (w *Walker) Stop() { if w.cancelFunc != nil { w.cancelFunc() @@ -221,6 +229,11 @@ func (w *Walker) walk(ctx context.Context, rootPath string) error { return err } + if _, ok := w.ignoreDirectoryNames[info.Name()]; ok { + w.logger.Printf("skipping %s", path) + return filepath.SkipDir + } + if _, ok := w.excludeModulePaths[dir]; ok { return filepath.SkipDir } @@ -272,6 +285,7 @@ func (w *Walker) walk(ctx context.Context, rootPath string) error { return nil } + // TODO? move this to the top? combine with ignoreDirectoryNames? if isSkippableDir(info.Name()) { w.logger.Printf("skipping %s", path) return filepath.SkipDir From ec1c28927933139cf69d3e1b8ea6d8562cb2b5cf Mon Sep 17 00:00:00 2001 From: Daniel Banck Date: Thu, 4 Nov 2021 15:21:28 +0100 Subject: [PATCH 04/10] add comment about dirs in settings.md --- internal/langserver/handlers/initialize_test.go | 2 +- internal/terraform/module/walker.go | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/langserver/handlers/initialize_test.go b/internal/langserver/handlers/initialize_test.go index 1b54de94..6d4d0e20 100644 --- a/internal/langserver/handlers/initialize_test.go +++ b/internal/langserver/handlers/initialize_test.go @@ -134,7 +134,7 @@ func TestInitialize_ignoreDirectoryNames(t *testing.T) { PerWorkDir: map[string][]*mock.Call{ pluginDir: validTfMockCalls(), emptyDir: { - // TODO? can this be empty? + // TODO! improve mock and remove entry for `emptyDir` here afterwards { Method: "GetExecPath", Repeatability: 1, diff --git a/internal/terraform/module/walker.go b/internal/terraform/module/walker.go index 8d26d07c..67f09b24 100644 --- a/internal/terraform/module/walker.go +++ b/internal/terraform/module/walker.go @@ -21,6 +21,8 @@ var ( // skipDirNames represent directory names which would never contain // plugin/module cache, so it's safe to skip them during the walk + // + // please keep the list in `SETTINGS.md` in sync skipDirNames = map[string]bool{ ".git": true, ".idea": true, From 14ef331fdcb4db6149b1f4648789d5c326bc636d Mon Sep 17 00:00:00 2001 From: Daniel Banck Date: Thu, 4 Nov 2021 15:54:49 +0100 Subject: [PATCH 05/10] validate IgnoreDirectoryNames input --- internal/langserver/handlers/initialize.go | 1 - internal/settings/settings.go | 14 ++++++++ internal/settings/settings_test.go | 38 ++++++++++++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/internal/langserver/handlers/initialize.go b/internal/langserver/handlers/initialize.go index 1d799355..74520d17 100644 --- a/internal/langserver/handlers/initialize.go +++ b/internal/langserver/handlers/initialize.go @@ -194,7 +194,6 @@ func (svc *service) Initialize(ctx context.Context, params lsp.InitializeParams) excludeModulePaths = append(excludeModulePaths, modPath) } - // TODO? do we need to validate the input here? svc.walker.SetIgnoreDirectoryNames(cfgOpts.IgnoreDirectoryNames) svc.walker.SetExcludeModulePaths(excludeModulePaths) svc.walker.EnqueuePath(fh.Dir()) diff --git a/internal/settings/settings.go b/internal/settings/settings.go index 9fde4406..0bbb31f2 100644 --- a/internal/settings/settings.go +++ b/internal/settings/settings.go @@ -4,7 +4,9 @@ import ( "fmt" "os" "path/filepath" + "strings" + "github.com/hashicorp/terraform-ls/internal/terraform/datadir" "github.com/mitchellh/mapstructure" ) @@ -47,6 +49,18 @@ func (o *Options) Validate() error { } } + if len(o.IgnoreDirectoryNames) > 0 { + for _, directory := range o.IgnoreDirectoryNames { + if directory == datadir.DataDirName { + return fmt.Errorf("cannot ignore data directory %q", datadir.DataDirName) + } + + if strings.Contains(directory, string(filepath.Separator)) { + return fmt.Errorf("expected directory name, got a path: %q", directory) + } + } + } + return nil } diff --git a/internal/settings/settings_test.go b/internal/settings/settings_test.go index 6ae8b527..17365b94 100644 --- a/internal/settings/settings_test.go +++ b/internal/settings/settings_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/hashicorp/terraform-ls/internal/terraform/datadir" ) func TestDecodeOptions_nil(t *testing.T) { @@ -40,3 +41,40 @@ func TestDecodeOptions_success(t *testing.T) { t.Fatalf("options mismatch: %s", diff) } } + +func TestValidate_IgnoreDirectoryNames_error(t *testing.T) { + tables := []struct { + input string + result string + }{ + {datadir.DataDirName, `cannot ignore data directory ".terraform"`}, + {"path/path", `expected directory name, got a path: "path/path"`}, + } + + for _, table := range tables { + out, err := DecodeOptions(map[string]interface{}{ + "IgnoreDirectoryNames": []string{table.input}, + }) + if err != nil { + t.Fatal(err) + } + + result := out.Options.Validate() + if result.Error() != table.result { + t.Fatalf("expected error: %s, got: %s", table.result, result) + } + } +} +func TestValidate_IgnoreDirectoryNames_success(t *testing.T) { + out, err := DecodeOptions(map[string]interface{}{ + "IgnoreDirectoryNames": []string{"directory"}, + }) + if err != nil { + t.Fatal(err) + } + + result := out.Options.Validate() + if result != nil { + t.Fatalf("did not expect error: %s", result) + } +} From f804f0d50058a4a0c773bea80e6970e848f2d361 Mon Sep 17 00:00:00 2001 From: Daniel Banck Date: Thu, 4 Nov 2021 16:00:34 +0100 Subject: [PATCH 06/10] implement isSkippableDir as function of the walker --- internal/terraform/module/walker.go | 36 ++++++++++++----------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/internal/terraform/module/walker.go b/internal/terraform/module/walker.go index 67f09b24..7e892539 100644 --- a/internal/terraform/module/walker.go +++ b/internal/terraform/module/walker.go @@ -61,14 +61,15 @@ const queueCap = 50 func NewWalker(fs filesystem.Filesystem, modMgr ModuleManager) *Walker { return &Walker{ - fs: fs, - modMgr: modMgr, - logger: discardLogger, - walkingMu: &sync.RWMutex{}, - queue: newWalkerQueue(fs), - queueMu: &sync.Mutex{}, - pushChan: make(chan struct{}, queueCap), - doneCh: make(chan struct{}, 0), + fs: fs, + modMgr: modMgr, + logger: discardLogger, + walkingMu: &sync.RWMutex{}, + queue: newWalkerQueue(fs), + queueMu: &sync.Mutex{}, + pushChan: make(chan struct{}, queueCap), + doneCh: make(chan struct{}, 0), + ignoreDirectoryNames: skipDirNames, } } @@ -88,7 +89,6 @@ func (w *Walker) SetExcludeModulePaths(excludeModulePaths []string) { } func (w *Walker) SetIgnoreDirectoryNames(ignoreDirectoryNames []string) { - w.ignoreDirectoryNames = make(map[string]bool) for _, path := range ignoreDirectoryNames { w.ignoreDirectoryNames[path] = true } @@ -209,6 +209,11 @@ func (w *Walker) IsWalking() bool { return w.walking } +func (w *Walker) isSkippableDir(dirName string) bool { + _, ok := w.ignoreDirectoryNames[dirName] + return ok +} + func (w *Walker) walk(ctx context.Context, rootPath string) error { // We ignore the passed FS and instead read straight from OS FS // because that would require reimplementing filepath.Walk and @@ -231,7 +236,7 @@ func (w *Walker) walk(ctx context.Context, rootPath string) error { return err } - if _, ok := w.ignoreDirectoryNames[info.Name()]; ok { + if w.isSkippableDir(info.Name()) { w.logger.Printf("skipping %s", path) return filepath.SkipDir } @@ -287,19 +292,8 @@ func (w *Walker) walk(ctx context.Context, rootPath string) error { return nil } - // TODO? move this to the top? combine with ignoreDirectoryNames? - if isSkippableDir(info.Name()) { - w.logger.Printf("skipping %s", path) - return filepath.SkipDir - } - return nil }) w.logger.Printf("walking of %s finished", rootPath) return err } - -func isSkippableDir(dirName string) bool { - _, ok := skipDirNames[dirName] - return ok -} From 098b9d1e1ef96ec50965680a56d84fb3d35f4cc7 Mon Sep 17 00:00:00 2001 From: Daniel Banck Date: Thu, 4 Nov 2021 16:29:02 +0100 Subject: [PATCH 07/10] make test platform agnostic --- internal/settings/settings_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/settings/settings_test.go b/internal/settings/settings_test.go index 17365b94..44b61275 100644 --- a/internal/settings/settings_test.go +++ b/internal/settings/settings_test.go @@ -1,6 +1,8 @@ package settings import ( + "fmt" + "path/filepath" "testing" "github.com/google/go-cmp/cmp" @@ -48,7 +50,7 @@ func TestValidate_IgnoreDirectoryNames_error(t *testing.T) { result string }{ {datadir.DataDirName, `cannot ignore data directory ".terraform"`}, - {"path/path", `expected directory name, got a path: "path/path"`}, + {filepath.Join("path", "path"), fmt.Sprintf(`expected directory name, got a path: "path%spath"`, string(filepath.Separator))}, } for _, table := range tables { From 74a63e251f04cc29d36eea08cbabb63c921c5de6 Mon Sep 17 00:00:00 2001 From: Daniel Banck Date: Fri, 5 Nov 2021 15:17:35 +0100 Subject: [PATCH 08/10] fix settings test for windows --- internal/settings/settings_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/settings/settings_test.go b/internal/settings/settings_test.go index 44b61275..c29fa94c 100644 --- a/internal/settings/settings_test.go +++ b/internal/settings/settings_test.go @@ -50,7 +50,7 @@ func TestValidate_IgnoreDirectoryNames_error(t *testing.T) { result string }{ {datadir.DataDirName, `cannot ignore data directory ".terraform"`}, - {filepath.Join("path", "path"), fmt.Sprintf(`expected directory name, got a path: "path%spath"`, string(filepath.Separator))}, + {filepath.Join("path", "path"), fmt.Sprintf(`expected directory name, got a path: %q`, filepath.Join("path", "path"))}, } for _, table := range tables { From a916f0fb6ba4c611b841ef23b15e27fa369e595b Mon Sep 17 00:00:00 2001 From: Daniel Banck Date: Mon, 8 Nov 2021 09:58:03 +0100 Subject: [PATCH 09/10] Apply suggestions from code review Co-authored-by: Radek Simko --- internal/settings/settings.go | 2 +- internal/settings/settings_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/settings/settings.go b/internal/settings/settings.go index 0bbb31f2..9829ee89 100644 --- a/internal/settings/settings.go +++ b/internal/settings/settings.go @@ -52,7 +52,7 @@ func (o *Options) Validate() error { if len(o.IgnoreDirectoryNames) > 0 { for _, directory := range o.IgnoreDirectoryNames { if directory == datadir.DataDirName { - return fmt.Errorf("cannot ignore data directory %q", datadir.DataDirName) + return fmt.Errorf("cannot ignore directory %q", datadir.DataDirName) } if strings.Contains(directory, string(filepath.Separator)) { diff --git a/internal/settings/settings_test.go b/internal/settings/settings_test.go index c29fa94c..e7344908 100644 --- a/internal/settings/settings_test.go +++ b/internal/settings/settings_test.go @@ -55,7 +55,7 @@ func TestValidate_IgnoreDirectoryNames_error(t *testing.T) { for _, table := range tables { out, err := DecodeOptions(map[string]interface{}{ - "IgnoreDirectoryNames": []string{table.input}, + "ignoreDirectoryNames": []string{table.input}, }) if err != nil { t.Fatal(err) @@ -69,7 +69,7 @@ func TestValidate_IgnoreDirectoryNames_error(t *testing.T) { } func TestValidate_IgnoreDirectoryNames_success(t *testing.T) { out, err := DecodeOptions(map[string]interface{}{ - "IgnoreDirectoryNames": []string{"directory"}, + "ignoreDirectoryNames": []string{"directory"}, }) if err != nil { t.Fatal(err) From 6e712ce95bbce72d3b04a7e6cc1ad44843299607 Mon Sep 17 00:00:00 2001 From: Daniel Banck Date: Mon, 8 Nov 2021 10:25:42 +0100 Subject: [PATCH 10/10] fix test --- docs/SETTINGS.md | 2 +- internal/settings/settings_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/SETTINGS.md b/docs/SETTINGS.md index a875c87f..f3c20a6c 100644 --- a/docs/SETTINGS.md +++ b/docs/SETTINGS.md @@ -70,7 +70,7 @@ as this arises in VS code because a server instance is started per VS Code works ## `ignoreDirectoryNames` (`[]string`) -This allows excluding directories from being checked by the walker by passing a static list of directory names. +This allows excluding directories from being indexed upon initialization by passing a list of directory names. The following list of directories will always be ignored: diff --git a/internal/settings/settings_test.go b/internal/settings/settings_test.go index e7344908..9c267ac8 100644 --- a/internal/settings/settings_test.go +++ b/internal/settings/settings_test.go @@ -49,7 +49,7 @@ func TestValidate_IgnoreDirectoryNames_error(t *testing.T) { input string result string }{ - {datadir.DataDirName, `cannot ignore data directory ".terraform"`}, + {datadir.DataDirName, `cannot ignore directory ".terraform"`}, {filepath.Join("path", "path"), fmt.Sprintf(`expected directory name, got a path: %q`, filepath.Join("path", "path"))}, }