8000 Consider typed, value-less variables to have `null` value by rrjjvv · Pull Request #3198 · docker/buildx · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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.

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
merged 2 commits into from
May 21, 2025

Conversation

rrjjvv
Copy link
Contributor
@rrjjvv rrjjvv commented May 20, 2025

Resolves #3160
Addresses issues discovered in #2530 (comment) and further discussed in #3189

A variable with a type but no default value or override resulted in an empty string. This matches the legacy behavior of untyped variables, but does not make sense when using types (an empty string is itself a type violation for everything except string). All variables defined with a type but with no value are now a typed null.

A variable explicitly typed any was previously treated as if the typing was omitted; with no defined value or override, that resulted in an empty string. The any type is now distinguished from an omitted type; these variables, with no default or override, are also null.

In other respects, the behavior of any is unchanged and largely behaves as if the type was omitted. It's not clear whether it should be supported, let alone how it should behave, so these tests were removed.
It's being treated as undefined behavior.

A variable with a type but no default value or override resulted in an
empty string.  This matches the legacy behavior of untyped variables,
but does not make sense when using types (an empty string is itself a
type violation for everything except `string`).  All variables defined
with a type but with no value are now a typed `null`.

A variable explicitly typed `any` was previously treated as if the
typing was omitted; with no defined value or override, that resulted in
an empty string.  The `any` type is now distinguished from an omitted
type; these variables, with no default or override, are also `null`.

In other respects, the behavior of `any` is unchanged and largely
behaves as if the type was omitted.  It's not clear whether it should be
 supported, let alone how it should behave, so these tests were removed.
It's being treated as undefined behavior.

Signed-off-by: Roberto Villarreal <rrjjvv@yahoo.com>
// 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

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.

@tonistiigi tonistiigi added this to the v0.24.0 milestone May 20, 2025
Copy link
Member
@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

I was considering if type=bool should default to false, but I think just keeping in null for all makes it most consistent.


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

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

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

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

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

@rrjjvv
Copy link
Contributor Author
rrjjvv commented May 20, 2025

I was considering if type=bool should default to false

Yeah, I struggled with that. A case could be made that primitives should get zero values (empty string, false, zero), but it really feels that it should be all or nothing... all null, or all zero values. However, all of our attention and tests are around build args, where a null isn't valid (and why this PR isn't super friendly, even if correct). But are there other bake attributes that legitimately accept a null? That answer might solidify whether the null approach will be a usability issue or be useful in its own right.

Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
@crazy-max crazy-max merged commit 776dbd4 into docker:master May 21, 2025
138 checks passed
@rrjjvv rrjjvv deleted the var-typing-no-value-fix branch May 21, 2025 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bake: allow override of list variables via environment
3 participants
0