bake: default variable to null if not defined

[crazy-max]

fixes #2529

With null val supported for variables in https://github.com/docker/buildx/pull/1449, we should default to this type instead of empty string. Might have been an oversight.

[tonistiigi]

Although I agree that this is a better default, it is a breaking change and if there are users atm who don't define default and rely on it being empty string, this change would break their files either by giving a parse error or sending a different value to buildkit. So I'm not sure if this is worth the risk as currently users can define default = null themselves for same behavior. If we add smth like type in the future maybe we can decide a better default for the non-string variables?

[crazy-max]

Although I agree that this is a better default, it is a breaking change and if there are users atm who don't define default and rely on it being empty string, this change would break their files either by giving a parse error or sending a different value to buildkit. So I'm not sure if this is worth the risk as currently users can define default = null themselves for same behavior. If we add smth like type in the future maybe we can decide a better default for the non-string variables?

Agreed if we add some validation flavor to variables similar to what terraform does: https://github.com/docker/buildx/pull/491#issue-773693934. I move this one to bake-ga milestone.

[tonistiigi]

So I'm not sure if this is worth the risk as currently users can define default = null themselves for same behavior. If we add smth like type in the future maybe we can decide a better default for the non-string variables?

@rrjjvv is there anything we should do here still or should we just close this?

[rrjjvv]

Funny you should ask... there's a couple reasons why my docs PR is in draft mode. One of them was very related to this.

When going through the original docs top-to-bottom trying to match language/style, I noticed the docs about null. It's not a feature I've ever used (though in hindsight, I've seen it a lot since it's heavily used in your projects). I had no tests around this and can't say with certainty how the combination will behave.

Tonight I was going to write some quick tests to validate the behavior, hoping it works as expected. I'll also take a closer look at this PR and related issues at the same time. Hopefully it all "just works" and resolves this issue as well. I'll let you know what I think/find after I get a chance to digest/revisit.

[rrjjvv]

TL;DR; I think you can close this, though one of your earlier comments/ideas is still valid and should be explicitly decided.

My original (potential) concern was default = null combined with typing (relevant, but different from this issue/PR). This does behave as expected; the value remains null regardless of specified type. There's another null + typing concern, but I may bring that up separately as it's not related to this ticket.

As far as this actual issue...

If we add smth like type in the future maybe we can decide a better default for the non-string variables?

Currently, specifying a typed variable with no default and no override results in an empty string, regardless of type. While that may be convenient (or expected given current non-typed behavior), I don't think it's logical. A case could be made for either null (probably the HCL way) or for the "zero value" for the given type (probably what users would expect or find more useful, and my personal preference). But an empty string seems incorrect. Luckily, compatibility isn't a concern yet...

[tonistiigi]

I did some of my own tests. It is somewhat weird though.

variable varstringnull {
  default = null
}

variable varstring {
}

variable varbool {
  type = bool
}

variable varnum {
  type = number
}

variable vararr {
  type = list(string)
}

variable varmap {
  type = map(string)
}

variable vartuple {
  type = tuple([string, number])
}

target foo {
  args = {
    varstringnull = varstringnull == null ? "null" : "notnull"
    varstring = varstring == null ? "null" : "notnull"
    varbool = varbool == null ? "null" : "notnull"
    varnum = varnum == null ? "null" : "notnull"
    vararr = vararr == null ? "null" : "notnull"
    varmap = varmap == null ? "null" : "notnull"
    vartuple = vartuple == null ? "null" : "notnull"
  }
}

  "target": {
    "foo": {
      "context": ".",
      "dockerfile": "Dockerfile",
      "args": {
        "vararr": "notnull",
        "varbool": "notnull",
        "varmap": "notnull",
        "varnum": "notnull",
        "varstring": "notnull",
        "varstringnull": "null",
        "vartuple": "notnull"
      }
    }
  }

Ideally I think at least the array types should be null. varstring is the only one that can't be null for backwards compatibility. But it might be too late to change that.

Additionally, if I add:

    vararrzero = length(vararr) == 0 ? "zero" : "notzero"

Then I get

docker-bake.hcl:37
--------------------
  35 |         varmap = varmap == null ? "null" : "notnull"
  36 |         vartuple = vartuple == null ? "null" : "notnull"
  37 | >>>     vararrzero = length(vararr) == 0 ? "zero" : "notzero"
  38 |       }
  39 |     }
--------------------
ERROR: docker-bake.hcl:37,18-25: Error in function call; Call to function "length" failed: collection must be a list, a map or a tuple., and 1 other diagnostic(s)

» vararr=1,2 docker buildx bake foo --print                                                                                                                                                                                                  ...
  "target": {
    "foo": {
      "context": ".",
      "dockerfile": "Dockerfile",
      "args": {
        "vararr": "notnull",
        "vararrzero": "notzero",
    ...
      }
    }
  }
}

Looks like type was not set to list at all if default and value were both missing.

[tonistiigi]

Similar:

vartuple = vartuple == null ? "null" : "${vartuple[0]} / ${vartuple[1]}"

> vartuple=foo,123 docker buildx bake --print 
        "vartuple": "foo / 123"

but with defaults

  36 | >>>     vartuple = vartuple == null ? "null" : "${vartuple[0]} / ${vartuple[1]}"
  37 |         vararrzero = length(vararr) == 0 ? "zero" : "notzero"
  38 |       }
--------------------
ERROR: docker-bake.hcl:36,55-58: Invalid index; This value does not have any indices., and 2 other diagnostic(s)

And if I set it to vartuple = vartuple == "" ? "null" : "${vartuple[0]} / ${vartuple[1]}" what logically makes no sense then there is no error.

[rrjjvv]

varstring is the only one that can't be null for backwards compatibility.

Agreed/understood.

But it might be too late to change that.

I assume that was in the context of possibly changing the array types. Whether it's "too late" is up to you folks... I don't know if you're release schedule is time-based, feature-based, or what. But it should not be difficult or time-consuming to make the changes. I'd be more than happy to. (I just looked at recent release history, and assuming the patterns aren't coincidence, you're likely looking to release 'very soon'. I have some flexibility and can implement changes quicker than I normally would if time is a factor.)

It is somewhat weird though.

Yup, and why I'm personally consider it a bug (albeit, unreleased). In all cases where typing was provided, the end result was an empty string. That's why you were getting those various errors for the tuple/list, and why that non-sensical ternary "worked". It's not really that the variable itself loses its type, it's that the 'resolution' step is incorrect (the point where the variable is getting assigned to the arg), if that makes sense. While it looks like the opposite on the surface, it's the same thing happening when you have default = null alongside typing. In every case, the value is truly null. Yet, when using it for an arg, only string, number, and bool appear to work (because the 'resolution' process results in those being converted to strings due to backward compatibility, but the complex types have no compatibility behavior, so you get errors trying to use a complex type where a string is expected, despite all having the same actual null value).

Hope that made sense. It's pretty clear when you see it in the form of tests (and resulting failures).

[tonistiigi]

I don't know if you're release schedule is time-based,

We need to cut GA tomorrow, but we can document that for this release users should always define default value if they set type.

[rrjjvv]

but we can document that for this release

In the actual docs, i.e. I should add a snippet in my documentation PR? Or something you'll take care of in the release notes?

[tonistiigi]

In both, but it looks like https://github.com/docker/buildx/pull/3198 might still make it so let's not do that yet.