buildflags: handle unknown values from cty #2965
Conversation
jsternberg
force-pushed
the
handle-unknown-values
branch
from
9cc2231 to
3e1da26
Compare
| target "app" { | ||
| attest = [ | ||
| "type=sbom,disabled=${SBOM}", | ||
| ] | ||
|
|
||
| cache-from = [ | ||
| { type = "registry", ref = "user/app:${FOO1}" }, | ||
| "type=local,src=path/to/cache:${FOO2}", | ||
| ] | ||
|
|
||
| cache-to = [ | ||
| { type = "local", dest = "path/to/${BAR}" }, | ||
| ] | ||
|
|
||
| output = [ | ||
| { type = "oci", dest = "../${OUTPUT}.tar" }, | ||
| ] | ||
|
|
||
| secret = [ | ||
| { id = "mysecret", src = "/local/${SECRET}" }, | ||
| ] | ||
|
|
||
| ssh = [ | ||
| { id = "key", paths = ["path/to/${SSH_KEY}"] }, | ||
| ] | ||
| } |
There was a problem hiding this comment.
Was looking at how this example behaves in Buildx 0.19 and I got 13 diagnostics:
docker buildx bake --print
#1 [internal] load local bake definitions
#1 reading docker-bake.hcl 451B / 451B done
#1 DONE 0.0s
docker-bake.hcl:6
--------------------
4 | ]
5 | cache-from = [
6 | >>> { type = "registry", ref = "user/app:${FOO1}" },
7 | "type=local,src=path/to/cache:${FOO2}",
8 | ]
--------------------
ERROR: docker-bake.hcl:6,43-47: Unknown variable; There is no variable named "FOO1"., and 12 other diagnostic(s)
But here I got 8 diagnostics:
docker buildx bake --print
#1 [internal] load local bake definitions
#1 reading docker-bake.hcl 451B / 451B done
#1 DONE 0.0s
docker-bake.hcl:13
--------------------
11 | ]
12 | output = [
13 | >>> { type = "oci", dest = "../${OUTPUT}.tar" },
14 | ]
15 | secret = [
--------------------
ERROR: docker-bake.hcl:13,33-39: Unknown variable; There is no variable named "OUTPUT"., and 7 other diagnostic(s)
Not for this PR but wonder if we could show all diags when --debug is set?
nit: Also seems to return OUTPUT as first diag with this PR but in older release returns what seems to be the first one FOO1
There was a problem hiding this comment.
I'd definitely say we should show all diagnostics. The number of diagnostics can be a bit strange sometimes because they will sometimes depend on the ordering of evaluation and one diagnostic can trigger another diagnostic. There will also be some diagnostics sometimes that say "cannot use unknown value" that are triggered somewhere.
Update the buildflags cty code to handle unknown values. When hcl decodes a value with an invalid variable name, it appends a diagnostic for the error and then returns an unknown value so it can continue processing the file and finding more errors. The iteration code has now been changed to use a rangefunc from go 1.23 and it skips empty or unknown values. Empty values are valid when they are skipped and unknown values will have a diagnostic for itself. Signed-off-by: Jonathan A. Sternberg <[email protected]>
jsternberg
force-pushed
the
handle-unknown-values
branch
from
3e1da26 to
abc85c3
Compare
|
I updated the test to ensure that it sees the "no variable named" for each of the named variables and it ignores the other diagnostics since they aren't wrong or important for the test. |
Added needs follow-up label for displaying all diags in debug mode. |
crazy-max
added
needs/docs-follow-up
status/needs-follow-up
and removed
needs/docs-follow-up
labels
| go 1.22.0 | ||
| go 1.23.0 |
There was a problem hiding this comment.
Was this intentional; i.e., go1.22 no longer supported with this change?
There was a problem hiding this comment.
This was intentional to be able to use rangefunc. After an offline conversation, I was informed that buildx has some downstream dependencies that haven't moved to go 1.23 yet and we generally try to make our go.mod files compatible with stable - 1 go versions so we can introduce this usage when go 1.24 releases.
I've opened #2978 to keep the bones of this functionality but to revert the compiler usage of the feature.
Update the buildflags cty code to handle unknown values. When hcl decodes a value with an invalid variable name, it appends a diagnostic for the error and then returns an unknown value so it can continue processing the file and finding more errors.
The iteration code has now been changed to use a rangefunc from go 1.23 and it skips empty or unknown values. Empty values are valid when they are skipped and unknown values will have a diagnostic for itself.
Fixes #2960.