Conversation
The new init graph builder creates a small graph that can be used to install or load module configuration. It reuses different walkers to either install modules or validate the manifest during configuration loading. The new module install node dynamically expands the graph after module installation with a subgraph for the installed module.
Instead of evaluating and parsing a module source and version on configuration loading, we now simply store the expression. Decoding is now done during the graph-based configuration loading in the module install node.
The init command and the module installer are now using the new graph-based workflow to install modules instead of using the recursive BuildConfig.
We previously used a loader -> BuildConfig flow to load configuration. This commit changes most (but not all yet) flows to use the new graph-based approach. Instead of simply recursively loading the modules, we now need to take a stepped approach: 1. Load the root module 2. Collect the variables and their values 3. Build the configuration with the graph-based approach Because this approach relies on different parts from different packages, it can't easliy be done within the `configload` package. So, now we do most of in the backend or command.
Since init only really cares about references that are used inside a module source (or version), we can treat all other references as dynamic/unknown and shortcut most of their validation steps.
This turns the string representation of the source and version attribute
of a module call into an expression. This leads to a change in the JSON
output:
`"source": "./foo"` --> `"source": {"constant_value": "./foo"},`
To be able to read and build the configuration in planfile related tests, we need methods from the `terraform` package. This commit moves the test into that package to make testing easier.
This commit moves some code around to fix configuration loading during the (legacy) import command. And add vars to the show command.
To be able to show version constraint from modules, we now store them during configuration loading.
36ea945 to
8b36a34
Compare
|
The equivalence tests will be updated. Please verify the changes here. |
|
The equivalence tests will be updated. Please verify the changes here. |
|
The equivalence tests will be updated. Please verify the changes here. |
mildwonkey
left a comment
There was a problem hiding this comment.
This is great! I checked it out and messed around with the new feature, and users are going to love it.
I left some hugely nitpicky comments about wording in error messages. These do not matter all that much and are not blockers. Address the ones you agree with, or ignore them, up to you. 😁
I am a bit unsure about the json change - I don't think it's a blocker, but we might need to coordinate with some other folks. I'm guessing the json output format version has never changed; the idea was that we'd bump that every time the schema changed so that the parsers knew what they were parsing, but I wasn't here long after 1.0 so I'm not sure what they ended up doing instead (just checking the terraform version, maybe?). Can https://github.com/hashicorp/terraform-json parse the new planfile?
I don't feel like I can approve this until we've (we = the terraform team, you don't need me specifically) discussed the json change, but this looks overall reasonable to me.
Thanks for the video tour + very good commit-by-commit breakdown, that's a lot of work!
| MockedCalls map[string]*configs.Module | ||
| } | ||
|
|
||
| func (m *MockModuleWalker) LoadModule(req *configs.ModuleRequest) (*configs.Module, *version.Version, hcl.Diagnostics) { |
There was a problem hiding this comment.
you don't need to change this, but a mild suggestion for the future: If you're going to build a mock implementation of an interface, it might as well be in it's own file (and near the interface definition) so it's easier for other developers to find and re-use!
| return tfdiags.Diagnostics{}.Append(&hcl.Diagnostic{ | ||
| Severity: hcl.DiagError, | ||
| Summary: `Invalid module source`, | ||
| Detail: `The module source can only reference input variables and local values.`, |
There was a problem hiding this comment.
🤔 This error message might be confusing for users when the for_each is directly referencing a const var - the below config results in the same error message.
variable "name" {
type = set(string)
const = true
default = ["hello","world"]
}
module "example" {
for_each = var.name
source = "terraform-iaac/${each.key}/kubernetes"
}
| diags = diags.Append(&hcl.Diagnostic{ | ||
| Severity: hcl.DiagError, | ||
| Summary: "Invalid module source", | ||
| Detail: "The module source can only reference input variables and local values.", |
There was a problem hiding this comment.
Should this message be more specific? const variables maybe? I ask because you can have non-const input variables, and also you can set defaults for consts, at which point they might technically be "input variables" but I don't peresonally think of them as such. It seems like it would be confusing to a user who used non-const variables but did set them on the command line.
| diags = diags.Append(&hcl.Diagnostic{ | ||
| Severity: hcl.DiagError, | ||
| Summary: "Invalid module version", | ||
| Detail: "The module version can only reference input variables and local values.", |
There was a problem hiding this comment.
(same ? about phrasing)
| if op == walkInit && n.Config.Const && !val.IsWhollyKnown() { | ||
| diags = diags.Append(&hcl.Diagnostic{ | ||
| Severity: hcl.DiagError, | ||
| Summary: "Static variables must be known", |
There was a problem hiding this comment.
I'm confused by the naming - we're calling them "static variables" but the attribute to set that is called "const"? Can we pick one and stick with it - Static variables (static = true) or const variables (const = true)?
| diags = diags.Append(&hcl.Diagnostic{ | ||
| Severity: hcl.DiagError, | ||
| Summary: "No value for required variable", | ||
| Detail: fmt.Sprintf("The root module input variable %q is not set, and has no default value. Use a -var or -var-file command line argument to provide a value for this variable.", name), |
There was a problem hiding this comment.
👍🏻 nice that this message also references the lack of default value!
| cfg.Root = cfg // Root module is self-referential. | ||
| return cfg, diags | ||
| } | ||
| betterVars, parseDiags := backendrun.ParseVariableValues(m.VariableValues, rootMod.Variables) |
| "sensitive_values": {} | ||
| } | ||
| ] | ||
| "format_version": "1.0", |
There was a problem hiding this comment.
In theory if we're changing the json output, this should get incremented, though the fact that it's still a 1.0 suggests to me that we haven't ever bothered with that (I don't know if terraform-json checks the format_version - that's what it was there for, but if it's unused we might consider removing it instead)
This is indeed a change we should discuss, however, as external tools/teams (used to?) parse this output. Does https://github.com/hashicorp/terraform-json need updating?
constattribute to variable blocks #38146This PR captures most of the work related to the dynamic module sources project. It is very difficult to split into individual PRs without ending up with a broken
mainbranch in between, since it touches a lot of the central configuration loading logic.Instead I've tried to create separate commits with detailed messages, so reviewing this large PR becomes easier.
Target Release
1.15.x
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry