sprinkle-tool / sprinkle

Sprinkle is a software provisioning tool you can use to build remote servers with. eg. to install a Rails, or Sinatra stack on a brand new slice directly after its been created
https://github.com/sprinkle-tool/sprinkle
MIT License
1.15k stars 138 forks source link

Default Capistrano to not use sudo #102

Closed micnigh closed 11 years ago

micnigh commented 11 years ago

See

61

If the user wants to default commands to using sudo, they can add below to their deploy.rb

set :run_method, :sudo
joshgoebel commented 11 years ago

Capistrano's default is to use sudo. (see commit comment) People using capistrano for both Sprinkle and a deployment recipe are likely already dependent upon this behavior, and understand that everything happens as sudo unless they set use_sudo to false. Why would confound those expectations by changing the default?

joshgoebel commented 11 years ago

I'm thinking perhaps we should not be setting it at all and using the setting coming into us from Capistrano. I assume that would also fix your problem? Or are you purposely wanting Capistrano to behave two different ways depending on how it's engaged?

micnigh commented 11 years ago

Capistrano sets :run_method based on what :use_sudo had been set to. The default for capistrano is determined by the :use_sudo setting.

Using whatever Capistranos was set to should work just as well.

micnigh commented 11 years ago

To clarify,

Capistrano normally sets the :run_method option based on :use_sudo parameter.

When debugging the capistrano actor, and having :use_sudo set to false, the fetch(:run_method) hasn't been set. However, I can call fetch(:use_sudo) and see that I've set that to false previously.

So something like below below should work, and follows capistranos conventions

via = fetch(:run_method, fetch(:use_sudo, true) ? :sudo : :run)
joshgoebel commented 11 years ago

And the use_sudo setting is itself defaulted to true in the one place in the code I see that it's used - that's what the second option to fetch does. :) So I think that line in our source needs to be just:

via = fetch(:run_method)

That way we are using whatever setting the use has already asked for in their Cap setup - and continuing to use sudo unless the used has turned it off explicitly. Though at that point I don't know if via is required to be passed to invoke or not... can you make that change and insure all tests are passing?

joshgoebel commented 11 years ago

Hmmmm.

joshgoebel commented 11 years ago

I think it's unset for you because you don't have a full Cap setup you're testing against? Most people have a Capfile that loads "deploy" to get all the defaults... I assume you do not?

joshgoebel commented 11 years ago
        # Invokes the given command. If a +via+ key is given, it will be used
        # to determine what method to use to invoke the command. It defaults
        # to :run, but may be :sudo, or any other method that conforms to the
        # same interface as run and sudo.
        def invoke_command(cmd, options={}, &block)
          options = options.dup
          via = options.delete(:via) || :run

So via is needed if you want sudo behavior. I think this is what we want:

via = fetch(:run_method) || :run

If it's unset it falls back to run... If someone is not using the capistrano default "deploy" then I don't think we should try an force those defaults upon them ourselves.

micnigh commented 11 years ago

I load my defaults through a deploy.rb file, pretty standard, buts its kind of old now, maybe its time to rebuild :)

I'd like to figure out why the fetch(:run_method) isn't being set; It's internal to Capistrano, so I'd rather it's default value be set there, rather than in Sprinkle.

The code you sent below should be the same as the original pull request, fetch's second parameter is a fallback if not set :) Previously it would be set to sudo by default.

joshgoebel commented 11 years ago

ROFL, we've come full circle. Looks like you need to patch up some tests though. This may need a changelog entry, but if you fix up the tests I think this is good to merge.

joshgoebel commented 11 years ago

The Capistrano defaults are usually pulled in from Capfile via load "deploy"... I bet the problem is that you have one but Sprinkle just loads the deploy.rb, not the Capfile... so it's possible we need to change that behavior as well as part of this.

joshgoebel commented 11 years ago

I just confirmed the default is for Cap to use sudo (if you're using a default Capistrano setup)... so we aren't really changing anything here for users using the defaults. I'm going to wrap everything up (your first commit, my tests, Capfile change) in one commit and merge it to 0-6-stable. Since this potentially changes behavior in an unexpected way it'll have to wait for 0.6 and get a mention in the changelog.

joshgoebel commented 11 years ago

Committed: https://github.com/sprinkle-tool/sprinkle/commit/38268d812aaca9fa0017a55e5ee7435faef97d2d

Anyone with a default Cap setup wanting to not use sudo needs either of these settings:

set :use_sudo, false
set :run_method, :run