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.

Sign up for GitHub

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

Jump to bottom

Module View Command Handler #632

Merged
merged 1 commit into from
Sep 23, 2021
Merged

Module View Command Handler #632

merged 1 commit into from
Sep 23, 2021

Conversation

jpogran
Copy link
Contributor

@jpogran jpogran commented Aug 27, 2021 •
edited
Loading

Adds a handler to return a list of modules in the current workspace to display client side.

Completes: hashicorp/vscode-terraform#746

@jpogran jpogran self-assigned this Aug 27, 2021
@jpogran jpogran force-pushed the module-view-command branch 3 times, most recently from 9bbe291 to f6144e5 Compare August 31, 2021 18:54
@jpogran jpogran mentioned this pull request Aug 31, 2021
Merged
6 tasks
@jpogran jpogran force-pushed the module-view-command branch 2 times, most recently from f1ce72c to 5bf868b Compare September 2, 2021 20:40
@jpogran jpogran force-pushed the module-view-command branch 3 times, most recently from 3cfd5e1 to 74e37de Compare September 13, 2021 23:18
@jpogran jpogran added editor/vscode https://code.visualstudio.com/ enhancement New feature or request modules Functionality related to the module block and modules generally labels Sep 13, 2021
@jpogran jpogran marked this pull request as ready for review September 14, 2021 02:40
@radeksimko radeksimko removed the editor/vscode https://code.visualstudio.com/ label Sep 14, 2021
@radeksimko radeksimko self-requested a review September 15, 2021 10:46
radeksimko
radeksimko reviewed Sep 15, 2021
View reviewed changes
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

This is a good start and works for some simple examples.

Aside from my in-line comments/suggestions we'll also need an E2E test for the new command.

Do let me know if you wish to talk through any particular comment, I'm happy to do it more synchronously via Zoom.

internal/langserver/handlers/command/module_list.go Outdated Show resolved Hide resolved
internal/langserver/handlers/command/module_list.go Outdated Show resolved Hide resolved
internal/langserver/handlers/command/module_list.go Outdated Show resolved Hide resolved
internal/langserver/handlers/command/module_list.go Outdated Show resolved Hide resolved
internal/langserver/handlers/command/module_list.go Outdated Show resolved Hide resolved
internal/langserver/handlers/command/module_list.go Outdated Show resolved Hide resolved
internal/langserver/handlers/command/module_list.go Outdated Show resolved Hide resolved
internal/langserver/handlers/command/module_list.go Outdated Show resolved Hide resolved
internal/langserver/handlers/execute_command.go Outdated Show resolved Hide resolved
internal/langserver/handlers/command/module_list.go Outdated Show resolved Hide resolved
@jpogran jpogran force-pushed the module-view-command branch from e41c880 to d0e9de9 Compare September 22, 2021 16:56
@jpogran jpogran requested a review from radeksimko September 22, 2021 17:08
radeksimko
radeksimko reviewed Sep 23, 2021
View reviewed changes
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Thank you for making these updates, I think it's pretty close to a merge-able state!

I left you some more in-line comments, majority of which are relatively minor/stylistic.

As always - do let me know if you wish to discuss any particular comment/feedback more synchronously via Zoom, I'm happy to do it!

internal/langserver/handlers/command/module_list.go Outdated Show resolved Hide resolved
internal/terraform/datadir/module_types.go Outdated Show resolved Hide resolved
internal/langserver/handlers/command/module_calls.go Outdated Show resolved Hide resolved
internal/langserver/handlers/command/module_calls.go Outdated Show resolved Hide resolved
internal/langserver/handlers/command/module_calls.go Outdated Show resolved Hide resolved
internal/terraform/datadir/module_types.go Show resolved Hide resolved
internal/terraform/datadir/module_types.go Outdated Show resolved Hide resolved
internal/terraform/datadir/module_types.go Outdated Show resolved Hide resolved
internal/terraform/datadir/module_types.go Show resolved Hide resolved
internal/langserver/handlers/command/module_calls_test.go Outdated Show resolved Hide resolved
@jpogran jpogran force-pushed the module-view-command branch 2 times, most recently from 0b44056 to 2dff66d Compare September 23, 2021 13:52
@jpogran jpogran mentioned this pull request Sep 23, 2021
Closed
@jpogran jpogran force-pushed the module-view-command branch from 2dff66d to c867e80 Compare September 23, 2021 14:22
@jpogran jpogran requested a review from radeksimko September 23, 2021 14:51
radeksimko
radeksimko approved these changes Sep 23, 2021
View reviewed changes
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

🚢 🚀

internal/langserver/handlers/command/module_calls_test.go Outdated Show resolved Hide resolved
internal/terraform/datadir/module_types.go Show resolved Hide resolved
@jpogran jpogran force-pushed the module-view-command branch 3 times, most recently from 27881fb to 4b0b154 Compare September 23, 2021 16:22
@jpogran
Module View Command Handler
4b0b154 
Adds a handler to return a list of modules used by a given module path. Each module returned shows the module name, version, documentation link and what type of module it is (local, github, or terraform registry). It also detects if a module has nested modules, and adds them in the `DependentModules` property.

This is meant to be used in tandem with hashicorp/vscode-terraform#746

TODO: It is technically incorrect to use the package hashicorp/terraform-registry-address here as it is written to parse Terraform provider addresses and may not work correctly on Terraform module addresses. The proper approach is to create a new parsing library that is dedicated to parsing these kinds of addresses correctly, by re-using the logic defined in the authorative source: hashicorp/terraform/internal/addrs/module_source.go. However this works enough for now to identify module types for display in vscode-terraform.

This also increases the CI timeout for the build process. We think the introduction of go-getter inflated the dependency tree and adds more time. At the moment it is more economical to eat the build time than take the engineering time to copy the methods needed to detect the correct module sources.
@jpogran jpogran merged commit d202fd5 into main Sep 23, 2021
@jpogran jpogran deleted the module-view-command branch September 23, 2021 16:41
@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 Oct 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@radeksimko radeksimko radeksimko approved these changes

Assignees

@jpogran jpogran

Labels
enhancement New feature or request modules Functionality related to the module block and modules generally
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jpogran @radeksimko