Show types during variable list operation #3207
Conversation
There was a problem hiding this comment.
Thanks! If you could also update docs at
buildx/docs/reference/buildx_bake.md
Lines 197 to 205 in e3c6618
bake/hclparser/hclparser.go
Outdated
| } | ||
| } | ||
| case vv.Type().IsPrimitiveType(): | ||
| // all primitive's can convert to string, so error should ever occur anyway |
There was a problem hiding this comment.
A nit for you, but embarrassing for me 😄. Fixed. For future reference, let me know if you prefer these fixes as new commits or squashed/force-pushed.
| description = "collection, null default" | ||
| } | ||
|
|
||
| // untyped, but previously didn't have its value output |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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):
Lines 1716 to 1724 in e3c6618
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think let's leave it with 1 then, unless there is some simple trick to make 4 possible
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Let's just leave it to 1. No need to overthink it.
There was a problem hiding this comment.
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?
If a type was explicitly provided, it will be displayed in the variable listing. Inferred type names are not displayed, as they likely would not match the user's intent. Previously only `string` a A832 nd `bool` default values were displayed in the listing. All default values, regardless of type, are now displayed. Signed-off-by: Roberto Villarreal <rrjjvv@yahoo.com>
If a type was explicitly provided, it will be displayed in the variable listing. Inferred type names are not displayed, as they likely would not match the user's intent.
Previously only
stringandbooldefault values were displayed in the listing. All default values, regardless of type, are now displayed.Fixes #3202