modernize shell_out method syntax

[lamont-granquist]

**options has worked ever since we've deprecated ruby 1.9.x

[lamont-granquist]

@chef/client-core trivial cleanup of ruby 1.8.7-era arguments mangling...

[danielsdeleo]

👍

[coderanger]

This seems like a lot of oddly duplicated code still.

[coderanger]

      def shell_out(*args, **options)
        # For backwards compat, in case something below us modifies args?
        args = args.dup
        options = options.dup
        env_key = options.has_key?(:env) ? :env : :environment
        options[env_key] = {
            "LC_ALL" => Chef::Config[:internal_locale],
            "LANGUAGE" => Chef::Config[:internal_locale],
            "LANG" => Chef::Config[:internal_locale],
          }.update(options[env_key] || {})
        shell_out_command(*args, **options)
      end

That should be equivalent, right?

[lamont-granquist]

you still have to dup options[env_key] i think

[lamont-granquist]

oh wait... parsing...

[coderanger]

I think that should be correct, it's a new hash object .update'd with the old one, should mutate the new object.

[lamont-granquist]

yeah, that works the same in my head, lets see if it works the same in the tests..

[coderanger]

We should have a comment on the args.dup one way or another, otherwise it looks unintentional since nothing in there mutates args :)

[lamont-granquist]

i think it is unintentional now and can be deleted?

[coderanger]

Or that, is there anything below this in the call stack that expects to modify the args but doesn't dup itself? If there is it's a huge bug but never say never ...

[lamont-granquist]

oh there's the deprecated options garbage...

[coderanger]

Yeah, might need to fix a few more functions at the same time :-(

[lamont-granquist]

yeah i got my yak trimmer out...

[lamont-granquist]

actually i think this works, #run_command_compatible_options does a #dup on args itself, and mixlib-shellout doesn't mangle its own args...

[coderanger]

👍 the deprecation code is gross but this should work I think (travis failure looks unrelated)

[lamont-granquist]

yeah but the deprecation code all gets nuked from orbit, hopefully soonish, whenever we start with Chef-13. i just want to make sure that when we grep for Chef.log_deprecation we find this and nuke it.

[codecov-io]

Current coverage is 100%

Merging #4865 into master will not change coverage

@@           master   #4865   diff @@
=====================================
  Files           0       0          
  Lines           0       0          
  Methods         0       0          
  Messages        0       0          
  Branches        0       0          
=====================================
  Hits            0       0          
  Misses          0       0          
  Partials        0       0          

Powered by Codecov. Last updated by 1c106d6

[lamont-granquist]

uh, lolwut?