hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io/
Other
42.53k stars 9.52k forks source link

variable_validation error_message restrictions should be simplified #24214

Closed alexjurkiewicz closed 2 years ago

alexjurkiewicz commented 4 years ago

I'd like to write an error message like:

variable user_name {
  type        = string
  description = "User to create"

  validation {
    condition     = length(var.user_name) <= 64
    error_message = "user_name maximum length is 64."
  }
}

But Terraform reports:

Validation error message must be at least one full English sentence starting with an uppercase letter and ending with a period or question mark.

So I have to rework to the more awkward:

  validation {
    condition     = length(var.user_name) > 64
    error_message = "Maximum length of user_name is 64."
  }

The smallest tweak to improve this situation would be to allow error_message to start with the variable name.

artburkart commented 4 years ago

I would also like to see these restrictions removed. I want to write a validation with the following format:

variable "foo" {
  type = string
  description = "Some valid description goes here."
  default = "a"
  validation {
    condition = contains(["a", "b", "c"], var.foo)
    error_message = <<EOF
The value of var.foo must be one of the following:
- "a"
- "b"
- "c"
EOF
  }
}

I would also like to be able to write the following:

variable "foo" {
  type = string
  description = "Some valid description goes here."
  default = "a"
  validation {
    condition = contains(["a", "b", "c"], var.foo)
    error_message = <<EOF
The value of var.foo must be "a", "b", or "c".
EOF
  }
}
iancward commented 3 years ago

I vote that the restriction be removed. Why must it be English and why must it be a proper sentence?

I also want to be able to write a message like @artburkart: tell the user is invalid and specify a list of correct values. I even wrote something like "Variable is invalid. Must be one of..." in a heredoc and Terraform still didn't like it (even though that meets the criteria specified in the error message).

snazzyfox commented 3 years ago

Agree with completely removing this restriction. It can force English-speaking developers to phrase the error in a less readable way.

But not all developers prefer English. It should not be Terraform's business to dictate and enforce that a certain language must be used. The language of the error message should depend on its intended audience. When the author and users of a module don't understand English well, forcing them to write in English can only lead to badly-written (or often completely wrong) messages that can't be understood by anyone. IMO this is much worse than simply allowing them to use their preferred language.

andreykaipov commented 3 years ago

Ran into this just now. I have several validations within a nested object that I'm checking by using alltrue, and the most natural way to report the error feels like a list. For example:

  validation {
    condition = alltrue(flatten(concat(
      [for spec in var.replication_specs : length(spec.regions) <= 7],
      [for spec in var.replication_specs : contains([3, 5, 7], sum([for r in spec.regions : r.nodes]))],
      [for spec in var.replication_specs : [for r in spec.regions : 1 <= r.nodes && r.nodes <= 5 ]],
    )))
    error_message = <<EOF
Make sure all of the following conditions are satisfied:
- no more than 7 regions in any zone
- total nodes in a zone must add to up 3, 5, or 7
- nodes in a region must be within the range [1,5]
EOF
  }
bcline760 commented 3 years ago

BTW, this worked

variable "name" {
  type        = string
  validation {
    condition     = can(regex("^([a-zA-Z0-9])+$", var.name))
    error_message = "¡Oye! No puedes tener un nombre con los caracteres que no son los numeros o las letras."
  }
}

As did this...

variable "name" {
  type        = string
  validation {
    condition     = can(regex("^([a-zA-Z0-9])+$", var.name))
    error_message = "Écoutez-moi! Vous avez échoué avec le nom. Vous ne pouvez que avoir les noms avec les nombres et les lettres."
  }
}

So it may not be as stringent as we think. I would like to see someone test who speaks like Arabic, Russian, Japanese (i.e. non-Roman writing) to see if it fails for them too.

apparentlymart commented 3 years ago

Hi all,

You're right to say that the error message here is imprecisely written (my fault) in that it's trying to present two ideas at the same time, even though they are actually distinct:

With all of that said, I will propose a change to separate the suggestion of using English in broadly-distributed modules from the requirement of using prose sentences, because that's clearly wrong and was a result of over-zealous text editing on my part. The other concerns here about changing the model of the error messages we'll need to keep separate, because they have potential impacts on other subsystems that our outside of our team's direct influence and therefore require broader discussion.

Thanks for pointing this out! I'm working on a release today so I won't be able to jump on this immediately, but I will send in a PR for a message text edit soon, and then we can get it included in the forthcoming v0.15.0 release.

alexkokkinos commented 3 years ago

An edge case with the current behavior is that a sentence like the one below is grammatically correct English, but fails validation:

Please set the variable to be either \"Staging\" or \"Production.\"

If you end a sentence with a quote, the period goes inside the closing quotation mark.

iainelder commented 3 years ago

Was just surprised by this today :smile:

@apparentlymart, I agree that it's good style, and we should do everything to encourage it.

But I think it's too far for a style deviation to invalidate the configuration.

If this feature must be kept, can we at least improve the documentation?

Today this is all it has to say:

The error message string should be at least one full sentence explaining the constraint that failed, using a sentence structure similar to the above examples.

What does "similar" mean? The error message produced by Terraform is clearer and could replace this phrase in the documentation.

What will it look like in context?

The text you give here is going to be printed out as part of a larger Terraform error message

This is how I see it presented today in Terraform v0.14.9:

Error: Invalid value for variable

  on variables.tf line 5:
   5: variable "profile" {

Must be "blah-profile-dev" or "blah-profile-prod".

This was checked by the validation rule at variables.tf:7,5-15.

The message is floating in ethereal whitespace and could be a fragment with no loss of clarity. If it were prefixed with "Validation message: ", I think it would be fine to accept whatever comes next as being understood in the context by the user (mathematical notation, pseudocode?).

@alexkokkinos raises a good point: which English?

In Alex's example the rules would fit British punctuation rules but not American. I imagine American English is the most popular variant among Terraform users.

https://www.thepunctuationguide.com/british-versus-american-style.html

iainelder commented 3 years ago

All the examples we've given here describe the "what" and not the "why".

They are entirely redundant, as the condition already describes the "what" more precisely than English ever can.

I think the description field already meets the requirements for describing the "why".

We could avoid the whole style debate for "what" by generating error_message value with a condition parser.

007 commented 3 years ago

Came across this today in 0.14.x.

With the existing restriction "Dafuq?" is a valid error_message but "The value must match one of: [\"AWS\"]" is not.

Please, drop this restriction and compensate for it appropriately at the time of message display. Wrap it in quotes or \n or -=<{ ... }>=- or whatever needs to happen to make the message intelligible in context.

solarmosaic-kflorence commented 2 years ago

See also https://github.com/hashicorp/terraform/issues/24123

jhancock93 commented 2 years ago

It would be great if the error would either include the value supplied that was invalid or allow the error_message to support string interpolation so that the value may be included. While often the variable can be inferred from the error, when using for_each to create many resources from a map of inputs, the current error message supplied by terraform variable validation is not enough to indicate which of the resources includes the invalid input.

alisdair commented 2 years ago

Thanks for all the feedback here.

Terraform 1.2 includes new custom condition checks: resource preconditions and postconditions, and output preconditions. As part of this work, we added support for interpolation in the error message, and removed the formatting restrictions.

We still recommend adhering to the Terraform language style which was previously strictly enforced, but this is no longer an error.

I tried several of the examples in this thread and verified that they now work as expected. For example:

$ terraform plan -var foo=d
╷
│ Error: Invalid value for variable
│
│   on main.tf line 1:
│    1: variable "foo" {
│     ├────────────────
│     │ var.foo is "d"
│
│ The value of var.foo must be one of the following:
│ - "a"
│ - "b"
│ - "c"
│
│ This was checked by the validation rule at main.tf:5,3-13.
╵
github-actions[bot] commented 2 years ago

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.