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

provider::assert::regex Invalid value for "s" parameter: argument must not be null #70

Open
dustindortch opened this issue Oct 23, 2024 · 6 comments
Assignees
Labels
under-consideration Indicates that the topic or suggestion is actively being reviewed and considered for implementation.

Comments

@dustindortch
Copy link

With the intent of this being for validation, assertions, etc... and the fact that Terraform doesn't support short-circuiting for conditional statement... the behavior of regex() (and all other functions in assert) should be to just return false when a null value is passed in (aside from null() and not_null()).

The real value of variable validation is that it cleans up having to do "defensive programming" techniques in the argument expressions. The current behavior requires doing these techniques within the validation, though. These should just be simple statements.

@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer label Oct 23, 2024
@bschaatsbergen bschaatsbergen removed the needs-triage Waiting for first response or review from a maintainer label Oct 28, 2024
@bschaatsbergen
Copy link
Member

bschaatsbergen commented Oct 28, 2024

Hey @dustindortch,

Thanks for raising this issue! I can see how, with validation and assertions in mind, having regex() and other functions in assert return false on null input (aside from null() and not_null()) would simplify things. Your point about reducing the need for defensive programming within validation expressions is helpful.

At the same time, sticking with Terraform core’s evaluation style for built-in functions helps maintain consistency and reduces potential confusion across similar components in the ecosystem.

For instance, here’s a similar function call from Terraform core:

$ terraform apply -auto-approve

│ Error: Invalid function argument

│   on main.tf line 2, in output "value":
│    2:  value = regex("", null)
│     ├────────────────
│     │ while calling regex(pattern, string)

│ Invalid value for "string" parameter: argument must not be null.

Using the Assert provider, the following code will produce the same error if no value is set for the user_email variable.

variable "user_email" {
  description = "The email address for the user."
  type        = string
  default     = null

  validation {
    condition     = provider::assert::regex("^[^@]+@[^@]+\\.[^@]+$", var.user_email)
    error_message = "The user email must be in a valid format, such as '[email protected]'."
  }
}

Alternatively, you could modify the condition to check whether the variable is initialized as null.

variable "user_email" {
  description = "The email address for the user."
  type        = string
  default     = null

  validation {
    condition     = var.user_email != null ? provider::assert::regex("^[^@]+@[^@]+\\.[^@]+$", var.user_email) : true
    error_message = "The user email must be in a valid format, such as '[email protected]'."
  }
}

I agree that returning false for null values that would otherwise error would enable cleaner implementations. I’ll gather input from a few peers on how we should standardize this for the provider. Thank you for your patience and for raising this issue.

@bschaatsbergen bschaatsbergen added the under-consideration Indicates that the topic or suggestion is actively being reviewed and considered for implementation. label Oct 28, 2024
@dustindortch
Copy link
Author

What is the purpose of this provider? I have seen several providers out there that seem to be used as libraries for functions. So if the intent is to offload the functions from the core, that would make sense to keep it the same. However, if the purpose is to have a library of functions useful for assertions, then there doesn't seem to be value in making them work the same as their counterparts in the core. If they're the same, why not just use the functions from the core and avoid even having a provider. Or, at least, that is my opinion on the matter.

@bschaatsbergen
Copy link
Member

What is the purpose of this provider? I have seen several providers out there that seem to be used as libraries for functions. So if the intent is to offload the functions from the core, that would make sense to keep it the same. However, if the purpose is to have a library of functions useful for assertions, then there doesn't seem to be value in making them work the same as their counterparts in the core. If they're the same, why not just use the functions from the core and avoid even having a provider. Or, at least, that is my opinion on the matter.

Thank you for sharing your feedback and perspective @dustindortch. The purpose of this provider is primarily to extend functionality beyond what’s available in the core. While some functions may appear similar, the intent is to provide additional features and flexibility that improve the overall experience making variable validation, continuous validation, and testing easier.

The Assert provider functions complement Terraform’s built-in functions rather than replacing them. If Terraform’s built-in functions better fit your requirements, they should be your choice.

As I mentioned earlier, we greatly appreciate your feedback and are actively considering your proposal for a more “defensive style” of writing assertions, as you’ve outlined in the issue. Thanks again!

@bschaatsbergen
Copy link
Member

bschaatsbergen commented Nov 1, 2024

@dustindortch, for the regex function with parameters pattern and s, we currently return false if s is null. However, how would you like us to handle a null pattern?

Would you prefer an explicit error in this case, or should we suppress the error and return false as well? This situation is a bit nuanced since pattern relies on a Go standard library function, and its behavior with null/nil values may not be entirely predictable—it might support a null/nil pattern, or it might not.

Note: While regex is the example here, this applies to any parameter that relies on library code which might or might not handle a null/nil value.

@bschaatsbergen bschaatsbergen self-assigned this Nov 1, 2024
@dustindortch
Copy link
Author

@bschaatsbergen I think that is the right way to handle it. It is failing for some reason. Let me try a couple of other things, similar to the null() function to see if there is any interference from other factors. Thank you.

@bschaatsbergen
Copy link
Member

bschaatsbergen commented Nov 4, 2024

A concern that popped up when evaluating and implementing is that the proposed behavior could significantly affect the invert operator, potentially causing false positives.

For example, let’s say we want to check if an endpoint does not start with “http”. We’d write an assertion like this:

!provider::assert::starts_with("http://", var.some_endpoint)

Here, the invert operator is applied at the beginning. But if var.some_endpoint is null, the function would return false, which then gets inverted—causing the test to pass unexpectedly, resulting in a false positive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
under-consideration Indicates that the topic or suggestion is actively being reviewed and considered for implementation.
Projects
None yet
Development

No branches or pull requests

2 participants