-
Notifications
You must be signed in to change notification settings - Fork 9
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
Change usage of STRAP_INTERACTIVE to STRAP_SUDO_PROMPT #6
base: master
Are you sure you want to change the base?
Conversation
lib/ansible.sh
Outdated
@@ -16,7 +16,7 @@ strap::lib::import exec || . exec.sh | |||
|
|||
STRAP_HOME="${STRAP_HOME:-}"; [[ -n "$STRAP_HOME" ]] || { echo "STRAP_HOME is not set" >&2; exit 1; } | |||
STRAP_USER_HOME="${STRAP_USER_HOME:-}"; [[ -n "$STRAP_USER_HOME" ]] || { echo "STRAP_USER_HOME is not set" >&2; exit 1; } | |||
STRAP_INTERACTIVE="${STRAP_INTERACTIVE:-}"; [[ -n "$STRAP_INTERACTIVE" ]] || STRAP_INTERACTIVE=true | |||
STRAP_SUDO_PROMPT="${STRAP_SUDO_PROMPT:-}"; [[ -n "$STRAP_SUDO_PROMPT" ]] || STRAP_SUDO_PROMPT=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.
So one backwards compat option could be to change this to:
STRAP_SUDO_PROMPT="${STRAP_SUDO_PROMPT:-$STRAP_INTERACTIVE}"; [[ -n "$STRAP_SUDO_PROMPT" ]] || STRAP_SUDO_PROMPT=true
Happy to make that change if folks think it would be useful.
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.
That change looks better from a backwards-compatibility stance IMO
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.
👍
@kelvinzhu-okta and @seanwang-okta can you guys take a look at this and verify? |
@mpatnode can you please help me understand the change? I could be missing something, but it looks like just a variable rename - why does renaming a variable change anything? |
STRAP_SUDO_PROMPT is already being used in sudo.sh. STRAP_INTERACTIVE appears to be a bigger hammer which says don't expect a TTY, while STRAP_SUDO_PROMPT says stop asking for the sudo password. The latter is all I want for my usage case, I don't want to disable all interactive prompts. |
So I was a little frustrated/surprised that setting STRAP_SUDO_PROMPT to false didn't stop strap from asking for my password. Seemed to make sense to me, but I can see how it might break existing usage.