fix(provisioner/terraform/tfparse): skip evaluation of unrelated parameters #16023
Conversation
| "stringparam": "a", | ||
| "numparam": "1", | ||
| "boolparam": "true", | ||
| "listparam": `["a", "b"]`, |
There was a problem hiding this comment.
review: this ends up being allowed due to the default of coder_parameter being a string.
| }`, | ||
| }, | ||
| expectTags: nil, | ||
| expectError: `can't convert variable default value to string: unsupported type []interface {}`, |
There was a problem hiding this comment.
review: this is also not allowed by the provider:
│ on main.tf line 38, in data "coder_workspace_tags" "tags":
│ 38: tags = {
│ 39: "listvar" = ["a"]
│ 40: }
│
│ Inappropriate value for attribute "tags": element "listvar": string required.
| }`, | ||
| }, | ||
| expectTags: nil, | ||
| expectError: `can't convert variable default value to string: unsupported type map[string]interface {}`, |
There was a problem hiding this comment.
review: also not allowed by the provider:
│ on main.tf line 38, in data "coder_workspace_tags" "tags":
│ 38: tags = {
│ 39: "mapvar" = {a = "b"}
│ 40: }
│
│ Inappropriate value for attribute "tags": element "mapvar": string required.
| @@ -290,6 +305,18 @@ func (p *Parser) CoderParameterDefaults(ctx context.Context, varsDefaults map[st | |||
| continue | |||
| } | |||
|
|
|||
| var found bool | |||
| needle := strings.Join([]string{"data", dataResource.Type, dataResource.Name}, ".") | |||
There was a problem hiding this comment.
As far as I can tell, there is no way to break a variable reference over multiple lines e.g.
data.\
coder_parameter.\
foo.\
value
| @@ -290,6 +305,18 @@ func (p *Parser) CoderParameterDefaults(ctx context.Context, varsDefaults map[st | |||
| continue | |||
| } | |||
|
|
|||
| var found bool | |||
| needle := strings.Join([]string{"data", dataResource.Type, dataResource.Name}, ".") | |||
| for _, tv := range tags { | ||
| if strings.Contains(tv, v.Name) { | ||
| found = true | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: you could extract this block and the one below to a common helper function
… workspace_tags found
ce68fe2 to
e56cac7
Compare
|
Smoke-tested on personal deployment with example from linked issue. Screen.Recording.2025-01-03.at.13.27.45.movNote that issue described in #16022 is still present if that parameter is referenced in |
| m := make(map[string]string) | ||
| for _, v := range p.module.Variables { | ||
| if v == nil { | ||
| continue | ||
| } | ||
| var found bool | ||
| for _, tv := range tags { | ||
| if strings.Contains(tv, v.Name) { |
There was a problem hiding this comment.
| if strings.Contains(tv, v.Name) { | |
| if strings.Contains(tv, "var."+v.Name) { |
| m := make(map[string]string) | ||
| for _, v := range p.module.Variables { | ||
| if v == nil { | ||
| continue | ||
| } | ||
| var found bool | ||
| for _, tv := range tags { | ||
| if strings.Contains(tv, v.Name) { |
There was a problem hiding this comment.
won't a strings.Contains also contain variable names that are subsets?
Like var.Foo will match both Foo and var.FooBar
There was a problem hiding this comment.
Here is a test case:
{
name: "overlapping var name",
files: map[string]string{
`main.tf`: `
// This file is the same as the above, except for this comment.
variable "a" {
type = string
default = "1"
}
variable "ab" {
description = "This is a variable of type string"
type = string
default = jsonencode(["a", "b"])
}
data "coder_workspace_tags" "tags" {
tags = {
"foo": "bar",
"a": var.a,
}
}`,
},
reqTags: map[string]string{"foo": "noclobber"},
wantTags: map[string]string{"owner": "", "scope": "organization", "foo": "bar", "a": "1"},
},There was a problem hiding this comment.
Gotcha, we may then need to do some more "intelligent" parsing.
I hesitate to reach for a regexp.... but \b may be exactly what we need here
There was a problem hiding this comment.
Is the reason you do a contains because it could be an expression like foo(var.name)?
There was a problem hiding this comment.
Is there any way to parse the expression and get the actual tokens? Or we only have string in the HCL?
There was a problem hiding this comment.
Yep. I'm looking into HCL traversals now as they might allow us to do this cleanly.
There was a problem hiding this comment.
OK, I've modified the implementation to instead:
- Gather the set of variables referenced by the tags up-front,
- Only evaluate the default value(s) for
coder_parametersreferenced by those tags. - As default values of coder_parameters can reference other variables, to avoid needing to perform a graph traversal of all the dependencies here I've elected to simply continue passing in all the variable default values. We don't immediately enforce default values for all variables.
- To avoid unrelated variables with default values of complex types being rejected, I added some 'last-ditch' support for JSON-encoding them.
|
/cherry-pick release-2.18 |
|
/cherry-pick release/2.18 |
Fixes #16021
tfparsehad been enforcing the "no functions, no locals" rule for allcoder_parameterdefault values, where we only want to enforce this for ones referenced bycoder_workspace_tags.