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

Make ownership_challenge argument optional (or documented) for cloudflare_logpush_job resources #1010

Closed
taurelius opened this issue Mar 31, 2021 · 1 comment · Fixed by #1048
Labels
kind/enhancement Categorizes issue or PR as related to improving an existing feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@taurelius
Copy link
Contributor

taurelius commented Mar 31, 2021

Current Terraform version

0.14.8

Description

Currently the ownership_challenge argument is required when creating a cloudflare_logpush_job resource. However, it is not a required field (in the API) when creating a Logpush job with a destination of either Datadog or Splunk.

From the docs:

Note: Unlike configuring Logpush jobs for AWS S3, GCS, or Azure, there is no ownership challenge when configuring Logpush to Datadog.

I can see the thinking here, the majority of destinations do require an ownership challenge, though it means when creating a Datadog job a dummy value needs to be provided e.g. 0.

I think either the argument should be made optional and a note added that it's required for the majority of use-cases, or left as is calling out the workaround for Datadog and Splunk. I'm more than happy to raise a PR for either of these.

The Cloudflare API docs for this also seem to be incorrect, again calling out the destination_challenge field as required (I've checked and verified it isn't) not matching up with the Datadog integration guide linked above.

Use cases

Creating a cloudflare_logpush_job with a destination of either Datadog or Splunk

Potential Terraform configuration

N/A

References

@taurelius taurelius added kind/enhancement Categorizes issue or PR as related to improving an existing feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 31, 2021
@taurelius taurelius changed the title Make ownership_challenge optional (or documented) for cloudflare_logpush_job resources Make ownership_challenge argument optional (or documented) for cloudflare_logpush_job resources Mar 31, 2021
@jacobbednarz
Copy link
Member

Sorry for the radio silence on this one @taurelius, it fell off my radar.

I'm 👍 to making the schema attribute optional and instead enforcing the validation in the Create/Update operations themselves looking at the destination_conf value. Something like the following would suffice:

destConf := d.Get("destination_conf").(string) 
ownershipChallenge := d.Get("ownership_challenge").(string) 
var re = regexp.MustCompile(`^(datadog|splunk)\:\/\/`)

if ownershipChallenge == "" && !re.MatchString(destConf) {
    // we will error out as the destination conf requires a value
} 

@jacobbednarz jacobbednarz added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Categorizes issue or PR as related to improving an existing feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants