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

Add Secret commands #1045

Merged
merged 2 commits into from
Feb 12, 2020
Merged

Add Secret commands #1045

merged 2 commits into from
Feb 12, 2020

Conversation

ashleymichal
Copy link
Contributor

closes #907, #912, #909 . just a repackaging of #994

@ashleymichal ashleymichal mentioned this pull request Feb 11, 2020
3 tasks
@ashleymichal ashleymichal changed the title Alewis/secrets Add Secret commands Feb 11, 2020
@ashleymichal ashleymichal requested a review from a team February 11, 2020 18:00
@ashleymichal ashleymichal self-assigned this Feb 11, 2020
@ashleymichal ashleymichal added changelog - feature regression Something is broken, but works in previous releases labels Feb 11, 2020
@ashleymichal ashleymichal added this to the 1.8.0-rc.0 milestone Feb 11, 2020
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.

Overall looks good! Most of my comments are about error messages and just a few questions I have 😄

src/commands/mod.rs Outdated Show resolved Hide resolved
src/commands/secret/mod.rs Outdated Show resolved Hide resolved
"Your wrangler.toml is likely missing the field \"account_id\", which is required to write to Workers KV."
}
10001 => "The content you passed in is not an accepted string. Try entering just a string",
10053 => "There is already another binding with a different type by this name. Check your wrangler.toml or dashboard for conflicting bindings",
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we decide we're listing secrets in wrangler.toml? Don't want to re-litigate that discussion just want to make sure if we're telling people to check their toml that there's something they can do there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I'd say "wrangler.toml or your Cloudflare dashboard"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so this happens in the case that there is a binding by the same name but of another kind, so perhaps a kv namespace or plain text binding, for example. this isn't in the case of a secret by the same name, because this is a set so it'll just overwrite.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah. how about "There is already a KV namespace or text binding with this name associated with your script"or something like that/ "binding with a different type by this name" is hard to parse

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is a world in which it is actually a text blob binding you are using for sites, or you went and called it wasm or something else that's taken by our build process. if we wanted to be really robust, we could query the list of bindings to show what's up, check their toml, look to see if they're stomping on their own asset manifest or wasm binding... this feels like a good add-on issue though.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

src/commands/secret/mod.rs Outdated Show resolved Hide resolved
src/commands/secret/mod.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/terminal/interactive.rs Outdated Show resolved Hide resolved
src/terminal/interactive.rs Show resolved Hide resolved
src/terminal/interactive.rs Outdated Show resolved Hide resolved
println!("{} [y/n]", prompt_string);
let mut response: String = read!("{}\n");
response = response.split_whitespace().collect(); // remove whitespace
response.make_ascii_lowercase(); // ensure response is all lowercase
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason we use make_ascii_lowercase instead of to_lowercase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This was copy from gabbi's work on interactive delete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interesting. it is oddly specific...perhaps we should leave it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm going to revert it to using this, and cc @gabbifish for when she gets back to see if we can change it. if not, i'd like to add a comment so we don't re-litigate this in the future.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
regression Something is broken, but works in previous releases
Projects
None yet
3 participants