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

walker: Index uninitialized modules #997

Merged
merged 3 commits into from
Jul 13, 2022
Merged

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Jul 8, 2022

Fixes #724

Depends on hashicorp/terraform-schema#126


The PR changes the walker such that it indexes any directory it finds with >0 *.tf files in it. This has a few side effects I had to account for:

  • as a result of indexing many more modules, this uncovered a race condition where reference collection would run before metadata decoding, resulting in no references being collected.
    • Reference collection jobs were moved under Defer to provide that guarantee.
  • Some E2E tests assume that the existence of .terraform dir without other files would trigger indexing, but the new behaviour is more correct in not indexing such a directory. The reason we start with empty directory with just .terraform is because it means we can more easily provide the actual test configuration as part of the test via didOpen and we don't have to deal with copying/creating test configs in FS.
    • The same function responsible for "bootstrapping" a directory to become indexable was updated to also create empty.tf
    • We could update all these E2E tests such that they don't rely on walker at all - i.e. we don't do any indexing on initialize, but that would also mean we have to find a different way of loading the schema, e.g. by inserting it into memdb as part of each test.
    • It is questionable whether these tests are in fact truly E2E when they mock out the Terraform CLI calls. I think the best solution might be having two versions of most tests, where (1) is truly E2E and installs and executes real Terraform CLI, and (2) mocks out enough memdb state such that tests can run offline
    • We so far used many E2E tests as a way to test the walker, so we'd have to also add some unit tests to the walker if we no longer run it as part of the E2E tests.

Indexing Before

New Indexer (now vs proposed)@2x

Indexing After

New Indexer (now vs proposed)@2x (3)


I filed a few follow-up issues to deal with the potential performance hit resulting from indexing more modules:

@radeksimko radeksimko added the enhancement New feature or request label Jul 8, 2022
@radeksimko radeksimko assigned radeksimko and unassigned radeksimko Jul 8, 2022
@radeksimko radeksimko added this to the v0.29.0 milestone Jul 8, 2022
@radeksimko radeksimko force-pushed the f-uninitialized-modules branch 5 times, most recently from 0ff6d0c to 25b160f Compare July 11, 2022 13:22
@radeksimko radeksimko marked this pull request as ready for review July 11, 2022 14:30
@radeksimko radeksimko requested a review from a team as a code owner July 11, 2022 14:30
@radeksimko radeksimko marked this pull request as draft July 12, 2022 08:21
@radeksimko radeksimko changed the title Index uninitialized modules walker: Index uninitialized modules Jul 12, 2022
@radeksimko radeksimko force-pushed the f-uninitialized-modules branch 2 times, most recently from a4f6e82 to 1297b5c Compare July 12, 2022 11:01
@radeksimko radeksimko marked this pull request as ready for review July 12, 2022 11:29
Replacing `filepath.Walk` with `fs.WalkDir`:

 - makes testing of the walker easier with `testing/fstest`, or any other `fs.FS` compatible libraries.
 - makes walker aligned with the FS interpretation of the rest of the codebase in that open documents are served from in-memory cache, not from disk
 - allows for slightly more readable code
This was actually required since the introduction of io/fs or `os.Readdir()` into filesystem package (introduced only in 1.16), so this is just retrospective.
https://pkg.go.dev/io/fs
https://pkg.go.dev/os#ReadDir
@radeksimko radeksimko force-pushed the f-uninitialized-modules branch 3 times, most recently from 3ebb8e0 to 568af4d Compare July 12, 2022 15:59
Checking MetaState in (ModuleStore).LocalModuleMeta prevents a possible race condition where terraform-schema would otherwise build out module schema just because the module entry is present, but metadata was not parsed yet.
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

Nice work! This is really fast & fun to use.

I tried different numbers of modules and couldn't see any performance impact

@radeksimko radeksimko merged commit c842acf into main Jul 13, 2022
@radeksimko radeksimko deleted the f-uninitialized-modules branch July 13, 2022 12:01
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Index uninitalized modules
2 participants