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

refactoring: Introduce root module (manager) abstraction #167

Merged
merged 1 commit into from
Jun 18, 2020

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Jun 16, 2020

Sorry for the bigger PR! 😅


The main theme of the refactoring is to decouple a few concepts:

  • watcher
  • schema storage
  • terraform executable discovery
  • terraform executor

and introduce a new concept of root module manager which better reflects the reality where each root module (directory) can have its own schema/plugin cache, its own terraform version/executable and its own module cache.

This also introduces module cache parser and implements cache invalidation when the module cache changes.

This doesn't yet solve, but introduces a more solid foundation for solving the following issues:

The immediate end-user effect of this PR is the following:

  • if an initd directory is opened as root by client, the server obtains the schema and makes it available for any referenced local modules
  • if client opens any referenced module, it gets schema data from the root module
  • if client opens module which is not referenced, the completion errors out with "finding compatible parser failed: root module not found for .../nested-path"

Technically this scenario seemed to work before too, but that was rather a side effect of the implementation where we had a single global plugin/schema cache and we would (wrongly) also provide this data to modules which were not referenced. So this PR effectively makes it unavailable for modules which are not actually referenced.

The aim of this initial refactoring PR is to keep the functional changes minimal, but follow-up with more smaller PRs afterwards which can:

  • make initialization of plugin cache optional (that will require some changes in langserver/handlers mocks - we'll probably need to add some real test data in form of folders with .terraform)
  • walk the hierarchy to reflect monorepo with multiple root modules
  • decide how to best match these multiple schema/plugin caches with module paths

go.sum Show resolved Hide resolved
@paultyng
Copy link
Contributor

Just a general observation as I read through, you may want to append an _test to all the mock files, to ensure they aren't included at runtime.

)

type workspace struct {
ctx context.Context
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like an issue to attach a context to a struct, you shouldn't need to

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for the persistence here is to allow watcher to ask for schema refresh without having to pass its own context. The watcher remains global (to help centralising paths we need to watch across the whole hierarchy) so it is designed to outlive a workspace, but operations on the workspace should be cancellable when e.g. workspace cache is removed (not possible today, but I don't see why not implement it later).

Copy link
Member Author

Choose a reason for hiding this comment

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

That said I don't like persisting it either, I just couldn't come up with a better solution.

@radeksimko
Copy link
Member Author

Just a general observation as I read through, you may want to append an _test to all the mock files, to ensure they aren't included at runtime.

I was thinking about it ☝️ , but then I also realized that the mock logic needs to be usable outside of the package and AFAIK anything in tests isn't exported. On the positive other side none of these mock functions/structs import testing, so they should be harmless to runtime.

@radeksimko radeksimko force-pushed the f-local-modules branch 6 times, most recently from dc0aa44 to 394b517 Compare June 18, 2020 10:27
@radeksimko radeksimko changed the title refactoring: Introduce workspace (manager) abstraction refactoring: Introduce root module (manager) abstraction Jun 18, 2020
@radeksimko radeksimko marked this pull request as ready for review June 18, 2020 10:30
Copy link
Contributor

@paultyng paultyng left a comment

Choose a reason for hiding this comment

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

One additional minor comment

The main theme of the refactoring is to decouple a few concepts:
 - watcher
 - schema storage
 - terraform executable discovery
 - terraform executor

and introduce a new concept of root module manager
which better reflects the reality of monorepos etc.
@ghost
Copy link

ghost commented Jul 18, 2020

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the context necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jul 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants