Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Check silence_root_warning option to skip warning on root install cmds #4113

Merged
merged 2 commits into from
Nov 21, 2015

Conversation

blackxored
Copy link
Contributor

Adds a new setting silence_root_warning (environment variable: BUNDLE_SILENCE_ROOT_WARNING) that skips the default warning when install is run as root.

The warning is useful but I think it should be able to be disabled if the user knows what he's doing, it's handy when run inside Docker containers (which by default run as root), or other environments where root is used.

Also adds a new preserve_env option to bundle helper in tests which allows running commands under sudo with the -E flag (preserve environment) as opposed to the default env_reset. Without this, the command would be run without our flag which makes it impossible to test. There might be cleaner ways to achieve this, though.

@segiddins
Copy link
Member

@@ -8,7 +8,7 @@ def initialize(options)
def run
Bundler.ui.level = "error" if options[:quiet]

warn_if_root
warn_if_root unless ENV['SILENCE_ROOT_WARNING']
Copy link
Member

Choose a reason for hiding this comment

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

this should be done via the bundler config (Bundler.settings[:silence_root_warning], which will need to be added as a bool key)

@blackxored
Copy link
Contributor Author

@segiddins I'll fix the style issues later this afternoon. As for the bundler config, I'm not really sure it belongs there as I don't want to avoid the warnings everywhere (i.e. make the setting project-wide) but only on specific environments. Let me hear your thoughts about it.

@segiddins
Copy link
Member

@blackxored it does belong in settings, since that's how bundler handles checking for settings.

@blackxored blackxored changed the title Check SILENCE_ROOT_WARNING env to skip warning on root install cmds Check silence_root_warning_option to skip warning on root install cmds Nov 18, 2015
@blackxored
Copy link
Contributor Author

I've fixed the style issues, updated to use Bundler.settings and the PR title and description. Added tests for setting it as an option as opposed to an env var, and also when is explicitly set to false. Let me know.

@blackxored blackxored changed the title Check silence_root_warning_option to skip warning on root install cmds Check silence_root_warning option to skip warning on root install cmds Nov 18, 2015
@segiddins
Copy link
Member

Looks good to me! r+, @indirect ?

@segiddins
Copy link
Member

@homu r+

@homu
Copy link
Contributor

homu commented Nov 21, 2015

📌 Commit b00e4da has been approved by segiddins

@homu
Copy link
Contributor

homu commented Nov 21, 2015

⌛ Testing commit b00e4da with merge 3720418...

homu added a commit that referenced this pull request Nov 21, 2015
Check silence_root_warning option to skip warning on root install cmds

Adds a new setting `silence_root_warning` (environment variable: `BUNDLE_SILENCE_ROOT_WARNING`) that skips the default warning when `install` is run as root.

The warning is useful but I think it should be able to be disabled if the user knows what he's doing, it's handy when run inside Docker containers (which by default run as root), or other environments where root is used.

Also adds a new `preserve_env` option to `bundle` helper in tests which allows running commands under sudo with the `-E` flag (preserve environment) as opposed to the default env_reset. Without this, the command would be run without our flag which makes it impossible to test. There might be cleaner ways to achieve this, though.
@homu
Copy link
Contributor

homu commented Nov 21, 2015

☀️ Test successful - status

@homu homu merged commit b00e4da into rubygems:master Nov 21, 2015
@envygeeks
Copy link

This is awesome, thanks so much!!!!!!

@blackxored
Copy link
Contributor Author

Thanks! I'm now trying to make it the default in the Docker Ruby images, not sure how that'd go.

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.

5 participants