6354 - MCOLLECTIVE_EXTRA_OPTS not being recognized with unified binary

Pre-parse the MCOLLECTIVE_EXTRA_OPTS stripping out everything except
config related options and parse that using the normal optparse environment
method

lib/mcollective.rb

@@ -6,6 +6,7 @@
 require 'singleton'
 require 'socket'
 require 'erb'
+require 'shellwords'
 require 'mcollective/monkey_patches'
 
 # == The Marionette Collective

lib/mcollective/applications.rb

@@ -41,14 +41,46 @@ def self.list
             return []
         end
 
-        # Loads the config and checks if --config or -c is given
+        # Filters a string of opts out using Shellwords
+        # keeping only things related to --config and -c
+        def self.filter_extra_options(opts)
+            res = ""
+            words = Shellwords.shellwords(opts)
+            words.each_with_index do |word,idx|
+                if word == "-c"
+                    return "--config=#{words[idx + 1]}"
+                elsif word == "--config"
+                    return "--config=#{words[idx + 1]}"
+                elsif word =~ /\-c=/
+                    return word
+                elsif word =~ /\-\-config=/
+                    return word
+                end
+            end
+        end
+
+        # We need to know the config file in order to know the libdir
+        # so that we can find applications.
+        #
+        # The problem is the CLI might be stuffed with options only the
+        # app in the libdir might understand so we have a chicken and
+        # egg situation.
+        #
+        # We're parsing and filtering MCOLLECTIVE_EXTRA_OPTS removing
+        # all but config related options and parsing the options looking
+        # just for the config file.
+        #
+        # We're handling failures gracefully and finally restoring the
+        # ARG and MCOLLECTIVE_EXTRA_OPTS to the state they were before
+        # we started parsing.
         #
         # This is mostly a hack, when we're redoing how config works
         # this stuff should be made less sucky
         def self.load_config
             return if Config.instance.configured
 
             original_argv = ARGV.clone
+            original_extra_opts = ENV["MCOLLECTIVE_EXTRA_OPTS"].clone
             configfile = nil
 
             parser = OptionParser.new
@@ -63,14 +95,26 @@ def self.load_config
             # avoid option parsers own internal version handling that sux
             parser.on("-v", "--verbose")
 
-            parser.environment("MCOLLECTIVE_EXTRA_OPTS")
+            begin
+                # optparse will parse the whole ENV in one go and refuse
+                # to play along with the retry trick I do below so in
+                # order to handle unknown options properly I parse out
+                # only -c and --config deleting everything else and
+                # then restore the environment variable later when I
+                # am done with it
+                ENV["MCOLLECTIVE_EXTRA_OPTS"] = filter_extra_options(ENV["MCOLLECTIVE_EXTRA_OPTS"].clone)
+                parser.environment("MCOLLECTIVE_EXTRA_OPTS")
+            rescue Exception => e
+                Log.error("Failed to parse MCOLLECTIVE_EXTRA_OPTS: #{e}")
+            end
 
             begin
                 parser.parse!
-            rescue OptionParser::InvalidOption
+            rescue OptionParser::InvalidOption => e
                 retry
             end
 
+            ENV["MCOLLECTIVE_EXTRA_OPTS"] = original_extra_opts.clone
             ARGV.clear
             original_argv.each {|a| ARGV << a}
 

spec/unit/applications_spec.rb

@@ -0,0 +1,27 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../spec_helper'
+
+module MCollective
+    describe Applications do
+        describe "#filter_extra_options" do
+            it "should parse --config=x" do
+                ["--config=x --foo=bar -f -f bar", "--foo=bar --config=x -f -f bar"].each do |t|
+                    Applications.filter_extra_options(t).should == "--config=x"
+                end
+            end
+
+            it "should parse --config x" do
+                ["--config x --foo=bar -f -f bar", "--foo=bar --config x -f -f bar"].each do |t|
+                    Applications.filter_extra_options(t).should == "--config=x"
+                end
+            end
+
+            it "should parse -c x" do
+                ["-c x --foo=bar -f -f bar", "--foo=bar -c x -f -f bar"].each do |t|
+                    Applications.filter_extra_options(t).should == "--config=x"
+                end
+            end
+        end
+    end
+end

website/changelog.md

@@ -11,6 +11,7 @@ title: Changelog
 
 |Date|Description|Ticket|
 |----|-----------|------|
+|2011/02/17|Correctly parse MCOLLECTIVE_EXTRA_OPTS in the new unified binary framework|6354|
 |2011/02/15|Allow the signing key and Debian distribution to be customized|6321|
 |2011/02/14|Remove inadvertently included package.ddl|6313|
 |2011/02/14|Handle missing libdirs without crashing|6306|