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

resource/logpush_job: Add support for "dataset" parameter #649

Merged

Conversation

jacobbednarz
Copy link
Member

Updates the logpush_job resource to support the "dataset" parameter
which allows different configurations for different types of data.

See https://developers.cloudflare.com/logs/logpush/logpush-configuration-api/understanding-logpush-api/

Closes #641

Updates the `logpush_job` resource to support the "dataset" parameter
which allows different configurations for different types of data.

See https://developers.cloudflare.com/logs/logpush/logpush-configuration-api/understanding-logpush-api/

Closes cloudflare#641
@ghost ghost added the size/XS label Apr 7, 2020
"dataset": {
Type: schema.TypeString,
Optional: true,
ValidateFunc: validation.StringInSlice([]string{"http_requests", "spectrum_events"}, false),
Copy link
Member Author

Choose a reason for hiding this comment

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

The API documentation has the validation has alphanumeric characters only however only "http_requests" and "spectrum_events" are listed in the developer guide. This leads me to believe that there are more data sets coming but for now we can be specific here and catch some user error before it gets too far into the processing to identify the issue.

@jacobbednarz
Copy link
Member Author

I also suspect in the future we may need to update some of the API calls to use the /datasets/:dataset/fields endpoints instead of the current blanket one too.

@ghost ghost added the kind/documentation Categorizes issue or PR as related to documentation. label Apr 13, 2020
@jacobbednarz jacobbednarz merged commit d930aaa into cloudflare:master Apr 13, 2020
@jacobbednarz jacobbednarz deleted the add-dataset-to-logpush-payload branch April 13, 2020 21:57
@jamesarosen
Copy link

Is it possible that this was a breaking change? If it's a new required field, it should go out in 3.0.

We have this resource:

resource "cloudflare_logpush_job" "everlane-com" {
  enabled             = true
  zone_id             = cloudflare_zone.example-com.id
  name                = "request-logs"
  logpull_options     = "fields=${join(",", local.logpush_fields)}&timestamps=rfc3339&sample=${local.logpush_sample_rate}"
  destination_conf    = "s3://${aws_s3_bucket.example-cloudflare-logs.bucket}/logs/?region=us-east-1"
  ownership_challenge = "this value is irrelevant after initial setup, but required for Terraform"
}

On my computer, with terraform-provider-cloudflare_v2.4.0_x4, terraform plan works fine.

On @antonhalim's computer, with terraform-provider-cloudflare_v2.8.0_x4, terraform plan results in

Error: Missing required argument
  on cloudflare_logs.tf line 84, in resource "cloudflare_logpush_job" "everlane-com":
  84: resource "cloudflare_logpush_job" "everlane-com" {
The argument "dataset" is required, but no definition was found.

@jacobbednarz
Copy link
Member Author

jacobbednarz commented Jul 7, 2020

I don't see this as a backwards-incompatible change; we didn't remove/alter functionality that would prevent systems from working. The change in question was always the implicit value however the API now offers multiple values so an explicit definition is required. We could have opted to make this optional however I feel that would have introduced more bugs as people accidentally creating logpush jobs with the wrong dataset value. The definition itself is not that widely used in Terraform as it is as we query by resource ID instead but that won't always be the case.

If you cannot add the resource parameter, you can pin to an older version of the provider until you have the capability to upgrade to support this parameter.

boekkooi-lengoo pushed a commit to boekkooi-lengoo/terraform-provider-cloudflare that referenced this pull request Feb 28, 2022
…ctions/goreleaser/goreleaser-action-2.6.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Categorizes issue or PR as related to documentation. size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cloudflare_logpush_job API should contain "dataset" argument
2 participants