repl: Improved value renderer for console outputs

[alisdair]

Use a slightly modified value renderer from terraform-provider-testing to display values in the console REPL, as well as outputs from the apply and outputs subcommands.

Derived from code in this repository, MIT licensed, as suggested by Martin. Hope this is what he meant!

Note that this is technically a breaking change for the console subcommand, which would previously error if the user attempted to render an unknown value (such as an unset variable). This was marked as an unintentional side effect, with the goal being the new behaviour of rendering "(unknown)", which is why I changed the behaviour in this commit.

Fixes #26099. Arguably fixes #25798 as well, I think.

Screenshot

With this configuration:

locals {
  object = {
    "foo" : {
      "x" : null,
      "y" : null
    }
    "bar" : {
      "x" : 2,
      "y" : 3
    }
  }
}

variable "animals" {
  type = list(string)
  default = [
    "cat",
    "dog",
    "marmot",
    "possum",
    "tortoise",
  ]
}

variable "integers" {
  type = set(number)
}

variable "versions" {
  type    = map(string)
  default = null
}

[codecov]

Codecov Report

Merging #26189 into master will increase coverage by 0.11%.
The diff coverage is 98.82%.

Impacted Files	Coverage Δ
repl/format.go	98.80% <98.78%> (+87.38%)	⬆️
command/apply.go	42.59% <100.00%> (+0.12%)	⬆️
command/output.go	48.07% <100.00%> (+0.65%)	⬆️
repl/session.go	88.23% <100.00%> (+12.04%)	⬆️
command/console.go	38.88% <0.00%> (-2.78%)	⬇️
terraform/evaluate.go	52.95% <0.00%> (+0.42%)	⬆️

[mildwonkey]

👏 Lovely!

[alisdair]

@apparentlymart No rush for this at all, but I wanted to ask for a second review from you, since this is both your idea & mostly your code 😅

[apparentlymart]

This looks good to me. Thanks!

The old formatter had a few special exceptions, like showing standalone string values raw rather than quoted, but I always found those exceptions rather confusing so I'm favor of removing them here and being consistent. However, I will note that this does create some risk of users being confused by it in a similar way as we sometimes see for the terraform plan output: not everyone seems to understand that Terraform is using (something close to) its own string quoting syntax and thinks that any escape characters are literal. That hasn't come up incredibly often, so I don't think we need to take this as a design constraint, but if we see it coming up more after we release this (not yet) it may behoove us to find a suitable place to write some docs on how to understand the output, which we can then refer users to when they ask questions about it.

I also find the big block license comments at the top of the files annoying, so I'm going to say it's fine to take this into Terraform without the explicit license statements, even though that contradicts the letter of the MIT license text in my other codebase, so that this can just be a contribution under the normal HashiCorp CLA terms, rather than an odd exception.

[apparentlymart]

I'd suggest changing this to (known after apply) to match with the terraform plan output. We've seen in the past that there are unfortunately a few other edge-cases where things appear as unknown in the console that are not caused by something that would be set after apply, but I believe we addressed the most common of those recently by making an exception to allow the impure (and thus, non-convergent) functions like timestamp to be evaluated in the console.

[apparentlymart]

Note mostly to myself: I don't actually recall what cty change I was imagining making here, or why this wouldn't be possible to implement already today. 🤔

It's likely that including a full object type in full detail here would be pretty verbose in many cases, but I guess not more verbose than a non-null value of that type would be, and so perhaps what I was anticipating implementing was a type pretty-printer? If so I'm second-guessing that being cty thing now, because we'd likely rather it produce Terraform's type constraint syntax, which lives at the HCL level of abstraction rather than cty.

[alisdair]

This is one of the changes I made from your original code, and the comment is mine, which is why you don't remember making it 😅

I was referring to this TODO in cty:

func (t typeObject) FriendlyName(mode friendlyTypeNameMode) string {
	// There isn't really a friendly way to write an object type due to its
	// complexity, so we'll just do something English-ish. Callers will
	// probably want to make some extra effort to avoid ever printing out
	// an object type FriendlyName in its entirety. For example, could
	// produce an error message by diffing two object types and saying
	// something like "Expected attribute foo to be string, but got number".
	// TODO: Finish this
	return "object"
}

I'll update the comment to remove the reference to cty here.

[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.