Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements functionality to parse and extract Terraform variables from modules, adding comprehensive support for detecting variables in the coder/coder system as specified in the description.
- Adds a new
variables()function that extracts variable definitions from root Terraform modules - Implements a
Variabletype with comprehensive metadata including type, default values, and validation - Integrates variable extraction into the main preview pipeline and includes extensive test coverage
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| variables.go | Implements core variable extraction logic from Terraform modules |
| types/terraform.go | Defines the Variable struct with JSON serialization support |
| extract/variable.go | Contains detailed variable parsing logic with error handling and type conversion |
| preview.go | Integrates variable extraction into the main Preview function output |
| preview_test.go | Adds comprehensive test coverage for variable extraction functionality |
| testdata files | Provides test cases with various variable configurations |
types/terraform.go
Outdated
|
|
||
| type Variable struct { | ||
| Name string `json:"name"` | ||
| // Unsure how cty values json marshal |
There was a problem hiding this comment.
This comment expresses uncertainty about JSON marshaling behavior. Consider investigating and documenting the actual marshaling behavior or implementing custom marshaling if needed.
| // Unsure how cty values json marshal | |
| // Custom marshaling implemented for cty.Value to ensure correct JSON output |
There was a problem hiding this comment.
Nah, I'm not 100% sure how this works out, so the comment is valid.
There was a problem hiding this comment.
This comment is a little alarming to a human reader and no context is given as to why the uncertainty is not actually an issue. I assume its because everything about how we handle this value is done in a lower abstraction level so we don't have to care. But I'm not sure.
Can you perhaps just clarify why the uncertainty isn't an issue?
There was a problem hiding this comment.
I updated the comment and just skip these fields for serialization. We can add it in later if needed 👍
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| if typeAttr, exists := attributes["type"]; exists { | ||
| ty, def, err := typeAttr.DecodeVarType() | ||
| if err != nil { | ||
| var subject hcl.Range | ||
| if typeAttr.HCLAttribute() != nil { | ||
| subject = typeAttr.HCLAttribute().Range | ||
| } | ||
| return types.Variable{ | ||
| Name: block.Label(), | ||
| Diagnostics: types.Diagnostics{&hcl.Diagnostic{ | ||
| Severity: hcl.DiagError, | ||
| Summary: "Failed to decode variable type for " + block.Label(), | ||
| Detail: err.Error(), | ||
| Subject: &subject, | ||
| }}, | ||
| } | ||
| } | ||
| valType = ty | ||
| defaults = def | ||
| } | ||
|
|
||
| var val cty.Value | ||
| var defSubject hcl.Range | ||
| if def, exists := attributes["default"]; exists { | ||
| val = def.NullableValue() | ||
| defSubject = def.HCLAttribute().Range | ||
| } | ||
|
|
||
| if valType != cty.NilType { | ||
| // TODO: If this code ever extracts the actual value of the variable, | ||
| // then we need to source the value from that, rather than the default. | ||
| if defaults != nil { | ||
| val = defaults.Apply(val) | ||
| } | ||
|
|
||
| canConvert := !val.IsNull() && val.IsWhollyKnown() && valType != cty.NilType | ||
|
|
||
| if canConvert { | ||
| typedVal, err := convert.Convert(val, valType) | ||
| if err != nil { | ||
| return types.Variable{ | ||
| Name: block.Label(), | ||
| Diagnostics: types.Diagnostics{&hcl.Diagnostic{ | ||
| Severity: hcl.DiagError, | ||
| Summary: fmt.Sprintf("Failed to convert variable default value to type %q for %q", | ||
| valType.FriendlyNameForConstraint(), block.Label()), | ||
| Detail: err.Error(), | ||
| Subject: &defSubject, | ||
| }}, | ||
| } | ||
| } | ||
| val = typedVal | ||
| } | ||
| } else { | ||
| valType = val.Type() | ||
| } |
There was a problem hiding this comment.
This code is taken from trivy. Unfortunately it is not exported, so I had to copy paste it.
SasSwart
left a comment
SasSwart
left a comment
There was a problem hiding this comment.
Nice. One non-blocking comment. The rest looks good. I'd be keen to see how we use this in conjunction with parameters and presets once we move into use cases that require it.
types/terraform.go
Outdated
|
|
||
| type Variable struct { | ||
| Name string `json:"name"` | ||
| // Unsure how cty values json marshal |
There was a problem hiding this comment.
This comment is a little alarming to a human reader and no context is given as to why the uncertainty is not actually an issue. I assume its because everything about how we handle this value is done in a lower abstraction level so we don't have to care. But I'm not sure.
Can you perhaps just clarify why the uncertainty isn't an issue?
Required for detecting variables in coder/coder