-
Notifications
You must be signed in to change notification settings - Fork 630
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 worker kv #595
Add worker kv #595
Conversation
Looks like integration tests are failing for this one:
|
client := meta.(*cloudflare.API) | ||
namespaceID, key := parseId(d) | ||
|
||
value, err := client.ReadWorkersKV(context.Background(), namespaceID, key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the Worker KV methods require setting api.AccountID
however we're not setting it or documenting it as a required configuration parameter. How does it work at the moment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the methods have a parameter for the account ID, it is set on the cloudflare.API client when the provider configures it. This is identical to how the existing cloudflare_workers_kv_namespace resource works, but I can add it to the documentation if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a note in the docs that setting account_id
on the provider, or the CLOUDFLARE_ACCOUNT_ID
environment variable is necessary to use this. Does that cover the requirement?
return &schema.Resource{ | ||
Create: resourceCloudflareWorkersKVCreate, | ||
Read: resourceCloudflareWorkersKVRead, | ||
Update: resourceCloudflareWorkersKVUpdate, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your create + update methods are pretty similar. In some resources where this is also the case, the Create
and Update
just share the one method. Up to you as I don't mind either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any harm in calling d.SetId
in an Update
method? That's the main difference between them. I'm all for saving some code if that isn't an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These have been combined into one method.
Co-Authored-By: Jacob Bednarz <jacob.bednarz@gmail.com>
…ces. Other small PR feedback fixes
@jacobbednarz I think I've addressed all of the feedback so far. Anything else we need before this can be approved? |
Thanks @nrf110 for the PR! |
dns: `Proxied` to become a boolean pointer
Support Workers KV Pairs. This should resolve #565
Proposed resource syntax:
resource "cloudflare_workers_kv" "example_kv" { namespace_id = cloudflare_workers_kv_namespace.example_ns.id key = "test-key" value = "some value" }