Skip to content

Consider typed, value-less variables to have null value #3198

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.

Sign up for GitHub

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

Merged
crazy-max merged 2 commits into from
May 21, 2025
Merged

Consider typed, value-less variables to have null value #3198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c0f5c5
Consider typed, value-less variables to have `null` value
rrjjvv May 20, 2025
291c353
bake: TestEmptyVariable
crazy-max May 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 73 additions & 28 deletions bake/hcl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,63 @@ func TestHCLNullVariables(t *testing.T) {
require.Equal(t, ptrstr("bar"), c.Targets[0].Args["foo"])
}

func TestHCLTypedNullVariables(t *testing.T) {
types := []string{
"any",
"string", "number", "bool",
"list(string)", "set(string)", "map(string)",
"tuple([string])", "object({val: string})",
}
for _, varType := range types {
tName := fmt.Sprintf("variable typed %q with null default remains null", varType)
t.Run(tName, func(t *testing.T) {
dt := fmt.Sprintf(`
variable "FOO" {
type = %s
default = null
}

target "default" {
args = {
foo = equal(FOO, null)
}
}`, varType)
c, err := ParseFile([]byte(dt), "docker-bake.hcl")
require.NoError(t, err)
require.Equal(t, 1, len(c.Targets))
require.Equal(t, "true", *c.Targets[0].Args["foo"])
})
}
}

func TestHCLTypedValuelessVariables(t *testing.T) {
types := []string{
"any",
"string", "number", "bool",
"list(string)", "set(string)", "map(string)",
"tuple([string])", "object({val: string})",
}
for _, varType := range types {
tName := fmt.Sprintf("variable typed %q with no default is null", varType)
t.Run(tName, func(t *testing.T) {
dt := fmt.Sprintf(`
variable "FOO" {
type = %s
}

target "default" {
args = {
foo = equal(FOO, null)
Copy link
Member

@tonistiigi tonistiigi May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just making sure that we do have a test that checks that without the type the value is empty string?

Copy link
Contributor Author

@rrjjvv rrjjvv May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't appear so.
There's one that verifies it's not an error condition (ignoring the value):

buildx/bake/hcl_test.go

Lines 1568 to 1576 in ea2b702

func TestEmptyVariableJSON(t *testing.T) {
dt := []byte(`{
"variable": {
"VAR": {}
}
}`)
_, err := ParseFile(dt, "docker-bake.json")
require.NoError(t, err)
}

But I actually think this one indirectly does so:

buildx/bake/bake_test.go

Lines 1989 to 2003 in ea2b702

func TestVariableValidation(t *testing.T) {
fp := File{
Name: "docker-bake.hcl",
Data: []byte(`
variable "FOO" {
validation {
condition = FOO != ""
error_message = "FOO is required."
}
}
target "app" {
args = {
FOO = FOO
}
}

I'm fairly sure that equality checks will not invoke coercion. I know the validation does (one of its defining features), but don't know whether it would coerce a null to an empty string. Seems unlikely, but can't say for sure.

Do you want an explicit one just to be safe?

Copy link
Member

@crazy-max crazy-max May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want an explicit one just to be safe?

Yes would be good

Copy link
Member

@crazy-max crazy-max May 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed extra commit to add test for empty string if no type set

}
}`, varType)
c, err := ParseFile([]byte(dt), "docker-bake.hcl")
require.NoError(t, err)
require.Equal(t, 1, len(c.Targets))
require.Equal(t, "true", *c.Targets[0].Args["foo"])
})
}
}

func TestJSONNullVariables(t *testing.T) {
dt := []byte(`{
"variable": {
Expand Down Expand Up @@ -1565,6 +1622,20 @@ target "two" {
require.Equal(t, map[string]*string{"b": ptrstr("pre-jkl")}, c.Targets[1].Args)
}

func TestEmptyVariable(t *testing.T) {
dt := []byte(`
variable "FOO" {}
target "default" {
args = {
foo = equal(FOO, "")
}
}`)
c, err := ParseFile(dt, "docker-bake.hcl")
require.NoError(t, err)
require.Equal(t, 1, len(c.Targets))
require.Equal(t, "true", *c.Targets[0].Args["foo"])
}

func TestEmptyVariableJSON(t *testing.T) {
dt := []byte(`{
"variable": {
Expand Down Expand Up @@ -1877,19 +1948,6 @@ func TestTypedVarOverrides(t *testing.T) {
override: `"hello"`,
wantValue: `"hello"`,
},
{
name: "any",
varType: "any",
override: "[1,2]",
wantValue: "[1,2]",
},
{
name: "any never convert to complex types",
varType: "any",
override: "[1,2]",
argValue: "length(FOO)",
wantErrorMsg: "collection must be a list",
},
{
name: "proper CSV list of strings",
varType: "list(string)",
Expand Down Expand Up @@ -2090,19 +2148,6 @@ func TestTypedVarOverrides_JSON(t *testing.T) {
override: `"hello"`,
wantValue: "hello",
},
{
name: "any",
varType: "any",
override: "[1,2]",
wantValue: "[1,2]",
},
{
name: "any never convert to complex types",
varType: "any",
override: "[1,2]",
argValue: "length(FOO)",
wantErrorMsg: "collection must be a list",
},
{
name: "list of strings",
varType: "list(string)",
Expand Down Expand Up @@ -2313,6 +2358,7 @@ func TestJSONOverridePriority(t *testing.T) {
dt := []byte(`
variable "foo" {
type = number
default = 101
}

target "default" {
Expand All @@ -2325,8 +2371,7 @@ func TestJSONOverridePriority(t *testing.T) {
c, err := ParseFile(dt, "docker-bake.hcl")
require.NoError(t, err)
require.Equal(t, 1, len(c.Targets))
// a variable with no value has always resulted in an empty string
require.Equal(t, "", *c.Targets[0].Args["bar"])
require.Equal(t, "101", *c.Targets[0].Args["bar"])

t.Setenv("foo_JSON", "42")
c, err = ParseFile(dt, "docker-bake.hcl")
Expand Down
12 changes: 5 additions & 7 deletions bake/hclparser/hclparser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ func (p *parser) resolveValue(ectx *hcl.EvalContext, name string) (err error) {
}

var diags hcl.Diagnostics
varType := cty.DynamicPseudoType
varType, typeSpecified := cty.DynamicPseudoType, false
def, ok := p.attrs[name]
if !ok {
vr, ok := p.vars[name]
Expand All @@ -295,12 +295,13 @@ func (p *parser) resolveValue(ectx *hcl.EvalContext, name string) (err error) {
if diags.HasErrors() {
return diags
}
typeSpecified = !varType.Equals(cty.DynamicPseudoType) || hcl.ExprAsKeyword(vr.Type) == "any"
}

if def == nil {
// lack of specified value is considered to have an empty string value,
// but any overrides get type checked
if _, ok, _ := p.valueHasOverride(name, false); !ok {
// Lack of specified value, when untyped is considered to have an empty string value.
// A typed variable with no value will result in (typed) nil.
Copy link
Contributor Author

@rrjjvv rrjjvv May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hindsight, I probably should have done an early return for the typed nil case rather than letting it flow through; let me know if you think it should be changed, or good enough as it is.

if _, ok, _ := p.valueHasOverride(name, false); !ok && !typeSpecified {
vv := cty.StringVal("")
v = &vv
return
Expand All @@ -322,9 +323,6 @@ func (p *parser) resolveValue(ectx *hcl.EvalContext, name string) (err error) {
}
}

// Not entirely true... this doesn't differentiate between a user that specified 'any'
// and a user that specified nothing. But the result is the same; both are treated as strings.
typeSpecified := !varType.Equals(cty.DynamicPseudoType)
envv, hasEnv, jsonEnv := p.valueHasOverride(name, typeSpecified)
_, isVar := p.vars[name]

Expand Down