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

Add Resources to Cloudflare Terraform Provider #2633

Merged

Conversation

apiazza-dd
Copy link
Contributor

This PR allows customers to provision resources for their Cloudflare Accounts using Terraform.

@apiazza-dd apiazza-dd requested review from a team as code owners October 28, 2024 21:30
@apiazza-dd apiazza-dd requested a review from a team as a code owner October 29, 2024 15:35
Copy link
Contributor

@maycmlee maycmlee left a comment

Choose a reason for hiding this comment

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

Small suggestions and a question.

@@ -33,6 +33,7 @@ resource "datadog_integration_cloudflare_account" "foo" {
### Optional

- `email` (String) The email associated with the Cloudflare account. If an API key is provided (and not a token), this field is also required.
- `resources` (List of String) An allowlist of resources to restrict pulling metrics for including 'web', 'dns', 'lb' (load balancer), 'worker')
Copy link
Contributor

@maycmlee maycmlee Oct 29, 2024

Choose a reason for hiding this comment

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

Are web, dns, etc. tags on the metrics? Or something else?

Suggested change
- `resources` (List of String) An allowlist of resources to restrict pulling metrics for including 'web', 'dns', 'lb' (load balancer), 'worker')
- `resources` (List of String) An allowlist of resources to restrict pulling metrics for including `web`, `dns`, `lb` (load balancer), `worker`)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are enabling which resources to collect when provisioning a cloudflare account via terraform.

@apiazza-dd apiazza-dd force-pushed the apiazza-dd/add-resources-to-cloudflare-terrraform-provider branch from 9db2b45 to 8b68c39 Compare October 29, 2024 16:44
…uring a Cloudflare account using the Terraform Provider and update tests accordingly.
@apiazza-dd apiazza-dd force-pushed the apiazza-dd/add-resources-to-cloudflare-terrraform-provider branch from 7f94ba1 to 3788560 Compare October 29, 2024 16:48
@apiazza-dd apiazza-dd requested a review from maycmlee October 29, 2024 17:15
Copy link
Contributor

@maycmlee maycmlee left a comment

Choose a reason for hiding this comment

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

How about this?

@@ -33,6 +33,7 @@ resource "datadog_integration_cloudflare_account" "foo" {
### Optional

- `email` (String) The email associated with the Cloudflare account. If an API key is provided (and not a token), this field is also required.
- `resources` (List of String) An allowlist of resources to restrict pulling metrics for including `web`, `dns`, `lb` (load balancer), `worker`)
Copy link
Contributor

@maycmlee maycmlee Oct 29, 2024

Choose a reason for hiding this comment

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

Suggested change
- `resources` (List of String) An allowlist of resources to restrict pulling metrics for including `web`, `dns`, `lb` (load balancer), `worker`)
- `resources` (List of String) An allowlist of resources, such as `web`, `dns`, `lb` (load balancer), `worker`, that restricts pulling metrics for those resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the restriction part is important here, by default, they're set to all, but if they want one turned off, then they use the resources list. Phrasing as an allow list makes it seem like if they specify one, they'll collect metrics for it, when it's the opposite.

Copy link
Contributor

Choose a reason for hiding this comment

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

@apiazza-dd Ah okay, thanks for the clarification. I updated the suggestion, what do you think?

Copy link
Contributor

@maycmlee maycmlee Oct 31, 2024

Choose a reason for hiding this comment

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

@apiazza-dd actually rereading your comment, I might have misunderstood.

but if they want one turned off, then they use the resources list.

Is the allowlist of resources, the resources that users want to pull metrics for or the resources they don't want pulling metrics for? If it's the latter, i think i'm throw off by it being called an allowlist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maycmlee These docs are autogenerated and have been updated to match our other repository. Does this look okay, or should I make a change in the other repo?

Copy link
Contributor

Choose a reason for hiding this comment

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

DM'd. Docs are autogenerated so the suggestion will be updated in the other repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alpinet
alpinet previously approved these changes Oct 30, 2024
…grationCloudflareAccountUpdateRequestBody to make sure this data is in the payload
@apiazza-dd apiazza-dd force-pushed the apiazza-dd/add-resources-to-cloudflare-terrraform-provider branch from 2b4ad10 to 6e4db55 Compare October 31, 2024 20:38
@apiazza-dd apiazza-dd requested a review from skarimo October 31, 2024 20:52
maycmlee
maycmlee previously approved these changes Nov 1, 2024
@@ -33,6 +33,7 @@ resource "datadog_integration_cloudflare_account" "foo" {
### Optional

- `email` (String) The email associated with the Cloudflare account. If an API key is provided (and not a token), this field is also required.
- `resources` (List of String) An allowlist of resources to restrict pulling metrics for including `web`, `dns`, `lb` (load balancer), `worker`)
Copy link
Contributor

Choose a reason for hiding this comment

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

DM'd. Docs are autogenerated so the suggestion will be updated in the other repo.

ZacharyAnderson
ZacharyAnderson previously approved these changes Nov 1, 2024
gunterd
gunterd previously approved these changes Nov 4, 2024
maycmlee
maycmlee previously approved these changes Nov 4, 2024
@skarimo skarimo dismissed stale reviews from maycmlee and gunterd via 921b0a5 November 4, 2024 18:28
gunterd
gunterd previously approved these changes Nov 4, 2024
@apiazza-dd apiazza-dd force-pushed the apiazza-dd/add-resources-to-cloudflare-terrraform-provider branch from d1d7c94 to 6327791 Compare November 4, 2024 19:20
@skarimo skarimo merged commit 9ffde5c into master Nov 5, 2024
11 checks passed
@skarimo skarimo deleted the apiazza-dd/add-resources-to-cloudflare-terrraform-provider branch November 5, 2024 14:07
Drarig29 pushed a commit that referenced this pull request Nov 6, 2024
* Allow customers to provision which resources they collect when configuring a Cloudflare account using the Terraform Provider and update tests accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants