Bake: allow override of list variables via environment

[rrjjvv]

Description

Given a bake file with

variable "FOO" {
  default = ["one", "two"]
}

there is currently no way to override FOO via environment variable. Being able to override lists of strings or numbers would very valuable, especially when these lists are used to build up a matrix (which itself cannot be overridden).

Other users have posted similar questions (#2882, #2398 (comment)) with creative workarounds, but those only work when you are in control of the file. In my case, an external project has published their own bake file, but I cannot currently take advantage of it (directly calling it remotely) due to not being able to override their list-based variables.

I looked into the error to ensure I wasn't simply doing something wrong and saw this TODO:

buildx/bake/hclparser/hclparser.go

Lines 320 to 321 in 2eaea64

	// TODO: support lists with csv values
	return errors.Errorf("unsupported type %s for variable %s", vv.Type().FriendlyName(), name)

I took a stab at implementing it, but came across issues that made me think it better to create an issue before submitting an actual PR. They all boil down to issues of typing and exactly what you want to support. Despite the code comment, there was only one (atypical) scenario where I was able to actually make a true list (same typed elements). Even in my example above, that is considered a tuple, which just happens to have members which are all strings. The only way I was able to produce a true list was by by using a function that returns an actual list, e.g.

variable "FOO" {
  default = split(",", "one,two")
}

To limit the complexity, I made some assumptions:

1. Any tuple type (the common case) needs members all of the same type. This means that variables with a default of [] cannot be overridden (since there is no defined type and the intended usage is unknown).
2. Sets were omitted, not because they're problematic, but I assumed they wouldn't actually occur.
3. Objects/Maps remain unsupported
4. These lists/tuples would contain only primitive values

If you're still open to this feature, I'd be happy to create a PR.

[tonistiigi]

I think generally this is fine but I think non-string variables should define an explicit type. Similar to terraform equivalent https://developer.hashicorp.com/terraform/language/values/variables#type-constraints

[rrjjvv]

Interesting... I'm not a user of Terraform and never saw mention/examples of explicit typing in the bake docs.

I had a list of further questions, but after stepping through the code to ensure I was asking intelligent questions, they were all moot. Specifying an explicit type (in the same manner as done in that link) can be done, but has no actual impact. It's not processed and is seen as just an arbitrary key/value pair. In fact, you can add virtually any arbitrary key/value pair.:

variable "FOO" {
  type = list(string, string)
  borg = "cool"
  default = [100, 1]
}

is valid, though neither type nor borg actually do anything. Specifically, they are tracked as an Attribute of the variable's Body. Presumably Terraform has implemented their own type specification system, or, if it's functionality provided by the HCL library, bake isn't taking advantage. Not sure which is the case (without digging it).

So, did I take your comment too literally, and missed the fact that there's something in bake that supports explicit typing that I simply missed? Just forgotten/unknown that the functionality might be specific to Terraform? Or are you fully aware of all that, but proposing that explicit typing capabilities be added implemented?

[tonistiigi]

Yes, this is not implemented atm. https://github.com/docker/buildx/blob/master/bake/hclparser/hclparser.go#L28-L34 . I was just suggesting that if we are going to convert env from string into custom types we should make it so that user defines what the type of variable should be, rather than justtrying to imply it from the default value. This would also mean that the default itself would be validated against type if both are set.

[rrjjvv]

Got it, and yes, that would not only simply the (conversion) code but also remove some of those arbitrary restrictions I put in place due to trying to infer type/intent from the default.

So back to

but I think non-string variables should define an explicit type

was that speaking to (logical) lists of non-strings? Or meaning that these explicit types should be implemented before handling any complex type?

Put another way, do you think my PR (after removing my support for lists of other primitive types) could be a "phase 1", with the explicit typing (and increased override flexibility) to come later? Or is this typing functionality a prerequisite to handling any complex types?

I'm happy to give it shot (really, it does sound fun), but it's only my second day in this code base (and first exposure to these libraries), so I'm guessing it's not something I'll crank out overnight.

[tonistiigi]

Put another way, do you think my PR (after removing my support for lists of other primitive types) could be a "phase 1", with the explicit typing (and increased override flexibility) to come later? Or is this typing functionality a prerequisite to handling any complex types?

I think ideally we would have them together. For numbers/bools as well, not just lists.

[rrjjvv]

This is looking promising. It took a while to unwind, but ultimately Terraform is building on top of an 'extension' that HCL provides (https://github.com/hashicorp/hcl/blob/main/ext/typeexpr/README.md). Or equally likely, they originally implemented it then extracted the generic pieces to this extension.

I have some initial tests, both happy and sad paths, working for overrides as well as enforcing any specified defaults. I'm sure it'll be a couple days before I have something ready to share (or another batch of questions), but until then:

1. I assume you'd want this typing effort separate from this issue, i.e. separate PR? A new issue created as well?
2. Based on your expertise, can you anticipate any issues I may run into, or things I may be need to consider before committing to my solution? Vague question, I know, but it appears that virtually all the work can be constrained to that same resolveValue method I touched in my PR, but fear I may be overlooking things that are obvious to you folks.

Some assumptions and tentative rules I'm currently working under:

1. You mentioned that (ideally) regular numbers/booleans would use this typing as well. Given that those can already be overridden today, I assume that behavior should remain unchanged (i.e., not breaking compatibility), but if typing is provided, it will be used.
2. It's okay to lean on that extension (not a new dependency), and we'll inherit any limitations imposed by that library (e.g., the type specification cannot be dynamically constructed).
3. When types are specified, I will defer to their type conversion/coercion routines as much as possible.
4. I'm assuming/hoping that there's no unique considerations around inheritance/merging and it'll "just work".
5. Most of my previous restrictions were practical in nature; most of those will be lifted.
6. While extending this to user-defined functions is a logical progression, I see that as being out of scope.

Let me know if this all sounds reasonable, and if you think this should be a separate issue and/or PR.

[rrjjvv]

I decided to be optimistic that lack of response was more indicative that you were were at least not opposed (since it's probably very quick and easy to say "no thanks"), so I created #3167. It probably make senses for me to close this one, but will wait for your input.