(#6758) Pass options as an argument to string actions.

Earlier in their implementation the String prototype would set global state on
a String object to reflect options set on the command line.  As we move
strings away from a CLI-only prototype, this becomes troublesome because we
can easily have, for example, HTTP access to a string, which means load
balancers can really make this confusing.

It also encourages global state pollution, where one invocation can adversely
influence another.  A better approach is that we pass options to the string
action invocation directly; this makes the interaction stateless.

Changes required to your code to adapt to the new world:

-    action(:foo) do |some, args|
+    action(:foo) do |some, args, options={}|
       if options[:whatever] then

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>

4 files changed, 13 insertions, 15 deletions

diff --git a/lib/puppet/application/string_base.rb b/lib/puppet/application/string_base.rb
index bc627adde..762fbfda8 100644
--- a/lib/puppet/application/string_base.rb
+++ b/lib/puppet/application/string_base.rb
@@ -63,11 +63,12 @@ class Puppet::Application::StringBase < Puppet::Application
   def setup
     Puppet::Util::Log.newdestination :console
 
+    # We copy all of the app options to the end of the call; This allows each
+    # action to read in the options.  This replaces the older model where we
+    # would invoke the action with options set as global state in the
+    # interface object.  --daniel 2011-03-28
     @verb = command_line.args.shift
-    @arguments = command_line.args
-    @arguments ||= []
-
-    @arguments = Array(@arguments)
+    @arguments = Array(command_line.args) << options
 
     @type = self.class.name.to_s.sub(/.+:/, '').downcase.to_sym
 
@@ -78,10 +79,6 @@ class Puppet::Application::StringBase < Puppet::Application
     @string = Puppet::String[@type, :current]
     @format ||= @string.default_format
 
-    # We copy all of the app options to the string.
-    # This allows each action to read in the options.
-    @string.options = options
-
     validate
   end
 
diff --git a/lib/puppet/string.rb b/lib/puppet/string.rb
index 783b6afe0..04db1f33b 100644
--- a/lib/puppet/string.rb
+++ b/lib/puppet/string.rb
@@ -53,7 +53,7 @@ class Puppet::String
     self.default_format = format.to_sym
   end
 
-  attr_accessor :type, :verb, :version, :arguments, :options
+  attr_accessor :type, :verb, :version, :arguments
   attr_reader :name
 
   def initialize(name, version, &block)
diff --git a/spec/unit/application/string_base_spec.rb b/spec/unit/application/string_base_spec.rb
index 86f9c09aa..65cadb8fd 100755
--- a/spec/unit/application/string_base_spec.rb
+++ b/spec/unit/application/string_base_spec.rb
@@ -5,6 +5,7 @@ require 'puppet/application/string_base'
 require 'tmpdir'
 
 class Puppet::Application::StringBase::Basetest < Puppet::Application::StringBase
+  option("--[no-]foo")
 end
 
 describe Puppet::Application::StringBase do
@@ -61,14 +62,14 @@ describe Puppet::Application::StringBase do
     it "should make sure arguments are an array" do
       @app.command_line.stubs(:args).returns(["find", "myname", "myarg"])
       @app.setup
-      @app.arguments.should == ["myname", "myarg"]
+      @app.arguments.should == ["myname", "myarg", {}]
     end
 
-    it "should set the options on the string" do
-      @app.options[:foo] = "bar"
+    it "should pass options as the last argument" do
+      @app.command_line.stubs(:args).returns(["find", "myname", "myarg", "--foo"])
+      @app.parse_options
       @app.setup
-
-      @app.string.options.should == @app.options
+      @app.arguments.should == ["myname", "myarg", { :foo => true }]
     end
   end
 end
diff --git a/spec/unit/string/action_spec.rb b/spec/unit/string/action_spec.rb
index caf3291b6..f4ca8316d 100755
--- a/spec/unit/string/action_spec.rb
+++ b/spec/unit/string/action_spec.rb
@@ -7,7 +7,7 @@ describe Puppet::String::Action do
   describe "when validating the action name" do
     [nil, '', 'foo bar', '-foobar'].each do |input|
       it "should treat #{input.inspect} as an invalid name" do
-        expect { Puppet::Interface::Action.new(nil, input) }.
+        expect { Puppet::String::Action.new(nil, input) }.
           should raise_error(/is an invalid action name/)
       end
     end