feat: validate user input values the same way as "default"

[Emyrk]

[Emyrk]

@johnstcn @matifali should this pass a template import?

Before this PR, this would pass a terraform plan.

data "coder_parameter" "region" {
  name        = "region"
  description = "Which region would you like to deploy to?"
  type        = "string"
  order       = 1

  option {
    name  = "Europe"
    value = "eu"
  }
  option {
    name  = "United States"
    value = "us"
  }
}

EDIT: Test case here https://github.com/coder/terraform-provider-coder/blob/2d51bff8064a2b44b087fcadae36ce937d2cd2d8/provider/parameter_test.go#L712-L712

This PR sets the error to:

Planning failed. Terraform encountered an error while generating this plan.

╷
│ Error: Value must be a valid option. The value is empty, did you forget to set it with a default or from user input?
│ 
│   with data.coder_parameter.region,
│   on main.tf line 21, in data "coder_parameter" "region":
│   21: data "coder_parameter" "region" {
│ 
│ the value "" must be defined as one of options

To fix this, either the value must be passed via an env var:

CODER_PARAMETER_c697d2981bf416569a16cfbcdec1542b5398f3cc77d2b905819aa99c46ecf6f6=us terraform plan

Or with a default:

data "coder_parameter" "region" {
  # ...
  default = "us"
  # ...
}

---

I feel like we should reject it? But that is a breaking change.

[matifali]

There are use cases where an admin may not want to set a default to force the user to choose an option. At least it was possible before this PR. I am not sure how many users were actually using it.

But I have seen requests to support coder_parameter without a default value better: https://github.com/coder/terraform-provider-coder/issues/144

[johnstcn]

There are use cases where an admin may not want to set a default to force the user to choose an option.

This behaviour is problematic because you could end up with a plan that does not correspond to the apply, depending on how the value of the parameter is used.

Would marking the parameter as ephemeral be a possible workaround here?

EDIT: marking a parameter as ephemeral requires it to be mutable and to also have a default set. This may not suit certain use-cases.

[matifali]

Do we require a user input if the parameters are ephemeral? Do we not allow setting a default value for such parameters? Do such ephemeral parameters support options too?

If the answers are yes, then we just need better docs on how to fulfill this use case.

So that people don't try to use the existing non ephemeral parameters.

[Emyrk]

@matifali @johnstcn

How I see it is the relaxed validation on template import is incorrect. If a parameter has 0 values, but will have some value at workspace create, then the terraform plan could be incorrect.

However, I understand the use case. The provider SDK we are using does not indicate anywhere if terraform plan vs terraform apply is being run. So there is no way to conditionally enforce validation that way.

My gut thought is that we need to effectively have 2 modes of validation. 1 for template import, 1 for workspace create, where the latter is strict. The strict validation is the most correct.

What if we pass through an env var a relaxed validation mode? coder/coder can pass CODER_VALIDATION=relaxed when doing a template import. Then the provider can accept null values.

[johnstcn]

What if we pass through an env var a relaxed validation mode? coder/coder can pass CODER_VALIDATION=relaxed when doing a template import. Then the provider can accept null values.

I think this approach could work, but we'd want to perhaps tie its behaviour specifically to what "operation" we're doing in Coder. We might also want to be wary of folks just setting this env var on the provisioner to work around null values entirely.

[Emyrk]

I think this approach could work, but we'd want to perhaps tie its behaviour specifically to what "operation" we're doing in Coder. We might also want to be wary of folks just setting this env var on the provisioner to work around null values entirely.

Yes, template import would have to pass some env var to change the behavior.

As for folks setting this env var at home, I think that would just be on them. By default the validation should be handled correctly in coder (when to be relaxed, and when not)

[spikecurtis]

As for folks setting this env var at home, I think that would just be on them. By default the validation should be handled correctly in coder (when to be relaxed, and when not)

If provisionerd always sets the environment variable before exec'ing terraform, then I don't think we need to worry about users setting it.

[Emyrk]

It seems very odd to me that in this PR, template-import mode, which is supposedly being created for back-compatibility is more strict than what you're calling default mode.

Shouldn't it be the other way around? That template-import mode skips some checks that would break backward compatibility?

Maybe I should have broken this into 2 PRs. My goal is to make existing imported templates backwards compatible. They will function as they used to.

Future imported templates I am closing some of the validation holes.

And the backwards feature I am keeping, is allowing empty an default value. So this is still valid:

data "coder_parameter" "region" {
  name        = "region"
  description = "Which region would you like to deploy to?"
  type        = "string"
  order       = 1

  option {
    name  = "Europe"
    value = "eu"
  }
}

The other changes I made are fixing validation bugs. Meaning validation used to be skipped on present values.

---

Validating input values is in the option set

This is in coder/coder. This change shifts some of the validation in coder/coder -> terraform-provider-coder.

This is now invalid except at template-import, since no input values are given. So at template-import, there is no change in behavior here.

# Using input value 3
# CODER_PARAMETER_f3c7807d475073ba009bf4801b2d934e9f0126cb96dd19a27dbffcae23a7f5a3=3 terraform plan

data "coder_parameter" "numbers" {
  name        = "numebrs"
  type        = "number"
  order       = 1

  option {
    name  = "Five"
    value = "5"
  }
}

Validating option values

This used to pass, and it will continue to pass create so that previous templates still build if they have been imported. I cannot break existing imported templates.

For future template-imports, I throw an error. I think this fix can be seen as a bug fix. Otherwise, dropdown options in the UI form will fail when you select them.

data "coder_parameter" "numbers" {
  name        = "numebrs"
  type        = "number"
  order       = 1
  default = 7

  validation {
    min = 6
    max = 10
    error = "The number must be between 6 and 10"
  }

  option {
    name  = "Seven"
    value = "7"
  }

  option {
    name  = "Four"
    value = "4"
  }
}

│ Error: Option "Four": Invalid parameter value according to 'validation' block
│ 
│   with data.coder_parameter.numbers,
│   on main.tf line 10, in data "coder_parameter" "numbers":
│   10: data "coder_parameter" "numbers" {
│ 
│ The number must be between 6 and 10

[Emyrk]

If provisionerd always sets the environment variable before exec'ing terraform, then I don't think we need to worry about users setting it.

I agree, I was asked this in a voice chat. It can be set by users, but I don't imagine they will.

[Emyrk]

I removed the idea of 2 validation modes. There is only 1. option values are not validated, just as they were previously skipped.

The new validation mode is backwards compatible in all intended behavior.