Skip to content

Show types during variable list operation #3207

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.

Sign up for GitHub

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
crazy-max merged 1 commit into from
Jun 2, 2025
Merged

Show types during variable list operation #3207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 30 additions & 5 deletions bake/hclparser/hclparser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ type variable struct {
Validations []*variableValidation `json:"validation,omitempty" hcl:"validation,block"`
Body hcl.Body `json:"-" hcl:",body"`
Remain hcl.Body `json:"-" hcl:",remain"`

// the type described by Type if it was specified
constraint *cty.Type
}

type variableValidation struct {
Expand Down Expand Up @@ -296,6 +299,9 @@ func (p *parser) resolveValue(ectx *hcl.EvalContext, name string) (err error) {
return diags
}
typeSpecified = !varType.Equals(cty.DynamicPseudoType) || hcl.ExprAsKeyword(vr.Type) == "any"
if typeSpecified {
vr.constraint = &varType
}
}

if def == nil {
Expand Down Expand Up @@ -674,6 +680,7 @@ func (p *parser) validateVariables(vars map[string]*variable, ectx *hcl.EvalCont
type Variable struct {
Name string `json:"name"`
Description string `json:"description,omitempty"`
Type string `json:"type,omitempty"`
Value *string `json:"value,omitempty"`
}

Expand Down Expand Up @@ -803,13 +810,31 @@ func Parse(b hcl.Body, opt Opt, val any) (*ParseMeta, hcl.Diagnostics) {
Name: p.vars[k].Name,
Description: p.vars[k].Description,
}
tc := p.vars[k].constraint
if tc != nil {
v.Type = tc.FriendlyNameForConstraint()
}
if vv := p.ectx.Variables[k]; !vv.IsNull() {
var s string
switch vv.Type() {
case cty.String:
s = vv.AsString()
case cty.Bool:
s = strconv.FormatBool(vv.True())
switch {
case tc != nil:
if bs, err := ctyjson.Marshal(vv, *tc); err == nil {
s = string(bs)
// untyped strings were always unquoted, so be consistent with typed strings as well
if tc.Equals(cty.String) {
s = strings.Trim(s, "\"")
}
}
case vv.Type().IsPrimitiveType():
// all primitives can convert to string, so error should never occur
if val, err := convert.Convert(vv, cty.String); err == nil {
s = val.AsString()
}
default:
// must be an (inferred) tuple or object
if bs, err := ctyjson.Marshal(vv, vv.Type()); err == nil {
s = string(bs)
}
}
v.Value = &s
}
Expand Down
4 changes: 2 additions & 2 deletions commands/bake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,7 @@ func printVars(w io.Writer, format string, vars []*hclparser.Variable) error {
tw := tabwriter.NewWriter(w, 1, 8, 1, '\t', 0)
defer tw.Flush()

tw.Write([]byte("VARIABLE\tVALUE\tDESCRIPTION\n"))
tw.Write([]byte("VARIABLE\tTYPE\tVALUE\tDESCRIPTION\n"))

for _, v := range vars {
var value string
Expand All @@ -672,7 +672,7 @@ func printVars(w io.Writer, format string, vars []*hclparser.Variable) error {
} else {
value = "<null>"
}
fmt.Fprintf(tw, "%s\t%s\t%s\n", v.Name, value, v.Description)
fmt.Fprintf(tw, "%s\t%s\t%s\t%s\n", v.Name, v.Type, value, v.Description)
}
return nil
}
Expand Down
11 changes: 7 additions & 4 deletions docs/reference/buildx_bake.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,12 +198,15 @@ To list variables:

```console
$ docker buildx bake --list=variables
VARIABLE VALUE DESCRIPTION
REGISTRY docker.io/username Registry and namespace
IMAGE_NAME my-app Image name
GO_VERSION <null>
VARIABLE TYPE VALUE DESCRIPTION
REGISTRY string docker.io/username Registry and namespace
IMAGE_NAME string my-app Image name
GO_VERSION <null>
DEBUG bool false Add debug symbols
```

Variable types will be shown when set using the `type` property in the Bake file.

By default, the output of `docker buildx bake --list` is presented in a table
format. Alternatively, you can use a long-form CSV syntax and specify a
`format` attribute to output the list in JSON.
Expand Down
89 changes: 88 additions & 1 deletion tests/bake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ var bakeTests = []func(t *testing.T, sb integration.Sandbox){
testBakeLoadPush,
testListTargets,
testListVariables,
testListTypedVariables,
testBakeCallCheck,
testBakeCallCheckFlag,
testBakeCallMetadata,
Expand Down Expand Up @@ -1737,7 +1738,93 @@ target "default" {
)
require.NoError(t, err, out)

require.Equal(t, "VARIABLE\tVALUE\tDESCRIPTION\nabc\t\t<null>\t\ndef\t\t\t\nfoo\t\tbar\tThis is foo", strings.TrimSpace(out))
require.Equal(t, "VARIABLE\tTYPE\tVALUE\tDESCRIPTION\nabc\t\t\t<null>\t\ndef\t\t\t\t\nfoo\t\t\tbar\tThis is foo", strings.TrimSpace(out))
}

func testListTypedVariables(t *testing.T, sb integration.Sandbox) {
bakefile := []byte(`
variable "abc" {
type = string
default = "bar"
description = "This is abc"
}
variable "def" {
type = string
description = "simple type, no default"
}
variable "ghi" {
type = number
default = 99
description = "simple type w/ default"
}
variable "jkl" {
type = list(string)
default = ["hello"]
description = "collection with quoted strings"
}
variable "mno" {
type = list(number)
description = "collection, no default"
}
variable "pqr" {
type = tuple([number, string, bool])
default = [99, "99", true]
}
variable "stu" {
type = map(string)
default = {"foo": "bar"}
}
variable "vwx" {
type = set(bool)
default = null
description = "collection, null default"
}

// untyped, but previously didn't have its value output
Copy link
Member

@tonistiigi tonistiigi May 27, 2025

Choose a reason for hiding this comment

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

seem to be missing a case with just variable block without type and default, and implied string cases with default.

I think it would also be ok to show the type even if it is inferred from default, as long as it is not a string. But if it complicated things then we can skip that.

Copy link
Contributor Author

@rrjjvv rrjjvv May 27, 2025

Choose a reason for hiding this comment

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

it would also be ok to show the type even if it is inferred from default

It wouldn't complicate things. The main reason I didn't is for the case of

variable "VERSIONS" {
  default = ["1.2", "1.3"]
}

A reader "knows" that it's a list(string) ("list of string") or maybe set(string) ("set of string"), but the inferred type is tuple([string, string]) ("tuple").

The inferred types for a primitive is probably going to be correct 99% of the time, but thought it better to be consistent. But it is not complicated or difficult if that's what you'd prefer.

Copy link
Member

@tonistiigi tonistiigi May 27, 2025

Choose a reason for hiding this comment

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

Why is it detected as "tuple" in this case? I would guess that an array is more generic and a tuple is a subset of array.

Copy link
Contributor Author

@rrjjvv rrjjvv May 27, 2025

Choose a reason for hiding this comment

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

seem to be missing a case with just variable block without type and default, and implied string cases with default.

Both of these were covered in an existing test (foo and def):

buildx/tests/bake.go

Lines 1716 to 1724 in e3c6618

variable "foo" {
default = "bar"
description = "This is foo"
}
variable "abc" {
default = null
}
variable "def" {
}

Copy link
Contributor Author

@rrjjvv rrjjvv May 27, 2025

Choose a reason for hiding this comment

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

Why is it detected as "tuple" in this case? I would guess that an array is more generic and a tuple is a subset of array.

The rationale is probably "since I can only see two elements both of which are strings, I can't guarantee you can safely add/remove elements at will". But more explicitly from the docs:

Only tuple and object values can be directly constructed via native syntax. Tuple and object values can in turn be converted to list, set and map values with other operations, which behaves as defined by the syntax-agnostic HCL information model.

Copy link
Member

@tonistiigi tonistiigi May 27, 2025

Choose a reason for hiding this comment

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

I think let's leave it with 1 then, unless there is some simple trick to make 4 possible

Copy link
Contributor Author

@rrjjvv rrjjvv May 27, 2025

Choose a reason for hiding this comment

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

In short, I think we'll have to go with (1); I'm not sure we'll find any truly simple solution for (4).

In hindsight, I should have never started batting around "inferred type". Hopefully I'm not responsible for any confusion. If we were truly dealing with inferred types (types inferred by how/where they're used), we probably wouldn't be having this discussion (to this extent at least). An actual inferred type would be the solution you're hoping for. We're just dealing with the simplest type that can be guaranteed to represent a specific value.

I'll take another stroll through the docs and code to see if there's a capability there that I missed, but I'm not hopeful. Probably the best we'll get is some insight or tools to make our own heuristics simpler/safer to implement. I'll let you know what I find.

Copy link
Contributor Author

@rrjjvv rrjjvv May 28, 2025

Choose a reason for hiding this comment

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

From what I can see, there is no general solution for (4) that is also simple. But if you're willing to sacrifice correctness for the 1% intending to use tuples for the sake of the 99% using lists (an earlier comment suggests you might), there could be a 'simple' (in terms of bake code) solution.

Via the cty "unification" functions, I was basically able to say "I have ["hi", "there"]; can that be made into a list of some type?" And it will (not just whether it can be done, but exactly what type).

But... given ["hello", 99], it says "it can be made into a list(string) (via coercion)". Under normal circumstances, something that superficially looks like a list but contains elements of various types would usually be considered a tuple. So increasing accuracy for list users may come at the expense of decreasing accuracy for tuple users.

There are two main variables in play specific to this functionality... 'safe' vs. 'unsafe', and whether cty.DynamicPseudoType is being utilized; I have not experimented with all combinations.

Let me know what you think... if you think this deserves more exploration, or if it's fine to leave as-is.

Copy link
Member

@tonistiigi tonistiigi May 28, 2025

Choose a reason for hiding this comment

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

Let's just leave it to 1. No need to overthink it.

Copy link
Contributor Author

@rrjjvv rrjjvv May 28, 2025

Choose a reason for hiding this comment

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

k. I guess there's another flavor of (4)... rather than try guessing the complex types, we don't even bother trying and simply fill the column with "other", "unknown", "<missing>", or similar.

That wouldn't provide any real value (though could be a subtle hint to the user that they can provide it), but would make the output look a little nicer (no gaps), would be easy, and would not be inaccurate.

If you like that option and give me an exact term to use, I can get that implemented later. Otherwise, I guess we're done?

variable "wxy" {
default = ["foo"]
description = "inferred tuple"
}
// untyped, but previously didn't have its value output
variable "xyz" {
default = {"foo": "bar"}
description = "inferred object"
}
// untyped, but previously didn't have its value output
variable "yza" {
default = true
description = "inferred bool"
}
target "default" {
}
`)
dir := tmpdir(
t,
fstest.CreateFile("docker-bake.hcl", bakefile, 0600),
)

out, err := bakeCmd(
sb,
withDir(dir),
withArgs("--list=variables"),
)
require.NoError(t, err, out)
require.Equal(t,
"VARIABLE\tTYPE\t\tVALUE\t\tDESCRIPTION\n"+
"abc\t\tstring\t\tbar\t\tThis is abc\n"+
"def\t\tstring\t\t<null>\t\tsimple type, no default\n"+
"ghi\t\tnumber\t\t99\t\tsimple type w/ default\n"+
"jkl\t\tlist of string\t[\"hello\"]\tcollection with quoted strings\n"+
"mno\t\tlist of number\t<null>\t\tcollection, no default\n"+
// the implementation for tuple's 'friendly name' is very basic
// and marked as TODO, so this may change/break at some point
"pqr\t\ttuple\t\t[99,\"99\",true]\t\n"+
"stu\t\tmap of string\t{\"foo\":\"bar\"}\t\n"+
"vwx\t\tset of bool\t<null>\t\tcollection, null default\n"+
"wxy\t\t\t\t[\"foo\"]\t\tinferred tuple\n"+
"xyz\t\t\t\t{\"foo\":\"bar\"}\tinferred object\n"+
"yza\t\t\t\ttrue\t\tinferred bool",
strings.TrimSpace(out))
}

func testBakeCallCheck(t *testing.T, sb integration.Sandbox) {
Expand Down