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

Fix wrangler config for systems with non-unix EOL #399

Merged
merged 1 commit into from
Aug 5, 2019
Merged

Conversation

xortive
Copy link
Contributor

@xortive xortive commented Aug 5, 2019

wrangler config was not properly truncating whitespace from the end of user input, resulting in a panic when trying to use wrangler publish as wrangler would try to create an HTTP header with invalid characters. Now, wrangler will properly truncate extra whitespace (including \r) from the end of input to wrangler config

Fixes #389

email: email.to_string(),
api_key: api_key.to_string(),
};
pub fn global_config(email: String, api_key: String) -> Result<(), failure::Error> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while I was in here, I fixed the ownership semantics of global_config to remove a useless string copy.

@@ -153,11 +153,13 @@ fn run() -> Result<(), failure::Error> {

if let Some(_matches) = matches.subcommand_matches("config") {
println!("Enter email: ");
let email: String = read!("{}\n");
let mut email: String = read!("{}\n");
email.truncate(email.trim_end().len());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the truncate dance here is to avoid allocating an extra string, and to keep email as an owned String instead of a string slice

let fake_home_dir = env::current_dir()
.expect("could not retrieve cwd")
.join(".it_generates_the_config");
.join(format!(".it_generates_the_config_{}", random_chars(5)));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because cargo test is multithreaded, I had to add this random_char thing to the end of the home directory name to keep the threads from deleting each other's home dir. We should probably enforce this for all tests that create directories in the future.

@xortive
Copy link
Contributor Author

xortive commented Aug 5, 2019

Also: wanted review on the actual PR title and message content, trying out formatting these such that they work well when copy-pasted into a changelog.

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, great work

println!("Enter api key: ");
let api_key: String = read!("{}\n");
let mut api_key: String = read!("{}\n");
api_key.truncate(api_key.trim_end().len());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😻

@xortive xortive force-pushed the malonso/fix-389 branch 3 times, most recently from f25e36f to 6b1c2cc Compare August 5, 2019 21:07
@EverlastingBugstopper EverlastingBugstopper added this to the 1.1.1 milestone Aug 5, 2019
@xortive xortive merged commit 0030a3f into master Aug 5, 2019
@delete-merged-branch delete-merged-branch bot deleted the malonso/fix-389 branch August 5, 2019 23:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrangler can panic if global user configuration contains non printable characters
2 participants