-
Notifications
You must be signed in to change notification settings - Fork 471
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bake: merge targets and vars from multiple JSON files #1025
Conversation
39f8867
to
9d250d1
Compare
bake/hcl_test.go
Outdated
@@ -620,3 +620,79 @@ func TestHCLBuiltinVars(t *testing.T) { | |||
require.Equal(t, "foo", *c.Targets[0].Context) | |||
require.Equal(t, "test", *c.Targets[0].Dockerfile) | |||
} | |||
|
|||
func TestCombineHCLAndJSON(t *testing.T) { |
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.
Please also add tests that actually check the attributes feature with multiple files, not only the reserved blocks.
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.
Last commit adds support for vars too.
9d250d1
to
3d8b115
Compare
3ba2e0a
to
8855bb1
Compare
}, | ||
{ | ||
Name: "bar.json", | ||
Data: []byte(`{"ABC": "ghi", "DEF": "jkl"}`), |
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.
Does it still work when hcl also defines ABC = "old"
and foo.json
"ABC": "old2"
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.
Indeed I tried with attributes in global scope an doesn't seem to work atm (see last commit)
=== RUN TestCombineHCLAndJSONAttrs
hcl_test.go:790:
Error Trace: hcl_test.go:790
Error: Not equal:
expected: map[string]string{"a":"pre-ghi-jkl"}
actual : map[string]string{"a":"pre-foo-jkl"}
I guess it's how merging blocks are handled for attributes in global scope where only the first one is retained atm:
ABC = "foo"
variable "DEF" {
default = ""
}
group "default" {
targets = ["one"]
}
target "one" {
args = {
a = "pre-${ABC}"
}
}
target "two" {
args = {
b = "pre-${DEF}"
}
}
{"ABC": "oof", "variable": {"DEF": {"default": "bar"}}, "target": { "one": { "args": {"a": "pre-${ABC}-${DEF}"}} } }
{"ABC": "ghi", "DEF": "jkl"}
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.
Does it at least work with HCL-only?
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.
Same with HCL only (see test in last commit) 😞:
=== RUN TestCombineHCLAttrs
hcl_test.go:801:
Error Trace: hcl_test.go:801
Error: Not equal:
expected: map[string]string{"a":"pre-ghi-jkl"}
actual : map[string]string{"a":"pre-foo-jkl"}
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.
Looks like we missed a case in #541. Changing this test with a global attr FOO = "abc"
in first file makes it failed:
func TestHCLMultiFileAttrs(t *testing.T) {
os.Unsetenv("FOO")
dt := []byte(`
FOO = "abc"
target "app" {
args = {
v1 = "pre-${FOO}"
}
}
`)
dt2 := []byte(`
FOO="def"
`)
c, err := ParseFiles([]File{
{Data: dt, Name: "c1.hcl"},
{Data: dt2, Name: "c2.hcl"},
}, nil)
require.NoError(t, err)
require.Equal(t, 1, len(c.Targets))
require.Equal(t, c.Targets[0].Name, "app")
require.Equal(t, "pre-def", c.Targets[0].Args["v1"])
os.Setenv("FOO", "ghi")
c, err = ParseFiles([]File{
{Data: dt, Name: "c1.hcl"},
{Data: dt2, Name: "c2.hcl"},
}, nil)
require.NoError(t, err)
require.Equal(t, 1, len(c.Targets))
require.Equal(t, c.Targets[0].Name, "app")
require.Equal(t, "pre-ghi", c.Targets[0].Args["v1"])
}
=== RUN TestHCLMultiFileAttrs
hcl_test.go:488:
Error Trace: hcl_test.go:488
Error: Not equal:
expected: "pre-def"
actual : "pre-abc"
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.
Ah JustAttributes
skips repeated attributes: https://github.com/hashicorp/hcl/blob/c7ee8b78101c33b4dfed2641d78cf5e9651eabb8/merged.go#L109-L120
We might need to bring in this func in the hclparser to fix that for us wdyt?
8855bb1
to
5e9d5ff
Compare
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
ed19533
to
9d03625
Compare
c3710b0
to
2246574
Compare
2246574
to
cad7ed6
Compare
@tonistiigi As discussed, attributes merge support has been removed. Only targets and vars are supported atm. Will be done in a follow-up. |
fixes #756
fixes docker/bake-action#61
while checking the issue reported in docker/bake-action#61, I found out that the HCL diagnostic being reported was wrong:
Blocks are not allowed here.
should be filtered out as it's ignored while checking HCL attributes:buildx/bake/hclparser/hclparser.go
Line 306 in 1ca30a5
Now that the diag is filtered out we have the right diagnostic being reported: https://github.com/crazy-max/buildx-buildkit-tests/tree/main/bake-action-61
This PR also filters out the diagnostic
Argument "x" was already set at
only for reserved names so we can merge targets from multiple JSON files: https://github.com/crazy-max/buildx-buildkit-tests/tree/main/buildx-756Same with https://github.com/crazy-max/buildx-buildkit-tests/tree/main/bake-action-61
@tonistiigi I'm not sure if we should skip
Argument "x" was already set at
diags for all reserved names or only skip for thetarget
one.cc @felipecrs @iwilltry42
Signed-off-by: CrazyMax crazy-max@users.noreply.github.com