-
Notifications
You must be signed in to change notification settings - Fork 337
Conversation
This PR is blocked until we have support for tokens in the workers UI and workers.dev. |
src/commands/config/mod.rs
Outdated
} else { | ||
message::warn("We don't recommend using your Global API Key! Please consider using an API Token instead. https://support.cloudflare.com/hc/en-us/articles/200167836-Managing-API-Tokens-and-Keys"); | ||
println!("Enter email: "); | ||
let mut email_str: String = read!("{}\n"); | ||
email_str.truncate(email_str.trim_end().len()); | ||
if !email_str.is_empty() { | ||
user.email = Some(email_str); | ||
} | ||
|
||
println!("Enter global API key: "); | ||
let mut api_key_str: String = read!("{}\n"); | ||
api_key_str.truncate(api_key_str.trim_end().len()); | ||
if !api_key_str.is_empty() { | ||
user.api_key = Some(api_key_str); | ||
} |
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.
shouldn't this else be moved to the level of if !api_key {} else {
, not if the API token string is empty? If the API token string is empty, I'd expect an error to be thrown (might as well check for length as well), but I wouldn't expect it to just continue to entering email and 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.
ah, so our baseline was that an empty string would not be serialized and would trigger the default API authentication failure message. This is preserving the preexisting behavior, but we can revisit this and think about adding additional error messages. We can do this after the RC is cut.
…rangler into gabbi/add-api-token-support
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.
the design is good, and apart from changes that have already been discussed, my only problem with this PR is shunting interactivity into a module function; as a rule, I think that should be kept as close to main as possible.
Co-Authored-By: Ashley Michal <[email protected]>
Co-Authored-By: Ashley Michal <[email protected]>
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
This closes #354.
API tokens can now be added to
wrangler config
with the following workflow:Currently, a user is expected to enter an email/API key pair or API token.