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

[Connections] Consider null and unknown as valid in url string validator #1942

Merged
merged 6 commits into from
Dec 6, 2024

Conversation

fridgepoet
Copy link
Member

@fridgepoet fridgepoet commented Dec 6, 2024

An issue was raised that string validation was returning an error on the result of each.value from for_each. In the following example, existing string validation was rejecting each.value because it is null/unknown.
This PR allows for null/unknown values in our string validation and adds tests using the for_each.

Example:

locals {
  jobs = [
    { name = "test", url = "<some valid url>" }
  ]
}

resource "grafana_connections_metrics_endpoint_scrape_job" "valid_url" {
  for_each = { for j in local.jobs : j.name => j.url }
  stack_id = "1"
  name = each.key
  enabled = true
  authentication_method = "bearer"
  authentication_bearer_token = "<some valid token>"
  url = each.value
}

Running terraform plan before would result in:

╷
│ Error: value must be valid URL with HTTPS
│
│   with grafana_connections_metrics_endpoint_scrape_job.valid_url,
│   on main.tf line 19, in resource "grafana_connections_metrics_endpoint_scrape_job" "valid_url":
│   19:   url = each.value
│
│ A valid URL is required.
│
│ Given Value: ""
│ 

Running terraform plan with the changes in this PR now results in:

Terraform will perform the following actions:

  # grafana_connections_metrics_endpoint_scrape_job.valid_url["test"] will be created
  + resource "grafana_connections_metrics_endpoint_scrape_job" "valid_url" {
      + authentication_bearer_token = (sensitive value)
      + authentication_method       = "bearer"
      + enabled                     = true
      + id                          = (known after apply)
      + name                        = "test"
      + scrape_interval_seconds     = 60
      + stack_id                    = "1"
      + url                         = "<some valid url>"
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Applying is successful and one can see the job in the UI:
Screenshot 2024-12-06 at 11 44 27

Fixes https://github.com/grafana/support-escalations/issues/13779

Copy link

github-actions bot commented Dec 6, 2024

In order to lower resource usage and have a faster runtime, PRs will not run Cloud tests automatically.
To do so, a Grafana Labs employee must trigger the cloud acceptance tests workflow manually.

@fridgepoet fridgepoet marked this pull request as ready for review December 6, 2024 10:50
@fridgepoet fridgepoet requested review from a team as code owners December 6, 2024 10:50
@fridgepoet fridgepoet changed the title [Connections] consider null and unknown as valid in url string validator [Connections] Consider null and unknown as valid in url string validator Dec 6, 2024
}
`

var resourceWithForEachInvalidURL = `
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So with the early break on IsNull or IsUnknown, we still do the validation at a later point?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Duologic
Copy link
Member

Duologic commented Dec 6, 2024

I think the tests are fixed after a rebase.

Copy link
Contributor

@matthewnolf matthewnolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fridgepoet fridgepoet merged commit 0cb6577 into main Dec 6, 2024
26 checks passed
@fridgepoet fridgepoet deleted the shirley/null-unknown-valid branch December 6, 2024 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants