Fix crash when an unset variable is used

[azr]

This was due to a misunderstanding on my side. When a variable is not set one needs to set it to cty.NullVal(cty.DynamicPseudoType) , like so:

packer/hcl2template/types.variables.go

Lines 57 to 93 in 23b940d

	func (v *Variable) Value() (cty.Value, *hcl.Diagnostic) {
	for _, value := range []cty.Value{
	v.CmdValue,
	v.VarfileValue,
	v.EnvValue,
	v.DefaultValue,
	} {
	if !value.IsNull() {
	return value, nil
	}
	}
	value := cty.NullVal(cty.DynamicPseudoType)
	return value, &hcl.Diagnostic{
	Severity: hcl.DiagError,
	Summary: fmt.Sprintf("Unset variable %q", v.Name),
	Detail: "A used variable must be set or have a default value; see " +
	"https://packer.io/docs/configuration/from-1.5/syntax.html for details.",
	Context: v.block.DefRange.Ptr(),
	}
	}
	type Variables map[string]*Variable
	func (variables Variables) Values() map[string]cty.Value {
	res := map[string]cty.Value{}
	for k, v := range variables {
	res[k], _ = v.Value()
	// here the value might not be used and in that case we don't want to
	// force users to set it. So this error is ignored. we still set it as
	// it can be a `cty.NullVal(cty.DynamicPseudoType)`, which is the go
	// cty value for 'unknown value' and should error when used.
	}
	return res
	}

I choose not to error when a variable is not set and not used. But when someone does something like this:

variable "proxmox_username" {
  type = string
}

build {
  sources = [
    "source.null.null-builder${var.proxmox_username}",
  ]
}


source "null" "null-builder" {
}

The error is the following:

  on .vscode/hcl2/crash/var_with_no_default.pkr.hcl line 8, in build:
   8:     "source.null.null-builder${var.proxmox_username}",

Variables may not be used here.

Error: Unsuitable value type

  on .vscode/hcl2/crash/var_with_no_default.pkr.hcl line 7, in build:
   7:   sources = [
   8:     "source.null.null-builder${var.proxmox_username}",
   9:   ]

Unsuitable value: value must be known

fix #8730

[sylviamoss]

LGTM! 🎉

[azr]

Question for the team. With this PR a variable can be defined and have no value.But this would be different that for Terraform; so quoting this message:

By default, a Terraform variable is required and so Terraform would emit an error if it isn't set. An author can make a variable optional by providing a default value.

variable "example" {
  type    = string
  default = "Hello"
}

In some cases there isn't any reasonable default value, and the module simply wants to detect whether or not the value is set. In that case, the author can specify the default as null to say that the variable is optional but it has no default:

variable "example" {
  type    = string
  default = null
}

Should we do the same ?

[sylviamoss]

Should we do the same ?

I think as a terraform user the one might expect to be the same. In terraform if the variable without a default is used, is it going to return an error as your implementation does?

I like the way you did and I think both solutions are good. If it's not too difficult, I would do it as terraform just for consistency. Maybe just add the option of setting default = null without removing what you did.

[nywilken]

💯

[nywilken]

Is this so that we can keep track of the block when generating diagnostics?

[azr]

yes !

[azr]

My pr was merged; so this can be removed.

[azr]

Hey folks ! I think this PR is good to review. I think the behaviour are super close to what we want in the RFC. But since this also fixes a crash I think we should merge it.

I'll gladly update the behaviours if/when the RFC changes later on 🙂.

[SwampDragons]

Did you mean "sensitive" here instead of "sensible"?

[azr]

yes !

[nywilken]

💯 👍 🏆 Nicely done on adding the variables table.

[azr]

thanks :D

[ghost]

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.