-
Notifications
You must be signed in to change notification settings - Fork 600
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 <[email protected]>
| // 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.
tonistiigi
left a comment
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 <[email protected]>
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
anywas previously treated as if the typing was omitted; with no defined value or override, that resulted in an empty string. Theanytype is now distinguished from an omitted type; these variables, with no default or override, are alsonull.In other respects, the behavior of
anyis 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.