-
Notifications
You must be signed in to change notification settings - Fork 131
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
ask for init if current folder is empty root module #257
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took me so long to review this PR.
I appreciate the PR as a form of starting a discussion.
A few high-level comments/questions which came up as I was reading the code:
- I'm not fully convinced about the value of tracking "potential root modules" specifically in this context. I'd say that the root module is no different from the (existing initialized) tracked ones after the user confirms the initialization. Relatedly that recently initialized module also needs to be watched for changes.
- I understand we need to track these "candidates" somewhere until they're actually initialized and added for tracking, but I'm not sure whether expanding the responsibility of
RootModuleManager
to do this is the right approach. 🤔 It would also mean we'd need to integrate the watcher there somehow. So maybe it's better to keep this logic apart in some form ofRootModuleInitializer
? - We're in the process of integrating
hashicorp/terraform-exec
which may change how we call Terraform a little bit internally. - As part of Validate configuration and publish diagnostics #27 we'll need to enable clients also to run certain commands, and that would likely include
terraform init
. I'm not saying that it should be a blocker for fixing Ask toterraform init
an uninitialized workspace #106 or that we need to call init the exact same way as clients would, but it's something to keep in mind.
@radeksimko @appilon I have updated this PR, could you have a look when free? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry it took me a while to get to this PR, but a colleague of mine is working on a similar feature on VS Code side and we needed to sync up and make sure that we don't introduce something conflicting.
The PR looks good in general - Frankly I think the UX around modules will go through many changes in the near future as we get it more accurate and relevant, but since your PR just makes the existing message more actionable and still leaves user the choice I think there is no harm in merging it, once we address the in-line comments.
@radeksimko thanks for your suggestions. I have updated this PR. |
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. |
fix #106.
there are some points that have not been considered. such as if current dir is in
IncludeRootModule
orExcludeRootModule