Use terraform-exec #268
Use terraform-exec #268
Conversation
Move exec tests to exec_mock_test.go since they don't use the concrete implementation.
internal/terraform/exec/exec.go
Outdated
| } | ||
| tf.SetLogger(e.logger) | ||
|
|
||
| // TODO: make sense of how this works |
There was a problem hiding this comment.
Still need to reason about how this worked
| version := strings.TrimPrefix(lines[0], "Terraform v") | ||
|
|
||
| return version, nil | ||
| // TODO: consider refactoring codebase to work directly with go-version.Version |
There was a problem hiding this comment.
I noticed a fair bit of back and forth between go-version.Version and the stringified representation. I think given I saw go-version types bled onto a few structs throughout the codebade, and given terraform-exec works with it directly it may be worth changing the language server codebase to work directly in that type, in a follow up PR.
| return formatterForVersion(v, m.Format) | ||
| } | ||
|
|
||
| func (m *mockExecutor) run(ctx context.Context, args ...string) ([]byte, error) { |
There was a problem hiding this comment.
the mockExecutor is essentially a stripped down copy of the previous "melded" implementation we had before
| @@ -13,3 +17,82 @@ func TestMain(m *testing.M) { | |||
|
|
|||
| os.Exit(m.Run()) | |||
| } | |||
|
|
|||
| func TestExec_timeout(t *testing.T) { | |||
There was a problem hiding this comment.
These tests were moved out of exec_test.go because I felt it was just testing the mock.
There was a problem hiding this comment.
yeah i think you can just drop them
| @@ -210,65 +112,30 @@ func (e *Executor) FormatterForVersion(v string) (Formatter, error) { | |||
| } | |||
|
|
|||
| if ver.GreaterThanOrEqual(fmtCapableVersion) { | |||
There was a problem hiding this comment.
You could move this version compatibility check to terraform-exec if you wanted.
There was a problem hiding this comment.
this is something @radeksimko upstreaming now. Depending on the outcome of the upstreaming work, this PR may be superseded.
| return &schemas, nil | ||
| func contextWithTimeout(ctx context.Context, timeout time.Duration) (context.Context, context.CancelFunc) { | ||
| cancel := func() {} | ||
| if timeout > 0 { |
There was a problem hiding this comment.
should it just be a WithCancel if its zero?
|
Just to summarize what we discussed via Zoom today:
|
|
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. |
ghost
locked as resolved and limited conversation to collaborators
Will bump terraform-exec to proper release before merging, but ready for review. Previously we maintained an "executor" library internally, the implementation had a nice trick that allowed it to become mocked for unit testing. While switching to terraform-exec was relatively easy, decoupling the built in mock required some refactoring.
This represents the least obtrusive drop in replacement of terraform-exec I could think of. With further refactoring we could likely remove the wrapping
Executorstruct. This refactor tried to strictly follow placing the responsibility of interface definitions on the consuming packages, which allows the producing packages to work in concrete types, and thus not forcing 1 large monolithic interface just for the sake of a mock to build up, albeit the interface created inrootmodulecovers most of the surface area.Closes #266