Skip to content
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

hcl2template: don't use it's var type when the default value for a variable is empty #11559

Closed
wants to merge 1 commit into from

Conversation

azr
Copy link
Contributor

@azr azr commented Feb 10, 2022

In HCL, when no type is given for a var, the type of the default value will be used. But the type of default = {} is the EmptyObject.

Converting something in HCL to the EmptyObject or EmptyTuple types will return a new empty object. Meaning that if we try to set these later on, the result will remain something empty.

This potentially fixes #11551

  • fix
  • tests

…riable is empty

* converting something in HCL to the EmptyObject or EmptyTuple types will return a new empty object.
@azr azr requested a review from a team as a code owner February 10, 2022 16:14
@azr azr marked this pull request as draft February 10, 2022 16:14
@azr azr added the bug label Feb 10, 2022
@nywilken
Copy link
Contributor

In theory I like the approach but I do wonder if the right thing to do here is error instead of trying EmptyObject and EmptyTuple as a non-type. I was curious how this would work if I made the default an EmptyObject and assigned it a number.
I would expect it to fail because of a type mismatch like we do for other.

~>  packer inspect /tmp
Packer Inspect: HCL2 mode

> input-variables:

var.bar: "1"
var.foo: "{\n  \"key\" = \"---\"\n}"

> local-variables:


> builds:
// test.auto.pkrvars.hcl
bar = 1

// test.pkr.hcl
variable "foo" {
  default = {
    "key" : "---"
  }
}

variable "bar" {
  default = {}
}

locals {
  baz = merge(var.foo, var.bar)
}

@azr
Copy link
Contributor Author

azr commented Feb 11, 2022

Yeah, that is a super valid point. I opened zclconf/go-cty#122 to see if this could be a good idea.

@azr
Copy link
Contributor Author

azr commented Feb 11, 2022

Note that for Terraform here, the default type is always ignored, so your example would just work.

@nywilken
Copy link
Contributor

Note that for Terraform here, the default type is always ignored, so your example would just work.

Yeah, I def. thought about that. TBH I think users would know better what to expect here. So I'm good with merging. If users find anything confusing we can revisit. Unless you prefer to hold until you get a response from the go-cty repo.

@azr azr marked this pull request as ready for review February 14, 2022 09:35
@azr
Copy link
Contributor Author

azr commented Feb 14, 2022

Hey @nywilken, yup, I think this can be merged. I need to add tests first !

The TLDR is that the conversion to a type only ensures that the receiving type has all attributes of the 'being-converted' object, so, this is intended and will always work. So for now, I think this PR ignoring these makes sense.

The next solution will be like I said in the issue to :

do like Terraform, and avoid using the default value's type as the type

(which changes behavior a little, but it's okay, because in any case, it is always better to set the type for an input var)

@azr
Copy link
Contributor Author

azr commented Feb 14, 2022

Closing this one in favor of #11566 to avoid conflict

@azr azr closed this Feb 14, 2022
@azr azr deleted the azr/fix_default_null_object_type_issues branch February 14, 2022 13:25
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't merge maps with heterogeneous values and use types any or map(any)
2 participants