-
Notifications
You must be signed in to change notification settings - Fork 187
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
[PDE-5660] feat(cli): Require --force
flag when updating production integration version environment variables via env:set
or env:unset
#942
base: main
Are you sure you want to change the base?
Conversation
|
||
if (this.flags.force) { | ||
requestOptions.extraHeaders = { | ||
'X-Force-Env-Var-Update': 'true', |
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.
hmmm I don't see this header referenced in the /multi-environment/
endpoint 🤔 or the Mixin classes it's a child of, are the headers directly passed to the lambda pools or am I looking at the wrong place?
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 believe this MR is the corresponding backend change.
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 think the developer experience change is good. But I have some suggestions on the code. Let me know what you think. Thanks!
char: 'f', | ||
description: | ||
'Force the update of environment variables regardless if the app version is production or not. Use with caution.', | ||
hidden: true, |
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 think it'd be more clear if --force
is visible in the help text. Any reason to hide it?
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.
You're right; I think in my head I was hoping to discourage folks from modifying live variables by "hiding" the option. But ultimately, they will see how to override via the error shown on 409
. So let's not complicate their lives and just show the option 😆
|
||
if (this.flags.force) { | ||
requestOptions.extraHeaders = { | ||
'X-Force-Env-Var-Update': 'true', |
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 believe this MR is the corresponding backend change.
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.
Looks good, thanks!
The idea here is to provide a bit of a warning for a potentially dangerous operation for live app versions. In fact, UI integrations can't update env vars for production versions at all, so it makes sense to add a buffer here for CLI integrations.
Since
is_production
is a backend check, let's react to a specific HTTP status and let the user know how to override via--force
. I'm not usingthis.confirm
(similar to user:add) since having users re-write the command with the added flag is even stricter.See related backend MR: https://gitlab.com/zapier/zapier/-/merge_requests/60723