- Notifications
You must be signed in to change notification settings - Fork1k
fix(provisioner/terraform/tfparse): skip evaluation of unrelated parameters#16023
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| "stringparam":"a", | ||
| "numparam":"1", | ||
| "boolparam":"true", | ||
| "listparam":`["a", "b"]`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review: this ends up being allowed due to thedefault ofcoder_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.| } | ||
| varfoundbool | ||
| needle:=strings.Join([]string{"data",dataResource.Type,dataResource.Name},".") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As far as I can tell, there is no way to break a variable reference over multiple lines e.g.
data.\coder_parameter.\foo.\valueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
correct 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I approve this change although I'm concerned about the overall parsing logic. With another patch it will become harder to debug or touch internals in the future.
| } | ||
| varfoundbool | ||
| needle:=strings.Join([]string{"data",dataResource.Type,dataResource.Name},".") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
correct 👍
| for_,tv:=rangetags { | ||
| ifstrings.Contains(tv,v.Name) { | ||
| found=true | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: you could extract this block and the one below to a common helper function
… workspace_tags found
ce68fe2 toe56cac7CompareSmoke-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 |
| } | ||
| varfoundbool | ||
| for_,tv:=rangetags { | ||
| ifstrings.Contains(tv,v.Name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| ifstrings.Contains(tv,v.Name) { | |
| ifstrings.Contains(tv,"var."+v.Name) { |
| } | ||
| varfoundbool | ||
| for_,tv:=rangetags { | ||
| ifstrings.Contains(tv,v.Name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
won't astrings.Contains also contain variable names that are subsets?
Likevar.Foo will match bothFoo andvar.FooBar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is the reason you do a contains because it could be an expression likefoo(var.name)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there any way to parse the expression and get the actual tokens? Or we only havestring in the HCL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yep. I'm looking into HCL traversals now as they might allow us to do this cleanly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
1ab10cf intomainUh oh!
There was an error while loading.Please reload this page.
/cherry-pick release-2.18 |
/cherry-pick release/2.18 |
Uh oh!
There was an error while loading.Please reload this page.
Fixes#16021
tfparsehad been enforcing the "no functions, no locals" rule forallcoder_parameterdefault values, where we only want to enforce this for ones referenced bycoder_workspace_tags.