Type check variables between modules #6185
Conversation
This test demonstrates a problem where the types to a module input are not checked. For example, if a module - inner - defines a variable "should_be_a_map" as a map, or with a default variable of map, we do not fail if the user sets the variable value in the outer module to a string value.
This adds a test where the assignment is in a nested module rather than the root.
| "github.com/mitchellh/mapstructure" | ||
| ) | ||
|
|
||
| type EvalTypeCheckVariable struct { |
There was a problem hiding this comment.
Would you mind doing a prose comment block summarizing the intended use case and context of the EvalNode? I've found these to be helpful when navigating unfamiliar EvalTrees, and the name is often not enough to fully get what the node is supposed to do.
There was a problem hiding this comment.
Yes that seems like a worthwhile effort - I think several others can be documented based on the research that went into this PR too!
jen20
force-pushed
the
b-type-checking-context
branch
from
516b04c
to
8b9896e
Compare
| // | ||
| // Currently since the type system is simple, we currently do not make | ||
| // use of the values since it is only valid to pass string values. The | ||
| // structure is in place for extension of the type system, however. |
|
LGTM! |
These tests demonstrates a problem where the types to a module input are not checked. For example, if a module - inner - defines a variable "should_be_a_map" as a map, or with a default variable of map, we do not fail if the user sets the variable value in the outer module to a string value. This is also a problem in nested modules. The implementation changes add a type checking step into the graph evaluation process to ensure invalid types are not passed.
Until hashicorp#6230 is addressed. This breaks certain TF modules in use at PayByPhone, so until there is some direction on what needs to be done here I'd rather just have it work like it did before. This reverts commit d7d3970.
|
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
locked and limited conversation to collaborators
This patch makes the tests pass - we expect an error if a string value is assigned to a variable which should be a map. This will need extending for lists/maps etc but is still relevant for the next point release, hence targeting master for now.