Skip to content

terraform console: enable use of impure functions#25442

Merged
mildwonkey merged 2 commits intomasterfrom
mildwonkey/impure-funcs-in-console
Jul 1, 2020
Merged

terraform console: enable use of impure functions#25442
mildwonkey merged 2 commits intomasterfrom
mildwonkey/impure-funcs-in-console

Conversation

@mildwonkey
Copy link
Copy Markdown
Contributor

Allow impure funcs in walkEval, which is used by terraform console.

I wrote a bit of a unit test for this, but it was so very simplistic that it did not seem worth doing (the test seemed more likely to give a false sense of coverage). I will happily do it anyway, or take other suggestions!

I am also considering what additional documentation might be useful here, if any. I started writing a note about impure functions in the console documentation to warn users about impure functions, but it felt pretty awkward. I'm inclined to skip it for now unless we start getting questions, but I can go back to that if y'all think it's something we should add now.

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 30, 2020

Codecov Report

Merging #25442 into master will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
terraform/evaluate.go 54.36% <100.00%> (+0.44%) ⬆️
dag/marshal.go 54.44% <0.00%> (+1.11%) ⬆️
terraform/node_resource_plan.go 93.44% <0.00%> (+1.63%) ⬆️
terraform/context.go 86.00% <0.00%> (+6.00%) ⬆️
terraform/graph_builder_eval.go 100.00% <0.00%> (+100.00%) ⬆️
terraform/node_provider_eval.go 100.00% <0.00%> (+100.00%) ⬆️

@mildwonkey mildwonkey requested a review from a team June 30, 2020 20:09
Copy link
Copy Markdown
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

This looks good to me!

The sort of testing I might suggest here is testing of the sort we have in the context_*_test.go files... perhaps we could add a new context_eval_test.go file and have it mimic how terraform console calls ctx.Eval, and then get an evaluation scope out of it the same way as terraform console does and run a table of "this expression in, that result out" tests against it so we can verify that it's able to access the sorts of things we expect console to be able to access: these functions, local values defined in the configuration test fixture, and maybe some other bits and bobs if they are easy to write without blowing up the scope of what you were trying to solve here.

@mildwonkey mildwonkey force-pushed the mildwonkey/impure-funcs-in-console branch from bc2e0d6 to e6d47ff Compare July 1, 2020 13:34
@mildwonkey mildwonkey merged commit f3a1f1a into master Jul 1, 2020
@mildwonkey mildwonkey deleted the mildwonkey/impure-funcs-in-console branch July 1, 2020 13:43
@ghost
Copy link
Copy Markdown

ghost commented Aug 1, 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 details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Aug 1, 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