-
Notifications
You must be signed in to change notification settings - Fork 84
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 support for Workers KV bulk upload API endpoint #28
Add support for Workers KV bulk upload API endpoint #28
Conversation
93d7c6f
to
723770b
Compare
88abdd4
to
3d0164a
Compare
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub expiration_ttl: Option<i32>, | ||
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub base64: Option<bool>, |
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.
We should be leveraging mechanisms discussed in dtolnay/request-for-implementation#18 for skip_serializing_if
and Option
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.
This is borat voice VERY NaIIICE (I added it in https://github.com/cloudflare/cloudflare-rs/pull/31/files)
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub expiration: Option<i32>, | ||
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub expiration_ttl: Option<i32>, |
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.
In other places (CreateDnsRecordParams
) TTL field has been defined as u32
, which seems more meaningful considering a negative TTL doesn't make sense.
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.
you may want to join the discussion here
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 chatted with the KV team and apparently they use i64! I will use that instead (its max value is much larger than u32).
pub key: String, | ||
pub value: String, | ||
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub expiration: Option<i32>, |
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.
What does this hold?
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.
It holds an expiration date (seconds since epoch)
This PR adds support for the Workers KV bulk upload API endpoint: https://api.cloudflare.com/#workers-kv-namespace-write-multiple-key-value-pairs.
It is used by cloudflare/wrangler-legacy#445