Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Refactor cloudflare-rs cli to be called from http.rs #841

Merged
merged 8 commits into from
Nov 8, 2019

Conversation

gabbifish
Copy link
Contributor

@gabbifish gabbifish commented Nov 4, 2019

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* and wrangler 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.

@gabbifish gabbifish added refactor status - needs review regression Something is broken, but works in previous releases labels Nov 4, 2019
@gabbifish gabbifish added this to the 1.6.0 milestone Nov 4, 2019
@gabbifish gabbifish changed the title Refactor cloudflare-rs cli to call from http.rs Refactor cloudflare-rs cli to be called from http.rs Nov 4, 2019
Copy link
Contributor

@ashleymichal ashleymichal left a 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.

src/http.rs Outdated Show resolved Hide resolved
src/http.rs Outdated Show resolved Hide resolved
src/http.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a 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

Copy link
Contributor

@ashleymichal ashleymichal left a 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(
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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.

src/http.rs Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
refactor regression Something is broken, but works in previous releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants