-
Notifications
You must be signed in to change notification settings - Fork 554
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
Conversation
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. |
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.
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.
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 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) |
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.
just making sure that we do have a test that checks that without the type
the value is empty string?
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.
It doesn't appear so.
There's one that verifies it's not an error condition (ignoring the value):
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:
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?
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.
Do you want an explicit one just to be safe?
Yes would be good
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.
Pushed extra commit to add test for empty string if no type set
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 |
Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
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 typednull
.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. Theany
type is now distinguished from an omitted type; these variables, with no default or override, are alsonull
.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.