From a3f5f974251e14f02e8f83e12f4589581dd21828 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Tue, 29 Mar 2011 11:56:43 -0700
Subject: MAINT: fix error reporting when you set terminus incorrectly.

Previously we tried to invoke a class method on an instance, and threw another
exception, which wasn't the most helpful behaviour.  This fixes that to
correctly report the array of available terminus classes to the user.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/string/indirector.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'lib/puppet/string')

diff --git a/lib/puppet/string/indirector.rb b/lib/puppet/string/indirector.rb
index 15984e39e..48ec32680 100644
--- a/lib/puppet/string/indirector.rb
+++ b/lib/puppet/string/indirector.rb
@@ -62,7 +62,7 @@ class Puppet::String::Indirector < Puppet::String
     begin
       indirection.terminus_class = from
     rescue => detail
-      raise "Could not set '#{indirection.name}' terminus to '#{from}' (#{detail}); valid terminus types are #{terminus_classes(indirection.name).join(", ") }"
+      raise "Could not set '#{indirection.name}' terminus to '#{from}' (#{detail}); valid terminus types are #{self.class.terminus_classes(indirection.name).join(", ") }"
     end
   end
 
-- 
cgit 


From 1400fec62e4e692badc5b68eb7d6d947997d7204 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Fri, 1 Apr 2011 13:10:42 -0700
Subject: MAINT: nicer to_s for actions, for user-focused rendering.

This makes it possible for us to just print the Action object directly and get
a human-focused output string.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/string/action.rb | 4 ++++
 1 file changed, 4 insertions(+)

(limited to 'lib/puppet/string')

diff --git a/lib/puppet/string/action.rb b/lib/puppet/string/action.rb
index 4db9e97e2..5a7f3f203 100644
--- a/lib/puppet/string/action.rb
+++ b/lib/puppet/string/action.rb
@@ -3,6 +3,10 @@ require 'puppet/string'
 class Puppet::String::Action
   attr_reader :name
 
+  def to_s
+    "#{@string}##{@name}"
+  end
+
   def initialize(string, name, attrs = {})
     name = name.to_s
     raise "'#{name}' is an invalid action name" unless name =~ /^[a-z]\w*$/
-- 
cgit 


From 5bba1a26140cd3326739b00c2d60dff9321d4044 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Thu, 24 Mar 2011 13:10:27 -0700
Subject: (#6749) Implement support for options on strings and actions.

We want to support both strings and actions specifying options, to support
generic wrappers that present strings to the user across multiple distinct
front-ends.

At the moment we focus on implementation of a generic CLI providing full
control to all the strings, but we aim to support other programmatic
interfaces including Ruby and RPC invocation as part of the overall change.

We also detect, at the time they are declared, duplicate options.  They are
reported, like any duplicate, with an error thrown.  Specifically:

It is illegal to declare a duplicate option in the same scope, such as within
the same string, or within the same action.  This is unchanged.

It is illegal to declare an option in an action that duplicates an option in
the string, or vice-versa.  This is reported when the duplicate is declared,
so may report on either the string or action depending on sequence.

It remains legal to duplicate the same option across multiple actions, with
different meanings.  There is no conflict, as the same option can't be passed
to both simultaneously.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/string/action.rb         | 32 ++++++++++++++++++++++----
 lib/puppet/string/action_builder.rb | 11 +++++----
 lib/puppet/string/action_manager.rb | 10 +++-----
 lib/puppet/string/option.rb         | 24 +++++++++++++++++++
 lib/puppet/string/option_builder.rb | 25 ++++++++++++++++++++
 lib/puppet/string/option_manager.rb | 46 +++++++++++++++++++++++++++++++++++++
 6 files changed, 132 insertions(+), 16 deletions(-)
 create mode 100644 lib/puppet/string/option.rb
 create mode 100644 lib/puppet/string/option_builder.rb
 create mode 100644 lib/puppet/string/option_manager.rb

(limited to 'lib/puppet/string')

diff --git a/lib/puppet/string/action.rb b/lib/puppet/string/action.rb
index 5a7f3f203..4219aca0a 100644
--- a/lib/puppet/string/action.rb
+++ b/lib/puppet/string/action.rb
@@ -1,4 +1,5 @@
 require 'puppet/string'
+require 'puppet/string/option'
 
 class Puppet::String::Action
   attr_reader :name
@@ -8,11 +9,10 @@ class Puppet::String::Action
   end
 
   def initialize(string, name, attrs = {})
-    name = name.to_s
-    raise "'#{name}' is an invalid action name" unless name =~ /^[a-z]\w*$/
-
-    @string = string
-    @name      = name
+    raise "#{name.inspect} is an invalid action name" unless name.to_s =~ /^[a-z]\w*$/
+    @string  = string
+    @name    = name.to_sym
+    @options = {}
     attrs.each do |k,v| send("#{k}=", v) end
   end
 
@@ -27,4 +27,26 @@ class Puppet::String::Action
       @string.meta_def(@name, &block)
     end
   end
+
+  def add_option(option)
+    if option? option.name then
+      raise ArgumentError, "#{option.name} duplicates an existing option on #{self}"
+    elsif @string.option? option.name then
+      raise ArgumentError, "#{option.name} duplicates an existing option on #{@string}"
+    end
+
+    @options[option.name] = option
+  end
+
+  def option?(name)
+    @options.include? name.to_sym
+  end
+
+  def options
+    (@options.keys + @string.options).sort
+  end
+
+  def get_option(name)
+    @options[name.to_sym] || @string.get_option(name)
+  end
 end
diff --git a/lib/puppet/string/action_builder.rb b/lib/puppet/string/action_builder.rb
index b3db51104..fb2a749ae 100644
--- a/lib/puppet/string/action_builder.rb
+++ b/lib/puppet/string/action_builder.rb
@@ -5,10 +5,8 @@ class Puppet::String::ActionBuilder
   attr_reader :action
 
   def self.build(string, name, &block)
-    name = name.to_s
-    raise "Action '#{name}' must specify a block" unless block
-    builder = new(string, name, &block)
-    builder.action
+    raise "Action #{name.inspect} must specify a block" unless block
+    new(string, name, &block).action
   end
 
   def initialize(string, name, &block)
@@ -24,4 +22,9 @@ class Puppet::String::ActionBuilder
     raise "Invoke called on an ActionBuilder with no corresponding Action" unless @action
     @action.invoke = block
   end
+
+  def option(name, attrs = {}, &block)
+    option = Puppet::String::OptionBuilder.build(@action, name, attrs, &block)
+    @action.add_option(option)
+  end
 end
diff --git a/lib/puppet/string/action_manager.rb b/lib/puppet/string/action_manager.rb
index c29dbf454..c980142ce 100644
--- a/lib/puppet/string/action_manager.rb
+++ b/lib/puppet/string/action_manager.rb
@@ -5,20 +5,15 @@ module Puppet::String::ActionManager
   # the code to do so.
   def action(name, &block)
     @actions ||= {}
-    name = name.to_s.downcase.to_sym
-
     raise "Action #{name} already defined for #{self}" if action?(name)
-
     action = Puppet::String::ActionBuilder.build(self, name, &block)
-
-    @actions[name] = action
+    @actions[action.name] = action
   end
 
   # This is the short-form of an action definition; it doesn't use the
   # builder, just creates the action directly from the block.
   def script(name, &block)
     @actions ||= {}
-    name = name.to_s.downcase.to_sym
     raise "Action #{name} already defined for #{self}" if action?(name)
     @actions[name] = Puppet::String::Action.new(self, name, :invoke => block)
   end
@@ -36,7 +31,8 @@ module Puppet::String::ActionManager
   end
 
   def get_action(name)
-    @actions[name].dup
+    @actions ||= {}
+    @actions[name.to_sym]
   end
 
   def action?(name)
diff --git a/lib/puppet/string/option.rb b/lib/puppet/string/option.rb
new file mode 100644
index 000000000..bdc3e07c5
--- /dev/null
+++ b/lib/puppet/string/option.rb
@@ -0,0 +1,24 @@
+class Puppet::String::Option
+  attr_reader :name, :string
+
+  def initialize(string, name, attrs = {})
+    raise "#{name.inspect} is an invalid option name" unless name.to_s =~ /^[a-z]\w*$/
+    @string = string
+    @name   = name.to_sym
+    attrs.each do |k,v| send("#{k}=", v) end
+  end
+
+  def to_s
+    @name.to_s.tr('_', '-')
+  end
+
+  Types = [:boolean, :string]
+  def type
+    @type ||= :boolean
+  end
+  def type=(input)
+    value = begin input.to_sym rescue nil end
+    Types.include?(value) or raise ArgumentError, "#{input.inspect} is not a valid type"
+    @type = value
+  end
+end
diff --git a/lib/puppet/string/option_builder.rb b/lib/puppet/string/option_builder.rb
new file mode 100644
index 000000000..2087cbc99
--- /dev/null
+++ b/lib/puppet/string/option_builder.rb
@@ -0,0 +1,25 @@
+require 'puppet/string/option'
+
+class Puppet::String::OptionBuilder
+  attr_reader :option
+
+  def self.build(string, name, attrs = {}, &block)
+    new(string, name, attrs, &block).option
+  end
+
+  private
+  def initialize(string, name, attrs, &block)
+    @string = string
+    @option = Puppet::String::Option.new(string, name, attrs)
+    block and instance_eval(&block)
+    @option
+  end
+
+  # Metaprogram the simple DSL from the option class.
+  Puppet::String::Option.instance_methods.grep(/=$/).each do |setter|
+    next if setter =~ /^=/      # special case, darn it...
+
+    dsl = setter.sub(/=$/, '')
+    define_method(dsl) do |value| @option.send(setter, value) end
+  end
+end
diff --git a/lib/puppet/string/option_manager.rb b/lib/puppet/string/option_manager.rb
new file mode 100644
index 000000000..df3ae6b4b
--- /dev/null
+++ b/lib/puppet/string/option_manager.rb
@@ -0,0 +1,46 @@
+require 'puppet/string/option_builder'
+
+module Puppet::String::OptionManager
+  # Declare that this app can take a specific option, and provide
+  # the code to do so.
+  def option(name, attrs = {}, &block)
+    @options ||= {}
+    raise ArgumentError, "Option #{name} already defined for #{self}" if option?(name)
+    actions.each do |action|
+      if get_action(action).option?(name) then
+        raise ArgumentError, "Option #{name} already defined on action #{action} for #{self}"
+      end
+    end
+    option = Puppet::String::OptionBuilder.build(self, name, &block)
+    @options[option.name] = option
+  end
+
+  def options
+    @options ||= {}
+    result = @options.keys
+
+    if self.is_a?(Class) and superclass.respond_to?(:options)
+      result += superclass.options
+    elsif self.class.respond_to?(:options)
+      result += self.class.options
+    end
+    result.sort
+  end
+
+  def get_option(name)
+    @options ||= {}
+    result = @options[name.to_sym]
+    unless result then
+      if self.is_a?(Class) and superclass.respond_to?(:get_option)
+        result = superclass.get_option(name)
+      elsif self.class.respond_to?(:get_option)
+        result = self.class.get_option(name)
+      end
+    end
+    return result
+  end
+
+  def option?(name)
+    options.include? name.to_sym
+  end
+end
-- 
cgit 


From a113e8f03d257375bf4eb2416a6ad7e1958d7868 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Tue, 29 Mar 2011 15:34:23 -0700
Subject: (#6749) implementing option handling in CLI string wrapper

The purpose of this is to adapt the generic option support in our strings to
the command line; we adapt the generic option information to optparse, and
establish our environment early in the process to ensure that we can play nice
with Puppet::Application for the moment.

In the process we ensure that we detect, and report, conflicts in option
naming across the board.  Additionally, when an option is declared with
multiple aliases, we insist that either all, or none, of them take an
argument.

To support this we support introspecting options having an optional argument,
as well as documentation and all.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/string/action.rb         | 16 +++++---
 lib/puppet/string/action_builder.rb |  4 +-
 lib/puppet/string/indirector.rb     |  6 +++
 lib/puppet/string/option.rb         | 73 ++++++++++++++++++++++++++++++-------
 lib/puppet/string/option_builder.rb |  8 ++--
 lib/puppet/string/option_manager.rb | 26 +++++++++----
 6 files changed, 100 insertions(+), 33 deletions(-)

(limited to 'lib/puppet/string')

diff --git a/lib/puppet/string/action.rb b/lib/puppet/string/action.rb
index 4219aca0a..692e467b4 100644
--- a/lib/puppet/string/action.rb
+++ b/lib/puppet/string/action.rb
@@ -29,13 +29,19 @@ class Puppet::String::Action
   end
 
   def add_option(option)
-    if option? option.name then
-      raise ArgumentError, "#{option.name} duplicates an existing option on #{self}"
-    elsif @string.option? option.name then
-      raise ArgumentError, "#{option.name} duplicates an existing option on #{@string}"
+    option.aliases.each do |name|
+      if conflict = get_option(name) then
+        raise ArgumentError, "Option #{option} conflicts with existing option #{conflict}"
+      elsif conflict = @string.get_option(name) then
+        raise ArgumentError, "Option #{option} conflicts with existing option #{conflict} on #{@string}"
+      end
     end
 
-    @options[option.name] = option
+    option.aliases.each do |name|
+      @options[name] = option
+    end
+
+    option
   end
 
   def option?(name)
diff --git a/lib/puppet/string/action_builder.rb b/lib/puppet/string/action_builder.rb
index fb2a749ae..e76044470 100644
--- a/lib/puppet/string/action_builder.rb
+++ b/lib/puppet/string/action_builder.rb
@@ -23,8 +23,8 @@ class Puppet::String::ActionBuilder
     @action.invoke = block
   end
 
-  def option(name, attrs = {}, &block)
-    option = Puppet::String::OptionBuilder.build(@action, name, attrs, &block)
+  def option(*declaration, &block)
+    option = Puppet::String::OptionBuilder.build(@action, *declaration, &block)
     @action.add_option(option)
   end
 end
diff --git a/lib/puppet/string/indirector.rb b/lib/puppet/string/indirector.rb
index 48ec32680..71b1f3b12 100644
--- a/lib/puppet/string/indirector.rb
+++ b/lib/puppet/string/indirector.rb
@@ -2,6 +2,12 @@ require 'puppet'
 require 'puppet/string'
 
 class Puppet::String::Indirector < Puppet::String
+  warn "REVISIT: Need to redefine this to take arguments again, eh."
+  option "--terminus TERMINUS" do
+    desc "REVISIT: You can select a terminus, which has some bigger effect
+that we should describe in this file somehow."
+  end
+
   def self.indirections
     Puppet::Indirector::Indirection.instances.collect { |t| t.to_s }.sort
   end
diff --git a/lib/puppet/string/option.rb b/lib/puppet/string/option.rb
index bdc3e07c5..26b769c2e 100644
--- a/lib/puppet/string/option.rb
+++ b/lib/puppet/string/option.rb
@@ -1,24 +1,69 @@
 class Puppet::String::Option
-  attr_reader :name, :string
+  attr_reader   :string
+  attr_reader   :name
+  attr_reader   :aliases
+  attr_accessor :desc
 
-  def initialize(string, name, attrs = {})
-    raise "#{name.inspect} is an invalid option name" unless name.to_s =~ /^[a-z]\w*$/
-    @string = string
-    @name   = name.to_sym
-    attrs.each do |k,v| send("#{k}=", v) end
+  def takes_argument?
+    !!@argument
+  end
+  def optional_argument?
+    !!@optional_argument
   end
 
+  def initialize(string, *declaration, &block)
+    @string   = string
+    @optparse = []
+
+    # Collect and sort the arguments in the declaration.
+    declaration.each do |item|
+      if item.is_a? String and item.to_s =~ /^-/ then
+        unless item =~ /^-[a-z]\b/ or item =~ /^--[^-]/ then
+          raise ArgumentError, "#{item.inspect}: long options need two dashes (--)"
+        end
+        @optparse << item
+      else
+        raise ArgumentError, "#{item.inspect} is not valid for an option argument"
+      end
+    end
+
+    if @optparse.empty? then
+      raise ArgumentError, "No option declarations found while building"
+    end
+
+    # Now, infer the name from the options; we prefer the first long option as
+    # the name, rather than just the first option.
+    @name = optparse_to_name(@optparse.find do |a| a =~ /^--/ end || @optparse.first)
+    @aliases = @optparse.map { |o| optparse_to_name(o) }
+
+    # Do we take an argument?  If so, are we consistent about it, because
+    # incoherence here makes our life super-difficult, and we can more easily
+    # relax this rule later if we find a valid use case for it. --daniel 2011-03-30
+    @argument = @optparse.any? { |o| o =~ /[ =]/ }
+    if @argument and not @optparse.all? { |o| o =~ /[ =]/ } then
+      raise ArgumentError, "Option #{@name} is inconsistent about taking an argument"
+    end
+
+    # Is our argument optional?  The rules about consistency apply here, also,
+    # just like they do to taking arguments at all. --daniel 2011-03-30
+    @optional_argument = @optparse.any? { |o| o.include? "[" }
+    if @optional_argument and not @optparse.all? { |o| o.include? "[" } then
+      raise ArgumentError, "Option #{@name} is inconsistent about the argument being optional"
+    end
+  end
+
+  # to_s and optparse_to_name are roughly mirrored, because they are used to
+  # transform strings to name symbols, and vice-versa.
   def to_s
     @name.to_s.tr('_', '-')
   end
 
-  Types = [:boolean, :string]
-  def type
-    @type ||= :boolean
-  end
-  def type=(input)
-    value = begin input.to_sym rescue nil end
-    Types.include?(value) or raise ArgumentError, "#{input.inspect} is not a valid type"
-    @type = value
+  def optparse_to_name(declaration)
+    unless found = declaration.match(/^-+([^= ]+)/) or found.length != 1 then
+      raise ArgumentError, "Can't find a name in the declaration #{declaration.inspect}"
+    end
+    name = found.captures.first.tr('-', '_')
+    raise "#{name.inspect} is an invalid option name" unless name.to_s =~ /^[a-z]\w*$/
+    name.to_sym
   end
 end
diff --git a/lib/puppet/string/option_builder.rb b/lib/puppet/string/option_builder.rb
index 2087cbc99..da0d213fb 100644
--- a/lib/puppet/string/option_builder.rb
+++ b/lib/puppet/string/option_builder.rb
@@ -3,14 +3,14 @@ require 'puppet/string/option'
 class Puppet::String::OptionBuilder
   attr_reader :option
 
-  def self.build(string, name, attrs = {}, &block)
-    new(string, name, attrs, &block).option
+  def self.build(string, *declaration, &block)
+    new(string, *declaration, &block).option
   end
 
   private
-  def initialize(string, name, attrs, &block)
+  def initialize(string, *declaration, &block)
     @string = string
-    @option = Puppet::String::Option.new(string, name, attrs)
+    @option = Puppet::String::Option.new(string, *declaration)
     block and instance_eval(&block)
     @option
   end
diff --git a/lib/puppet/string/option_manager.rb b/lib/puppet/string/option_manager.rb
index df3ae6b4b..f952ad4f0 100644
--- a/lib/puppet/string/option_manager.rb
+++ b/lib/puppet/string/option_manager.rb
@@ -3,16 +3,26 @@ require 'puppet/string/option_builder'
 module Puppet::String::OptionManager
   # Declare that this app can take a specific option, and provide
   # the code to do so.
-  def option(name, attrs = {}, &block)
-    @options ||= {}
-    raise ArgumentError, "Option #{name} already defined for #{self}" if option?(name)
-    actions.each do |action|
-      if get_action(action).option?(name) then
-        raise ArgumentError, "Option #{name} already defined on action #{action} for #{self}"
+  def option(*declaration, &block)
+    add_option Puppet::String::OptionBuilder.build(self, *declaration, &block)
+  end
+
+  def add_option(option)
+    option.aliases.each do |name|
+      if conflict = get_option(name) then
+        raise ArgumentError, "Option #{option} conflicts with existing option #{conflict}"
+      end
+
+      actions.each do |action|
+        action = get_action(action)
+        if conflict = action.get_option(name) then
+          raise ArgumentError, "Option #{option} conflicts with existing option #{conflict} on #{action}"
+        end
       end
     end
-    option = Puppet::String::OptionBuilder.build(self, name, &block)
-    @options[option.name] = option
+
+    option.aliases.each { |name| @options[name] = option }
+    option
   end
 
   def options
-- 
cgit 


From 3bb614525f625a688baf8d67c5a580f8a51f4cad Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Wed, 30 Mar 2011 16:56:08 -0700
Subject: (#6749) fix an inheritance bug in ActionManager

When we wrote class inheritance of actions for strings we didn't implement
method (ahem, action) lookup correctly.  This changes that, by providing the
implementation to our standards, along with appropriate tests.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/string/action_manager.rb | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

(limited to 'lib/puppet/string')

diff --git a/lib/puppet/string/action_manager.rb b/lib/puppet/string/action_manager.rb
index c980142ce..7d22a0c52 100644
--- a/lib/puppet/string/action_manager.rb
+++ b/lib/puppet/string/action_manager.rb
@@ -32,7 +32,15 @@ module Puppet::String::ActionManager
 
   def get_action(name)
     @actions ||= {}
-    @actions[name.to_sym]
+    result = @actions[name.to_sym]
+    if result.nil?
+      if self.is_a?(Class) and superclass.respond_to?(:get_action)
+        result = superclass.get_action(name)
+      elsif self.class.respond_to?(:get_action)
+        result = self.class.get_action(name)
+      end
+    end
+    return result
   end
 
   def action?(name)
-- 
cgit 


From 512778f95058a423a3d2e08d1803eb4a90fb975a Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Wed, 30 Mar 2011 16:56:40 -0700
Subject: (#6749) detect duplicate aliases in a single option statement.

This ensures that an option declaration that shadows itself is found, and
reported to the user, rather than silently eating one of the two.

This could have actually lost, for example, the distinction between an
argument-requiring and an argument-missing variant of the same thing.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/string/option.rb | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

(limited to 'lib/puppet/string')

diff --git a/lib/puppet/string/option.rb b/lib/puppet/string/option.rb
index 26b769c2e..70d62a01f 100644
--- a/lib/puppet/string/option.rb
+++ b/lib/puppet/string/option.rb
@@ -1,5 +1,5 @@
 class Puppet::String::Option
-  attr_reader   :string
+  attr_reader   :parent
   attr_reader   :name
   attr_reader   :aliases
   attr_accessor :desc
@@ -11,17 +11,26 @@ class Puppet::String::Option
     !!@optional_argument
   end
 
-  def initialize(string, *declaration, &block)
-    @string   = string
+  def initialize(parent, *declaration, &block)
+    @parent   = parent
     @optparse = []
 
     # Collect and sort the arguments in the declaration.
+    dups = {}
     declaration.each do |item|
       if item.is_a? String and item.to_s =~ /^-/ then
         unless item =~ /^-[a-z]\b/ or item =~ /^--[^-]/ then
           raise ArgumentError, "#{item.inspect}: long options need two dashes (--)"
         end
         @optparse << item
+
+        # Duplicate checking...
+        name = optparse_to_name(item)
+        if dup = dups[name] then
+          raise ArgumentError, "#{item.inspect}: duplicates existing alias #{dup.inspect} in #{@parent}"
+        else
+          dups[name] = item
+        end
       else
         raise ArgumentError, "#{item.inspect} is not valid for an option argument"
       end
-- 
cgit 


From 423fe1f6b7c5bc0ca9b53a87f636668514802ad7 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Wed, 30 Mar 2011 16:58:17 -0700
Subject: (#6749) string cli base: implement preinit CLI parsing

In order to identify the full set of options we need to know the action that
is being invoked; that actually requires a pre-processing step to identify
that out of the global options.

Notably, our spec is that options can be to the right of their declaration
point, but not to the left: that means that we can now extract the full set of
options, and interact with the standard Puppet option handling code, resulting
in at least vaguely saner behaviour...

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/string/option.rb | 1 +
 1 file changed, 1 insertion(+)

(limited to 'lib/puppet/string')

diff --git a/lib/puppet/string/option.rb b/lib/puppet/string/option.rb
index 70d62a01f..e7b6f187c 100644
--- a/lib/puppet/string/option.rb
+++ b/lib/puppet/string/option.rb
@@ -2,6 +2,7 @@ class Puppet::String::Option
   attr_reader   :parent
   attr_reader   :name
   attr_reader   :aliases
+  attr_reader   :optparse
   attr_accessor :desc
 
   def takes_argument?
-- 
cgit 


From c52261c7aa86e7e75f215ba0f6b8c140003c4ead Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Thu, 31 Mar 2011 13:36:19 -0700
Subject: (#6749) disable Action#invoke for this release.

So, we have Action#invoke, but it binds to the declaring class, not to the
correct instance.  Solving all the subtle issues around threads, global state,
and bindings without causing us too much pain is actually pretty hard, so
instead we pull the feature.

It can be enabled again in a future release and, being a strict extension
feature, we can do that without overly hurting anyone.  We still have full
access to the invocation through a marginally less pleasant syntax, but one
that people MUST be able to arrange to allow invoke to work, so on that front
we are clean.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/string/action.rb | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

(limited to 'lib/puppet/string')

diff --git a/lib/puppet/string/action.rb b/lib/puppet/string/action.rb
index 692e467b4..9e82f4d5d 100644
--- a/lib/puppet/string/action.rb
+++ b/lib/puppet/string/action.rb
@@ -1,3 +1,4 @@
+# -*- coding: utf-8 -*-
 require 'puppet/string'
 require 'puppet/string/option'
 
@@ -16,9 +17,39 @@ class Puppet::String::Action
     attrs.each do |k,v| send("#{k}=", v) end
   end
 
-  def invoke(*args, &block)
-    @string.method(name).call(*args,&block)
-  end
+  # Initially, this was defined to allow the @action.invoke pattern, which is
+  # a very natural way to invoke behaviour given our introspection
+  # capabilities.   Heck, our initial plan was to have the string delegate to
+  # the action object for invocation and all.
+  #
+  # It turns out that we have a binding problem to solve: @string was bound to
+  # the parent class, not the subclass instance, and we don't pass the
+  # appropriate context or change the binding enough to make this work.
+  #
+  # We could hack around it, by either mandating that you pass the context in
+  # to invoke, or try to get the binding right, but that has probably got
+  # subtleties that we don't instantly think of – especially around threads.
+  #
+  # So, we are pulling this method for now, and will return it to life when we
+  # have the time to resolve the problem.  For now, you should replace...
+  #
+  #     @action = @string.get_action(name)
+  #     @action.invoke(arg1, arg2, arg3)
+  #
+  # ...with...
+  #
+  #     @action = @string.get_action(name)
+  #     @string.send(@action.name, arg1, arg2, arg3)
+  #
+  # I understand that is somewhat cumbersome, but it functions as desired.
+  # --daniel 2011-03-31
+  #
+  # PS: This code is left present, but commented, to support this chunk of
+  # documentation, for the benefit of the reader.
+  #
+  # def invoke(*args, &block)
+  #   @string.send(name, *args, &block)
+  # end
 
   def invoke=(block)
     if @string.is_a?(Class)
-- 
cgit 


From 3d88808270e9a0cb848a66825c66676598559dc3 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Thu, 31 Mar 2011 16:46:43 -0700
Subject: (#6749) base indirector string should fail on invalid terminus.

We used to generate an info-level message, then carry on, which typically
resulted in raising because 'nil' doesn't implement the expected method that
we immediately try to call.

So, instead, raise a clear error at the time we fail to load, which gives a
pleasant rather than confusing error to the user.  Which at least means they
know why things have gone wrong...

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/string/indirector.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

(limited to 'lib/puppet/string')

diff --git a/lib/puppet/string/indirector.rb b/lib/puppet/string/indirector.rb
index 71b1f3b12..0f5f405ff 100644
--- a/lib/puppet/string/indirector.rb
+++ b/lib/puppet/string/indirector.rb
@@ -59,7 +59,8 @@ that we should describe in this file somehow."
   # One usually does.
   def indirection
     unless @indirection
-      Puppet.info("Could not find terminus for #{indirection_name}") unless @indirection = Puppet::Indirector::Indirection.instance(indirection_name)
+      @indirection = Puppet::Indirector::Indirection.instance(indirection_name)
+      @indirection or raise "Could not find terminus for #{indirection_name}"
     end
     @indirection
   end
-- 
cgit 


From eb4c4fbdc3951c220a76ec01abc33a7654d89e53 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Fri, 1 Apr 2011 10:44:35 -0700
Subject: (#6749) Start porting existing strings to the options API.

This provides a solid test of the new code, by migrating the existing strings
to match.  This also gives us a chance to determine any weak points in the
code as written.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/string/action.rb         | 2 +-
 lib/puppet/string/catalog.rb        | 4 ++--
 lib/puppet/string/catalog/select.rb | 2 +-
 lib/puppet/string/certificate.rb    | 6 +++---
 lib/puppet/string/config.rb         | 1 +
 lib/puppet/string/configurer.rb     | 2 +-
 lib/puppet/string/facts.rb          | 2 +-
 lib/puppet/string/report.rb         | 2 +-
 8 files changed, 11 insertions(+), 10 deletions(-)

(limited to 'lib/puppet/string')

diff --git a/lib/puppet/string/action.rb b/lib/puppet/string/action.rb
index 9e82f4d5d..ff419c090 100644
--- a/lib/puppet/string/action.rb
+++ b/lib/puppet/string/action.rb
@@ -14,7 +14,7 @@ class Puppet::String::Action
     @string  = string
     @name    = name.to_sym
     @options = {}
-    attrs.each do |k,v| send("#{k}=", v) end
+    attrs.each do |k, v| send("#{k}=", v) end
   end
 
   # Initially, this was defined to allow the @action.invoke pattern, which is
diff --git a/lib/puppet/string/catalog.rb b/lib/puppet/string/catalog.rb
index 0ddd83176..c6de47708 100644
--- a/lib/puppet/string/catalog.rb
+++ b/lib/puppet/string/catalog.rb
@@ -2,7 +2,7 @@ require 'puppet/string/indirector'
 
 Puppet::String::Indirector.define(:catalog, '0.0.1') do
   action(:apply) do
-    invoke do |catalog|
+    invoke do |catalog, options|
       report = Puppet::Transaction::Report.new("apply")
       report.configuration_version = catalog.version
 
@@ -23,7 +23,7 @@ Puppet::String::Indirector.define(:catalog, '0.0.1') do
   end
 
   action(:download) do
-    invoke do |certname,facts|
+    invoke do |certname, facts, options|
       Puppet::Resource::Catalog.terminus_class = :rest
       facts_to_upload = {:facts_format => :b64_zlib_yaml, :facts => CGI.escape(facts.render(:b64_zlib_yaml))}
       catalog = nil
diff --git a/lib/puppet/string/catalog/select.rb b/lib/puppet/string/catalog/select.rb
index 52c77d3ce..a8f4480cd 100644
--- a/lib/puppet/string/catalog/select.rb
+++ b/lib/puppet/string/catalog/select.rb
@@ -1,7 +1,7 @@
 # Select and show a list of resources of a given type.
 Puppet::String.define(:catalog, '0.0.1') do
   action :select do
-    invoke do |host,type|
+    invoke do |host, type, options|
       catalog = Puppet::Resource::Catalog.indirection.find(host)
 
       catalog.resources.reject { |res| res.type != type }.each { |res| puts res }
diff --git a/lib/puppet/string/certificate.rb b/lib/puppet/string/certificate.rb
index 7b2e5f397..53f731e81 100644
--- a/lib/puppet/string/certificate.rb
+++ b/lib/puppet/string/certificate.rb
@@ -4,7 +4,7 @@ require 'puppet/ssl/host'
 Puppet::String::Indirector.define(:certificate, '0.0.1') do
 
   action :generate do
-    invoke do |name|
+    invoke do |name, options|
       host = Puppet::SSL::Host.new(name)
       host.generate_certificate_request
       host.certificate_request.class.indirection.save(host.certificate_request)
@@ -12,7 +12,7 @@ Puppet::String::Indirector.define(:certificate, '0.0.1') do
   end
 
   action :list do
-    invoke do
+    invoke do |options|
       Puppet::SSL::Host.indirection.search("*", {
         :for => :certificate_request,
       }).map { |h| h.inspect }
@@ -20,7 +20,7 @@ Puppet::String::Indirector.define(:certificate, '0.0.1') do
   end
 
   action :sign do
-    invoke do |name|
+    invoke do |name, options|
       Puppet::SSL::Host.indirection.save(Puppet::SSL::Host.new(name))
     end
   end
diff --git a/lib/puppet/string/config.rb b/lib/puppet/string/config.rb
index ae1a408cf..49a1688fc 100644
--- a/lib/puppet/string/config.rb
+++ b/lib/puppet/string/config.rb
@@ -3,6 +3,7 @@ require 'puppet/string'
 Puppet::String.define(:config, '0.0.1') do
   action(:print) do
     invoke do |*args|
+      options = args.pop
       Puppet.settings[:configprint] = args.join(",")
       Puppet.settings.print_config_options
       nil
diff --git a/lib/puppet/string/configurer.rb b/lib/puppet/string/configurer.rb
index a6ea74b6a..2520d4188 100644
--- a/lib/puppet/string/configurer.rb
+++ b/lib/puppet/string/configurer.rb
@@ -2,7 +2,7 @@ require 'puppet/string'
 
 Puppet::String.define(:configurer, '0.0.1') do
   action(:synchronize) do
-    invoke do |certname|
+    invoke do |certname, options|
       facts = Puppet::String[:facts, '0.0.1'].find(certname)
       catalog = Puppet::String[:catalog, '0.0.1'].download(certname, facts)
       report = Puppet::String[:catalog, '0.0.1'].apply(catalog)
diff --git a/lib/puppet/string/facts.rb b/lib/puppet/string/facts.rb
index 73acb0df6..31298813b 100644
--- a/lib/puppet/string/facts.rb
+++ b/lib/puppet/string/facts.rb
@@ -6,7 +6,7 @@ Puppet::String::Indirector.define(:facts, '0.0.1') do
 
   # Upload our facts to the server
   action(:upload) do
-    invoke do |*args|
+    invoke do |options|
       Puppet::Node::Facts.indirection.terminus_class = :facter
       facts = Puppet::Node::Facts.indirection.find(Puppet[:certname])
       Puppet::Node::Facts.indirection.terminus_class = :rest
diff --git a/lib/puppet/string/report.rb b/lib/puppet/string/report.rb
index 55a008533..5b617e49e 100644
--- a/lib/puppet/string/report.rb
+++ b/lib/puppet/string/report.rb
@@ -2,7 +2,7 @@ require 'puppet/string/indirector'
 
 Puppet::String::Indirector.define(:report, '0.0.1') do
   action(:submit) do
-    invoke do |report|
+    invoke do |report, options|
       begin
         Puppet::Transaction::Report.terminus_class = :rest
         report.save
-- 
cgit 


From 4d23d60fc331220418d4502930bd2fad7ee44b84 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Fri, 1 Apr 2011 10:48:07 -0700
Subject: (#6749) add a shim around the action implementation.

To present a pleasant Ruby API to folks invoking actions we want to have
default values for the trailing 'options' argument, and to be able to pass a
block to the code to allow yield to work.

Given limitations of Ruby 1.8 regarding blocks, we can't use either of those
because the block slot is bound at declaration time, and you can't give
optional arguments.

To work around this we inject, using eval, a full regular Ruby method into the
final block of code.  This can provide the default argument at the end in an
intelligent way (albeit by emulating what the interpreter would do).

This doesn't solve the yield problem, but the same method can pass the block
explicitly, and allows other hooks to be injected, which makes it easier to do
smarter things in future...

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/string/action.rb     | 36 ++++++++++++++++++++++++++++++++++--
 lib/puppet/string/indirector.rb |  2 +-
 2 files changed, 35 insertions(+), 3 deletions(-)

(limited to 'lib/puppet/string')

diff --git a/lib/puppet/string/action.rb b/lib/puppet/string/action.rb
index ff419c090..ee3b2991b 100644
--- a/lib/puppet/string/action.rb
+++ b/lib/puppet/string/action.rb
@@ -52,10 +52,42 @@ class Puppet::String::Action
   # end
 
   def invoke=(block)
+    # We need to build an instance method as a wrapper, using normal code, to
+    # be able to expose argument defaulting between the caller and definer in
+    # the Ruby API.  An extra method is, sadly, required for Ruby 1.8 to work.
+    #
+    # In future this also gives us a place to hook in additional behaviour
+    # such as calling out to the action instance to validate and coerce
+    # parameters, which avoids any exciting context switching and all.
+    #
+    # Hopefully we can improve this when we finally shuffle off the last of
+    # Ruby 1.8 support, but that looks to be a few "enterprise" release eras
+    # away, so we are pretty stuck with this for now.
+    #
+    # Patches to make this work more nicely with Ruby 1.9 using runtime
+    # version checking and all are welcome, but they can't actually help if
+    # the results are not totally hidden away in here.
+    #
+    # Incidentally, we though about vendoring evil-ruby and actually adjusting
+    # the internal C structure implementation details under the hood to make
+    # this stuff work, because it would have been cleaner.  Which gives you an
+    # idea how motivated we were to make this cleaner.  Sorry. --daniel 2011-03-31
+
+    internal_name = "#{@name} implementation, required on Ruby 1.8".to_sym
+    file = __FILE__ + "+eval"
+    line = __LINE__ + 1
+    wrapper = "def #{@name}(*args, &block)
+                 args << {} unless args.last.is_a? Hash
+                 args << block if block_given?
+                 self.__send__(#{internal_name.inspect}, *args)
+               end"
+
     if @string.is_a?(Class)
-      @string.define_method(@name, &block)
+      @string.class_eval do eval wrapper, nil, file, line end
+      @string.define_method(internal_name, &block)
     else
-      @string.meta_def(@name, &block)
+      @string.instance_eval do eval wrapper, nil, file, line end
+      @string.meta_def(internal_name, &block)
     end
   end
 
diff --git a/lib/puppet/string/indirector.rb b/lib/puppet/string/indirector.rb
index 0f5f405ff..48280cc77 100644
--- a/lib/puppet/string/indirector.rb
+++ b/lib/puppet/string/indirector.rb
@@ -75,7 +75,7 @@ that we should describe in this file somehow."
 
   def call_indirection_method(method, *args)
     begin
-      result = indirection.send(method, *args)
+      result = indirection.__send__(method, *args)
     rescue => detail
       puts detail.backtrace if Puppet[:trace]
       raise "Could not call '#{method}' on '#{indirection_name}': #{detail}"
-- 
cgit 


From d328af73e688df136ee6fe10340adf7ba72b951e Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Fri, 1 Apr 2011 12:46:12 -0700
Subject: (#6760) set terminus in indirector string base class.

We now accept a terminus option to each invocation, and set the terminus based
on that call.  This is probably incomplete, because it only sets the terminus
when given, and doesn't try to reset it to the default afterwards.

This also resets the terminus class after every invocation, to stop it leaking
state across calls.  This make, sadly, have some effects if you are not just
using the strings to invoke the terminus, but it beats having the strings
broken as well...

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>
---
 lib/puppet/string/indirector.rb | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

(limited to 'lib/puppet/string')

diff --git a/lib/puppet/string/indirector.rb b/lib/puppet/string/indirector.rb
index 48280cc77..bb081533f 100644
--- a/lib/puppet/string/indirector.rb
+++ b/lib/puppet/string/indirector.rb
@@ -2,7 +2,6 @@ require 'puppet'
 require 'puppet/string'
 
 class Puppet::String::Indirector < Puppet::String
-  warn "REVISIT: Need to redefine this to take arguments again, eh."
   option "--terminus TERMINUS" do
     desc "REVISIT: You can select a terminus, which has some bigger effect
 that we should describe in this file somehow."
@@ -16,6 +15,21 @@ that we should describe in this file somehow."
     Puppet::Indirector::Terminus.terminus_classes(indirection.to_sym).collect { |t| t.to_s }.sort
   end
 
+  def call_indirection_method(method, *args)
+    options = args.pop
+    options.has_key?(:terminus) and set_terminus(options[:terminus])
+
+    begin
+      result = indirection.__send__(method, *args)
+    rescue => detail
+      puts detail.backtrace if Puppet[:trace]
+      raise "Could not call '#{method}' on '#{indirection_name}': #{detail}"
+    end
+
+    indirection.reset_terminus_class
+    return result
+  end
+
   action :destroy do
     invoke { |*args| call_indirection_method(:destroy, *args) }
   end
@@ -35,11 +49,16 @@ that we should describe in this file somehow."
   # Print the configuration for the current terminus class
   action :info do
     invoke do |*args|
+      options = args.pop
+      options.has_key?(:terminus) and set_terminus(options[:terminus])
+
       if t = indirection.terminus_class
         puts "Run mode '#{Puppet.run_mode.name}': #{t}"
       else
         $stderr.puts "No default terminus class for run mode '#{Puppet.run_mode.name}'"
       end
+
+      indirection.reset_terminus_class
     end
   end
 
@@ -72,15 +91,4 @@ that we should describe in this file somehow."
       raise "Could not set '#{indirection.name}' terminus to '#{from}' (#{detail}); valid terminus types are #{self.class.terminus_classes(indirection.name).join(", ") }"
     end
   end
-
-  def call_indirection_method(method, *args)
-    begin
-      result = indirection.__send__(method, *args)
-    rescue => detail
-      puts detail.backtrace if Puppet[:trace]
-      raise "Could not call '#{method}' on '#{indirection_name}': #{detail}"
-    end
-
-    result
-  end
 end
-- 
cgit 


From 5a0b547f3289cb8e13b197d021322e03d05bee8e Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 4 Apr 2011 11:04:17 -0700
Subject: (#6749) Fix optional vs mandatory argument handling.

optparse will treat '--foo --bar' as "foo with the argument --bar" when foo
takes a mandatory argument.  We need to emulate that behaviour in our
pre-parse of the command line.

Incidentally, fix up a bug in boolean options, and improve our testing.

Reviewed-By: Nick Lewis <nick@puppetlabs.com>
---
 lib/puppet/string/option.rb | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

(limited to 'lib/puppet/string')

diff --git a/lib/puppet/string/option.rb b/lib/puppet/string/option.rb
index e7b6f187c..f499e4b95 100644
--- a/lib/puppet/string/option.rb
+++ b/lib/puppet/string/option.rb
@@ -63,7 +63,8 @@ class Puppet::String::Option
   end
 
   # to_s and optparse_to_name are roughly mirrored, because they are used to
-  # transform strings to name symbols, and vice-versa.
+  # transform strings to name symbols, and vice-versa.  This isn't a full
+  # bidirectional transformation though.
   def to_s
     @name.to_s.tr('_', '-')
   end
@@ -72,7 +73,7 @@ class Puppet::String::Option
     unless found = declaration.match(/^-+([^= ]+)/) or found.length != 1 then
       raise ArgumentError, "Can't find a name in the declaration #{declaration.inspect}"
     end
-    name = found.captures.first.tr('-', '_')
+    name = found.captures.first.sub('[no-]', '').tr('-', '_')
     raise "#{name.inspect} is an invalid option name" unless name.to_s =~ /^[a-z]\w*$/
     name.to_sym
   end
-- 
cgit