-
Notifications
You must be signed in to change notification settings - Fork 753
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: wrangler secret * --local
#307
Conversation
🦋 Changeset detectedLatest commit: a7385e8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
55a087e
to
a76b2bc
Compare
|
||
feat: `wrangler secret * --local` | ||
|
||
This PR implements `wrangler secret` for `--local` mode. The implementation is simply a no-op, since we don't want to actually write secret values to disk (I think?). I also got the messaging for remote mode right by copying from wrangler 1. Further, I added tests for all the `wrangler secret` commands. |
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.
I'm not sure I follow the reasoning behind not storing to disk. The current workflow for secret
is run it once, then never need to again. If that workflow differs for --local
it might cause more complication. (If you have a Worker with a secret, how do you test it locally?)
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.
I'm actually not sure what to do here! Writing to disk introduces the chance of people committing it to git or a cache, which may be worse. I'm not sure what the workflow here should be, and it probably requires a resolution on #190 to move forward. minflare doesn't even implement a workflow for secrets. This PR would at least have something, is why I opened it up. Got any ideas?
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.
ping
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.
Hmm, interesting. I guess let's merge for now and figure that out later.
@@ -1043,6 +1042,13 @@ export async function main(argv: string[]): Promise<void> { | |||
"Enter a secret value:", | |||
"password" | |||
); | |||
|
|||
if (args.local) { | |||
return; |
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.
How about some simple notification with console.log...
that local mode don't go on writing secrets? 🤔
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.
It does log a warning, on line 1020
This PR implements `wrangler secret` for `--local` mode. The implementation is simply a no-op, since we don't want to actually write secret values to disk (I think?). I also got the messaging for remote mode right by copying from wrangler 1. Further, I added tests for all the `wrangler secret` commands.
a76b2bc
to
a7385e8
Compare
So...I'm not sure I understand how this task got checked off in the umbrella task. Am I understanding correctly that using Mini-flare with wrangler 2 locally simply can't use secrets? |
(On phone, excuse typos) We need to document this, but you can make a |
Maybe we can now implement secret --local that reads/writes to this file 🤔 |
Thanks so much! That's super extra helpful! |
Does that work for any environment? |
Hi @Crisfol. The |
This PR implements
wrangler secret
for--local
mode. The implementation is simply a no-op, since we don't want to actually write secret values to disk (I think?). I also got the messaging for remote mode right by copying from wrangler 1. Further, I added tests for all thewrangler secret
commands.Fixes #302