-
Notifications
You must be signed in to change notification settings - Fork 337
Refactor cloudflare-rs cli to be called from http.rs #841
Conversation
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 looks good to me generally, but i don't like the idea of dragging domain-specific logic into a shared module (i.e. the kv_help fn). Suggested compromise in the comments.
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.
lgtm once @ashleymichal's concerns are addressed
…are/wrangler into gabbi/refactor-cloudflare-rs-cli
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.
one obnoxious nit but lgtm
@@ -61,3 +72,59 @@ fn add_auth_headers<'a>(headers: &'a mut HeaderMap, user: &GlobalUser) { | |||
} | |||
} | |||
} | |||
|
|||
////---------------------------NEW API CLIENT CODE---------------------------//// | |||
pub fn api_client( |
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.
boy do i not want to bikeshed this, but i might suggest lending some specificity for future proofing, like cf_v4_api_client
or something. non-blocking
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.
ooh, i do like this idea... i defs want us to think about this more as a team. Mind if we make a separate ticket for this?
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.
yes, though being a soon-to-be ubiquitous public function it should be done sooner than later.
This is a minor refactor to where we call cloudflare-rs logic and methods for handling errors from this library. We move this logic from commands/kv/mod.rs to http.rs so that we can access the cloudflare-rs library beyond the KV module. This is crucial for #439, given that we want to use shared methods between
wrangler kv*
andwrangler config
to validate that a user's credentials work. Namely, we want to be able to use the same error handling logic of which KV was once the sole user.