The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Feature #14149

The Puppet::Modules (or some other namespace) should be reserved for use by modules

Added by Jeff McCune over 2 years ago. Updated almost 2 years ago.

Status:ClosedStart date:04/23/2012
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:modules
Target version:-
Affected Puppet version:2.6.0 Branch:https://github.com/puppetlabs/puppet/pull/704
Keywords:module namespace constant class modules utility util methods constants

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This issue is currently not available for export. If you are experiencing the issue described below, please file a new ticket in JIRA. Once a new ticket has been created, please add a link to it that points back to this Redmine ticket.


Description

Overview

3rd party modules often need to define their own constants in the form of Ruby module or class objects. Puppet does not provide a safe namespace for this use. As a result, 3rd party applications may inadvertently monkey patch Puppet or Puppet may inadvertently monkey patch 3rd party software.

In the worst case, the software breaks if the constants are mutually exclusive, as was the case with this code from the Puppet Module tool and Puppet:

puppet-module tool

module Puppet
  class Module
  end
end

Puppet

module Puppet
  module Module
  end
end

This will break because Puppet::Module cannot exist as a class and a module at the same time. If it doesn’t break entirely, the two pieces of software will monkey patch each other.

Impact Data

All modules that need to define common utility code share this problem. We’ve largely been ignoring this problem as is evidenced by recent issues with the following pieces of software and their namespace conflicts

  • Puppet::Module
  • SemVer
  • Cloud Provisioner (Puppet 2.7.12 + Cloud Provisioner 1.0.4 installed results in err: no such file to load — puppet/face/node/classify err: Try ‘puppet help help help’ for usage)
  • Registry utility methods
  • Network Device Support Puppet::Util::NetworkDevice::F5
  • Any module that defines constants inside of the Puppet ruby module.

Initial Proposal

As an initial proposal to put a band aid on the bullet wound we have, we should reserve Puppet::Modules as a namespace for 3rd party modules. Puppet should have a place holder lib/puppet/modules.rb file that defines Puppet::Modules as a ruby module and is otherwise an empty object. Puppet modules may then open this object and monkey patch it as they see fit.


Related issues

Related to Puppet - Bug #13682: Puppet should not conflict with released puppet-module Gem Closed 04/08/2012
Related to Puppet - Feature #4248: Load "library" plugins that are used by multiple puppet f... Accepted 07/15/2010
Related to Puppet - Feature #7788: Puppet should allow rubygems to deliver new functionality Closed 06/04/2011
Related to Puppet - Bug #15300: Pluginsync collisions Accepted 06/29/2012

History

#1 Updated by Jeff McCune over 2 years ago

  • Status changed from Unreviewed to In Topic Branch Pending Review
  • Branch set to https://github.com/puppetlabs/puppet/pull/704

Pull Request

This is mostly to get the discussion started, but I have a pull request with spec tests that at least reserves Puppet::Modules to reduce the rash of maintenance issues we’ve had recently.

https://github.com/puppetlabs/puppet/pull/704

#2 Updated by Nan Liu over 2 years ago

Due to how network device is initialized, we need some refactor to get network device out of util:

https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/network_device.rb

def self.init(device)
  require "puppet/util/network_device/#{device.provider}/device"
  @current = Puppet::Util::NetworkDevice.const_get(device.provider.capitalize).const_get(:Device).new(device.url)
rescue => detail
  raise "Can't load #{device.provider} for #{device.name}: #{detail}"
end

There was some desire to spin out cisco devices from Puppet core, and any decision related to Puppet::Module and best practices for modules would certainly impact that migration.

#3 Updated by Anonymous over 2 years ago

  • Status changed from In Topic Branch Pending Review to Rejected

This doesn’t actually solve the problem, just makes it vaguely more unlikely. A much better solution is that third party code uses a namespace that isn’t owned by someone else, just like the policy that every other Ruby project does.

#4 Updated by R.I. Pienaar over 2 years ago

Do we want pluginsync to sync out and load in random other namespaces?

#5 Updated by Anonymous over 2 years ago

R.I. Pienaar wrote:

Do we want pluginsync to sync out and load in random other namespaces?

No. On which point, PluginSync no longer loads anything, ever, unless it was already loaded.

Where we do automatically load things, it is in existing, well defined parts of the namespace – custom functions, terminus implementations – that are not even remotely associated with this “reserve a namespace for modules”.

So, the only thing that would load code outside the Puppet namespace is the custom code that got loaded to implement your extension point. The user makes the decision about loading things outside the namespace, presumably at the same time they implement the thing the we will automatically load.

#6 Updated by R.I. Pienaar over 2 years ago

So it seems the desire for a known namespace where ppl can put their own other classes and such is specifically so that they can get it pluginsynced out. I’ve faced this problem many times while writing little things to extend puppet. You can either create a big horrible file with all your classes in it one and pluginsync that or suffer.

To keep your code clean u want some sub classes, modules etc and people tend to put this in Puppet::Util now but thats in the past caused problems too.

So I think we do need some known place where arbitrary code can live and we should cater for that namespace in pluginsync.

#7 Updated by Anonymous over 2 years ago

R.I. Pienaar wrote:

So it seems the desire for a known namespace where ppl can put their own other classes and such is specifically so that they can get it pluginsynced out.

Sure, and that works correctly today, and will continue to work tomorrow. PluginSync does not impose any limitation on what it will transfer to the client. It just doesn’t load anything itself – the autoloader does that, and it doesn’t care how those files got there. It treats PluginSync and other install sources equally.

To keep your code clean u want some sub classes, modules etc and people tend to put this in Puppet::Util now but thats in the past caused problems too.

So I think we do need some known place where arbitrary code can live and we should cater for that namespace in pluginsync.

…and we absolutely do: if you put content in ::AwesomeSauce, PluginSync will put it on the client and arrange for it to be available to you. When your code is loaded by Puppet, in one of the defined entry points, it can load your utility code from a distinct namespace.

The idea that a magic namespace inside Puppet that has no behaviour at all adds value here is, I think, the core of the problem. Everything you want to do should work today. If it doesn’t, it is a bug in Puppet. None of it requires this magic namespace.

#8 Updated by R.I. Pienaar over 2 years ago

OK, I will test again with 2.7, last time I checked this did not work as expected.

#9 Updated by Jeff McCune over 2 years ago

So what’s the story we’re telling?

My proposal was “Define your own module constants inside of Puppet::Modules, e.g. Puppet::Modules::Registry.”

Is the new story, “Define your own module constants anywhere you want, just NOT inside of Puppet?”

#10 Updated by Anonymous over 2 years ago

Jeff McCune wrote:

So what’s the story we’re telling? My proposal was “Define your own module constants inside of Puppet::Modules, e.g. Puppet::Modules::Registry.” Is the new story, “Define your own module constants anywhere you want, just NOT inside of Puppet?”

The story is “define your constants anywhere you please, the same way the rest of Ruby works”. If you want to define constants inside the namespace owned by another process, you can do that. You run all the risks that putting your content into a namespace you don’t own implies.

Unless we operate the IANA of things under that namespace, you still have the same problem. Two people can happily put things into the “MySQL” or “Database”, or whatever namespace with or without “Puppet::Modules” stuck on the front.

#11 Updated by Jeff McCune over 2 years ago

  • Status changed from Rejected to Needs More Information

I need to send an email to puppet-dev to get some feedback about putting it in Puppet::Modules or something else like Com::PuppetLabs::Registry. Based on this feedback and if we clearly document the convention in the “extending puppet” documentation this might be accepted again.

#12 Updated by Jeff McCune over 2 years ago

  • Assignee set to Jeff McCune

#14 Updated by Andrew Parker over 2 years ago

Jeff and I just talked about this. Daniel is right that his doesn’t solve the problem of people having safe sandboxes where their code cannot interfere with the code of other modules. However, it can be used to setup a convention whereby a well-behaved module has a known place that it can put things and no problems will arise if they follow the convention.

The thing that bugs me about this convention as it stands, is that it places more code under the puppet namespace. I suggest that instead of Puppet::Modules we have PuppetX, which is a convention that I have seen used for extension code in other systems. The convention would then be that if you are writing a module called mysql you place your code (except for types, providers. and functions) under PuppetX::Mysql. If it was done this way, then there would be no need to reserve anything in the puppet codebase and this would be a thing of just publishing a convention that module authors should use and we should promote.

#15 Updated by Anonymous over 2 years ago

On Thu, Jun 14, 2012 at 1:38 PM, tickets@puppetlabs.com wrote:

Jeff and I just talked about this. Daniel is right that his doesn’t solve the problem of people having safe sandboxes where their code cannot interfere with the code of other modules. However, it can be used to setup a convention whereby a well-behaved module has a known place that it can put things and no problems will arise if they follow the convention.

That isn’t true: they will have no problems if they follow the convention AND nobody else uses a conflicting name.

Which, as experience with the Puppet::Util namespace being used for this, isn’t assured by just throwing your terminal into that.

A safer approach would be to establish a convention like the Java com.puppetlabs.whatever naming.

(…or enforce that there is only ever one module for a given name. I think the current forge effectively disproves that theory. :)

#16 Updated by Jeff McCune over 2 years ago

So how about PuppetX::PuppetLabs::Registry

The convention and story being: “Start with PuppetX, put the CamelCase of your author name (from the Modulefile), then put the actual CamelCase of your module name.

PuppetX::::

-Jeff

#17 Updated by Anonymous over 2 years ago

Issue #14149 has been updated by Jeff McCune.

So how about PuppetX::PuppetLabs::Registry

The convention and story being: “Start with PuppetX, put the CamelCase of your author name (from the Modulefile), then put the actual CamelCase of your module name.

That avoids the conflicts, indeed.

#18 Updated by Jeff McCune about 2 years ago

  • Status changed from Needs More Information to Closed
  • Assignee deleted (Jeff McCune)

#19 Updated by Jeff McCune about 2 years ago

An example of how to migrate code running into #4248 and #14149 to the work-around can be found at:

https://github.com/puppetlabs/puppetlabs-registry/pull/18/files

From 4e780aa661e3989d1b488686d1085e3b23203d31 Mon Sep 17 00:00:00 2001
From: Jeff McCune 
Date: Wed, 25 Jul 2012 07:30:08 -0700
Subject: [PATCH 1/2] (#14149) Move utility code to puppet_x convention

This patch is a simple rename only change in preparation for the real
patch of migrating Puppet::Modules::Registry to
PuppetX::Puppetlabs::Registry
---
 lib/puppet/modules.rb                             |    8 -
 lib/puppet/modules/registry.rb                    |  163 ---------------------
 lib/puppet/modules/registry/provider_base.rb      |   88 -----------
 lib/puppet_x/puppetlabs/registry.rb               |  163 +++++++++++++++++++++
 lib/puppet_x/puppetlabs/registry/provider_base.rb |   88 +++++++++++
 5 files changed, 251 insertions(+), 259 deletions(-)
 delete mode 100644 lib/puppet/modules.rb
 delete mode 100644 lib/puppet/modules/registry.rb
 delete mode 100644 lib/puppet/modules/registry/provider_base.rb
 create mode 100644 lib/puppet_x/puppetlabs/registry.rb
 create mode 100644 lib/puppet_x/puppetlabs/registry/provider_base.rb

diff --git a/lib/puppet/modules.rb b/lib/puppet/modules.rb
deleted file mode 100644
index 6d12bdd..0000000
--- a/lib/puppet/modules.rb
+++ /dev/null
@@ -1,8 +0,0 @@
-# Open up the Puppet::Modules namespace for use by the registry module utility
-# methods.  This will throw an error if there is a Puppet::Modules constant
-# that is not itself a module.  (e.g. it may be a class as was the case with
-# Puppet::Module and the puppet-module tool.
-module Puppet
-  module Modules
-  end
-end
diff --git a/lib/puppet/modules/registry.rb b/lib/puppet/modules/registry.rb
deleted file mode 100644
index b6d0477..0000000
--- a/lib/puppet/modules/registry.rb
+++ /dev/null
@@ -1,163 +0,0 @@
-require 'pathname'
-# JJM WORK_AROUND
-# explicitly require files without relying on $LOAD_PATH until #14073 is fixed.
-# https://projects.puppetlabs.com/issues/14073 is fixed.
-require Pathname.new(__FILE__).dirname.expand_path
-
-module Puppet::Modules::Registry
-  # For 64-bit OS, use 64-bit view. Ignored on 32-bit OS
-  KEY_WOW64_64KEY = 0x100 unless defined? KEY_WOW64_64KEY
-  # For 64-bit OS, use 32-bit view. Ignored on 32-bit OS
-  KEY_WOW64_32KEY = 0x200 unless defined? KEY_WOW64_32KEY
-
-  # This is the base class for Path manipulation.  This class is meant to be
-  # abstract, RegistryKeyPath and RegistryValuePath will customize and override
-  # this class.
-  class RegistryPathBase < String
-    attr_reader :path
-    def initialize(path)
-      @filter_path_memo = nil
-      @path ||= path
-      super(path)
-    end
-
-    # The path is valid if we're able to parse it without exceptions.
-    def valid?
-      (filter_path and true) rescue false
-    end
-
-    def canonical
-      filter_path[:canonical]
-    end
-
-    # This method is meant to help setup aliases so autorequire can sort itself
-    # out in a case insensitive but preserving manner.  It returns an array of
-    # resource identifiers.
-    def aliases
-      [canonical.downcase]
-    end
-
-    def access
-      filter_path[:access]
-    end
-
-    def root
-      filter_path[:root]
-    end
-
-    def ascend(&block)
-      p = canonical
-      while idx = p.rindex('\\')
-        p = p[0, idx]
-        yield p
-      end
-    end
-
-    private
-
-    def filter_path
-      if @filter_path_memo
-        return @filter_path_memo
-      end
-      result = {}
-
-      path = @path
-
-      result[:valuename] = case path[-1, 1]
-      when '\\'
-        result[:is_default] = true
-        ''
-      else
-        result[:is_default] = false
-        idx = path.rindex('\\') || 0
-        if idx > 0
-          path[idx+1..-1]
-        else
-          ''
-        end
-      end
-
-      # Strip off any trailing slash.
-      path = path.gsub(/\\*$/, '')
-
-      unless captures = /^(32:)?([h|H][^\\]*)((?:\\[^\\]{1,255})*)$/.match(path)
-        raise ArgumentError, "Invalid registry key: #{path}"
-      end
-
-      case captures[1]
-      when '32:'
-        result[:access] = Puppet::Modules::Registry::KEY_WOW64_32KEY
-        result[:prefix] = '32:'
-      else
-        result[:access] = Puppet::Modules::Registry::KEY_WOW64_64KEY
-        result[:prefix] = ''
-      end
-
-      # canonical root key symbol
-      result[:root] = case captures[2].to_s.downcase
-              when /hkey_local_machine/, /hklm/
-                :hklm
-              when /hkey_classes_root/, /hkcr/
-                :hkcr
-              when /hkey_current_user/, /hkcu/,
-                /hkey_users/, /hku/,
-                /hkey_current_config/, /hkcc/,
-                /hkey_performance_data/,
-                /hkey_performance_text/,
-                /hkey_performance_nlstext/,
-                /hkey_dyn_data/
-                raise ArgumentError, "Unsupported prefined key: #{path}"
-              else
-                raise ArgumentError, "Invalid registry key: #{path}"
-              end
-
-      result[:trailing_path] = captures[3]
-
-      result[:trailing_path].gsub!(/^\\/, '')
-
-      if result[:trailing_path].empty?
-        result[:canonical] = "#{result[:prefix]}#{result[:root].to_s}"
-      else
-        # Leading backslash is not part of the subkey name
-        result[:canonical] = "#{result[:prefix]}#{result[:root].to_s}\\#{result[:trailing_path]}"
-      end
-
-      @filter_path_memo = result
-    end
-  end
-
-  class RegistryKeyPath < RegistryPathBase
-    def subkey
-      filter_path[:trailing_path]
-    end
-  end
-
-  class RegistryValuePath < RegistryPathBase
-    def canonical
-      # This method gets called in the type and the provider.  We need to
-      # preserve the trailing backslash for the provider, otherwise it won't
-      # think this is a default value.
-      if default?
-        filter_path[:canonical] << "\\"
-      else
-        filter_path[:canonical]
-      end
-    end
-
-    def subkey
-      if default?
-        filter_path[:trailing_path]
-      else
-        filter_path[:trailing_path].gsub(/^(.*)\\.*$/, '\1')
-      end
-    end
-
-    def valuename
-      filter_path[:valuename]
-    end
-
-    def default?
-      !!filter_path[:is_default]
-    end
-  end
-end
diff --git a/lib/puppet/modules/registry/provider_base.rb b/lib/puppet/modules/registry/provider_base.rb
deleted file mode 100644
index f0c0edb..0000000
--- a/lib/puppet/modules/registry/provider_base.rb
+++ /dev/null
@@ -1,88 +0,0 @@
-require 'pathname' # JJM WORK_AROUND #14073
-require Pathname.new(__FILE__).dirname.expand_path
-
-# This module is meant to be mixed into the registry_key AND registry_value providers.
-module Puppet::Modules::Registry::ProviderBase
-  # This is a class method in order to be easily mocked in the spec tests.
-  def self.initialize_system_api
-    if Puppet.features.microsoft_windows?
-      begin
-        require 'win32/registry'
-      rescue LoadError => exc
-        msg = "Could not load the required win32/registry library (ErrorID 1EAD86E3-D533-4286-BFCB-CCE8B818DDEA) [#{exc.message}]"
-        Puppet.err msg
-        error = Puppet::Error.new(msg)
-        error.set_backtrace exc.backtrace
-        raise error
-      end
-    end
-  end
-
-  def self.included(base)
-    # Initialize the Win32 API.  This is a method call so the spec tests can
-    # easily mock the initialization of the Win32 libraries on non-win32
-    # systems.
-    initialize_system_api
-
-    # Define an hkeys class method in the eigenclass we're being mixed into.
-    # This is the one true place to define the root hives we support.
-    class << base
-      def hkeys
-        # REVISIT: I'd like to make this easier to mock and stub.
-        {
-          :hkcr => Win32::Registry::HKEY_CLASSES_ROOT,
-          :hklm => Win32::Registry::HKEY_LOCAL_MACHINE,
-        }
-      end
-    end
-  end
-
-  # The rest of these methods will be mixed in as instance methods into the
-  # provider class.  The path method is expected to be mixed in by the provider
-  # specific module, ProviderKeyBase or ProviderValueBase
-  def hkeys
-    self.class.hkeys
-  end
-
-  def hive
-    hkeys[path.root]
-  end
-
-  def access
-    path.access
-  end
-
-  def root
-    path.root
-  end
-
-  def subkey
-    path.subkey
-  end
-
-  def valuename
-    path.valuename
-  end
-
-  def type2name_map
-    {
-      Win32::Registry::REG_NONE      => :none,
-      Win32::Registry::REG_SZ        => :string,
-      Win32::Registry::REG_EXPAND_SZ => :expand,
-      Win32::Registry::REG_BINARY    => :binary,
-      Win32::Registry::REG_DWORD     => :dword,
-      Win32::Registry::REG_QWORD     => :qword,
-      Win32::Registry::REG_MULTI_SZ  => :array
-    }
-  end
-
-  def type2name(type)
-    type2name_map[type]
-  end
-
-  def name2type(name)
-    name2type = {}
-    type2name_map.each_pair {|k,v| name2type[v] = k}
-    name2type[name]
-  end
-end
diff --git a/lib/puppet_x/puppetlabs/registry.rb b/lib/puppet_x/puppetlabs/registry.rb
new file mode 100644
index 0000000..b6d0477
--- /dev/null
+++ b/lib/puppet_x/puppetlabs/registry.rb
@@ -0,0 +1,163 @@
+require 'pathname'
+# JJM WORK_AROUND
+# explicitly require files without relying on $LOAD_PATH until #14073 is fixed.
+# https://projects.puppetlabs.com/issues/14073 is fixed.
+require Pathname.new(__FILE__).dirname.expand_path
+
+module Puppet::Modules::Registry
+  # For 64-bit OS, use 64-bit view. Ignored on 32-bit OS
+  KEY_WOW64_64KEY = 0x100 unless defined? KEY_WOW64_64KEY
+  # For 64-bit OS, use 32-bit view. Ignored on 32-bit OS
+  KEY_WOW64_32KEY = 0x200 unless defined? KEY_WOW64_32KEY
+
+  # This is the base class for Path manipulation.  This class is meant to be
+  # abstract, RegistryKeyPath and RegistryValuePath will customize and override
+  # this class.
+  class RegistryPathBase < String
+    attr_reader :path
+    def initialize(path)
+      @filter_path_memo = nil
+      @path ||= path
+      super(path)
+    end
+
+    # The path is valid if we're able to parse it without exceptions.
+    def valid?
+      (filter_path and true) rescue false
+    end
+
+    def canonical
+      filter_path[:canonical]
+    end
+
+    # This method is meant to help setup aliases so autorequire can sort itself
+    # out in a case insensitive but preserving manner.  It returns an array of
+    # resource identifiers.
+    def aliases
+      [canonical.downcase]
+    end
+
+    def access
+      filter_path[:access]
+    end
+
+    def root
+      filter_path[:root]
+    end
+
+    def ascend(&block)
+      p = canonical
+      while idx = p.rindex('\\')
+        p = p[0, idx]
+        yield p
+      end
+    end
+
+    private
+
+    def filter_path
+      if @filter_path_memo
+        return @filter_path_memo
+      end
+      result = {}
+
+      path = @path
+
+      result[:valuename] = case path[-1, 1]
+      when '\\'
+        result[:is_default] = true
+        ''
+      else
+        result[:is_default] = false
+        idx = path.rindex('\\') || 0
+        if idx > 0
+          path[idx+1..-1]
+        else
+          ''
+        end
+      end
+
+      # Strip off any trailing slash.
+      path = path.gsub(/\\*$/, '')
+
+      unless captures = /^(32:)?([h|H][^\\]*)((?:\\[^\\]{1,255})*)$/.match(path)
+        raise ArgumentError, "Invalid registry key: #{path}"
+      end
+
+      case captures[1]
+      when '32:'
+        result[:access] = Puppet::Modules::Registry::KEY_WOW64_32KEY
+        result[:prefix] = '32:'
+      else
+        result[:access] = Puppet::Modules::Registry::KEY_WOW64_64KEY
+        result[:prefix] = ''
+      end
+
+      # canonical root key symbol
+      result[:root] = case captures[2].to_s.downcase
+              when /hkey_local_machine/, /hklm/
+                :hklm
+              when /hkey_classes_root/, /hkcr/
+                :hkcr
+              when /hkey_current_user/, /hkcu/,
+                /hkey_users/, /hku/,
+                /hkey_current_config/, /hkcc/,
+                /hkey_performance_data/,
+                /hkey_performance_text/,
+                /hkey_performance_nlstext/,
+                /hkey_dyn_data/
+                raise ArgumentError, "Unsupported prefined key: #{path}"
+              else
+                raise ArgumentError, "Invalid registry key: #{path}"
+              end
+
+      result[:trailing_path] = captures[3]
+
+      result[:trailing_path].gsub!(/^\\/, '')
+
+      if result[:trailing_path].empty?
+        result[:canonical] = "#{result[:prefix]}#{result[:root].to_s}"
+      else
+        # Leading backslash is not part of the subkey name
+        result[:canonical] = "#{result[:prefix]}#{result[:root].to_s}\\#{result[:trailing_path]}"
+      end
+
+      @filter_path_memo = result
+    end
+  end
+
+  class RegistryKeyPath < RegistryPathBase
+    def subkey
+      filter_path[:trailing_path]
+    end
+  end
+
+  class RegistryValuePath < RegistryPathBase
+    def canonical
+      # This method gets called in the type and the provider.  We need to
+      # preserve the trailing backslash for the provider, otherwise it won't
+      # think this is a default value.
+      if default?
+        filter_path[:canonical] << "\\"
+      else
+        filter_path[:canonical]
+      end
+    end
+
+    def subkey
+      if default?
+        filter_path[:trailing_path]
+      else
+        filter_path[:trailing_path].gsub(/^(.*)\\.*$/, '\1')
+      end
+    end
+
+    def valuename
+      filter_path[:valuename]
+    end
+
+    def default?
+      !!filter_path[:is_default]
+    end
+  end
+end
diff --git a/lib/puppet_x/puppetlabs/registry/provider_base.rb b/lib/puppet_x/puppetlabs/registry/provider_base.rb
new file mode 100644
index 0000000..f0c0edb
--- /dev/null
+++ b/lib/puppet_x/puppetlabs/registry/provider_base.rb
@@ -0,0 +1,88 @@
+require 'pathname' # JJM WORK_AROUND #14073
+require Pathname.new(__FILE__).dirname.expand_path
+
+# This module is meant to be mixed into the registry_key AND registry_value providers.
+module Puppet::Modules::Registry::ProviderBase
+  # This is a class method in order to be easily mocked in the spec tests.
+  def self.initialize_system_api
+    if Puppet.features.microsoft_windows?
+      begin
+        require 'win32/registry'
+      rescue LoadError => exc
+        msg = "Could not load the required win32/registry library (ErrorID 1EAD86E3-D533-4286-BFCB-CCE8B818DDEA) [#{exc.message}]"
+        Puppet.err msg
+        error = Puppet::Error.new(msg)
+        error.set_backtrace exc.backtrace
+        raise error
+      end
+    end
+  end
+
+  def self.included(base)
+    # Initialize the Win32 API.  This is a method call so the spec tests can
+    # easily mock the initialization of the Win32 libraries on non-win32
+    # systems.
+    initialize_system_api
+
+    # Define an hkeys class method in the eigenclass we're being mixed into.
+    # This is the one true place to define the root hives we support.
+    class << base
+      def hkeys
+        # REVISIT: I'd like to make this easier to mock and stub.
+        {
+          :hkcr => Win32::Registry::HKEY_CLASSES_ROOT,
+          :hklm => Win32::Registry::HKEY_LOCAL_MACHINE,
+        }
+      end
+    end
+  end
+
+  # The rest of these methods will be mixed in as instance methods into the
+  # provider class.  The path method is expected to be mixed in by the provider
+  # specific module, ProviderKeyBase or ProviderValueBase
+  def hkeys
+    self.class.hkeys
+  end
+
+  def hive
+    hkeys[path.root]
+  end
+
+  def access
+    path.access
+  end
+
+  def root
+    path.root
+  end
+
+  def subkey
+    path.subkey
+  end
+
+  def valuename
+    path.valuename
+  end
+
+  def type2name_map
+    {
+      Win32::Registry::REG_NONE      => :none,
+      Win32::Registry::REG_SZ        => :string,
+      Win32::Registry::REG_EXPAND_SZ => :expand,
+      Win32::Registry::REG_BINARY    => :binary,
+      Win32::Registry::REG_DWORD     => :dword,
+      Win32::Registry::REG_QWORD     => :qword,
+      Win32::Registry::REG_MULTI_SZ  => :array
+    }
+  end
+
+  def type2name(type)
+    type2name_map[type]
+  end
+
+  def name2type(name)
+    name2type = {}
+    type2name_map.each_pair {|k,v| name2type[v] = k}
+    name2type[name]
+  end
+end
-- 
1.7.10


From 67b7ffd8d973d50b8baf987eee4324c220d936db Mon Sep 17 00:00:00 2001
From: Jeff McCune 
Date: Wed, 25 Jul 2012 07:51:09 -0700
Subject: [PATCH 2/2] (#14149) Refactor utility code into PuppetX namespace

Without this patch the registry module is a bad example for contributors
and module authors because it does not follow the PuppetX convention
described at Puppet Design Guidelines [1]

This patch fixes the problem by moving all of the utility code and
mix-in modules from Puppet::Modules::Registry to
PuppetX::Puppetlabs::Registry namespace.

In addition, the loading code (require statements) have been modified to
assume #7788 (rubygems support) is implemented.  In the scenario where
7788 is not yet implemented we fall back to the work around of requiring
fully qualified paths to operate with pluginsync as described in #14073.

[1] http://projects.puppetlabs.com/projects/puppet/wiki/Puppet_Design_Guidelines
---
 lib/puppet/provider/registry_key/registry.rb      |   18 +++++++++++++-----
 lib/puppet/provider/registry_value/registry.rb    |   16 +++++++++++-----
 lib/puppet/type/registry_key.rb                   |   16 ++++++++++------
 lib/puppet/type/registry_value.rb                 |   14 +++++++++-----
 lib/puppet_x/puppetlabs/registry.rb               |   16 +++++++---------
 lib/puppet_x/puppetlabs/registry/provider_base.rb |   11 +++++++----
 6 files changed, 57 insertions(+), 34 deletions(-)

diff --git a/lib/puppet/provider/registry_key/registry.rb b/lib/puppet/provider/registry_key/registry.rb
index f1afd6f..d07516a 100644
--- a/lib/puppet/provider/registry_key/registry.rb
+++ b/lib/puppet/provider/registry_key/registry.rb
@@ -1,10 +1,18 @@
 # REMIND: need to support recursive delete of subkeys & values
-require 'pathname' # JJM WORK_AROUND #14073
-require Pathname.new(__FILE__).dirname.dirname.dirname.expand_path + 'modules/registry'
-require Pathname.new(__FILE__).dirname.dirname.dirname.expand_path + 'modules/registry/provider_base'
+begin
+  # We expect this to work once Puppet supports Rubygems in #7788
+  require "puppet_x/puppetlabs/registry"
+  require "puppet_x/puppetlabs/registry/provider_base"
+rescue LoadError => detail
+  # Work around #7788 (Rubygems support for modules)
+  require 'pathname' # JJM WORK_AROUND #14073
+  module_base = Pathname.new(__FILE__).dirname
+  require module_base + "../../../" + "puppet_x/puppetlabs/registry"
+  require module_base + "../../../" + "puppet_x/puppetlabs/registry/provider_base"
+end
 
 Puppet::Type.type(:registry_key).provide(:registry) do
-  include Puppet::Modules::Registry::ProviderBase
+  include PuppetX::Puppetlabs::Registry::ProviderBase
 
   defaultfor :operatingsystem => :windows
   confine    :operatingsystem => :windows
@@ -51,6 +59,6 @@ def values
   private
 
   def path
-    @path ||= Puppet::Modules::Registry::RegistryKeyPath.new(resource.parameter(:path).value)
+    @path ||= PuppetX::Puppetlabs::Registry::RegistryKeyPath.new(resource.parameter(:path).value)
   end
 end
diff --git a/lib/puppet/provider/registry_value/registry.rb b/lib/puppet/provider/registry_value/registry.rb
index abbfbfe..d0d8b33 100644
--- a/lib/puppet/provider/registry_value/registry.rb
+++ b/lib/puppet/provider/registry_value/registry.rb
@@ -1,10 +1,16 @@
 require 'puppet/type'
-require 'pathname' # JJM WORK_AROUND #14073
-require Pathname.new(__FILE__).dirname.dirname.dirname.expand_path + 'modules/registry'
-require Pathname.new(__FILE__).dirname.dirname.dirname.expand_path + 'modules/registry/provider_base'
+begin
+  require "puppet_x/puppetlabs/registry"
+  require "puppet_x/puppetlabs/registry/provider_base"
+rescue LoadError => detail
+  require "pathname" # JJM WORK_AROUND #14073 and #7788
+  module_base = Pathname.new(__FILE__).dirname + "../../../"
+  require module_base + "puppet_x/puppetlabs/registry"
+  require module_base + "puppet_x/puppetlabs/registry/provider_base"
+end
 
 Puppet::Type.type(:registry_value).provide(:registry) do
-  include Puppet::Modules::Registry::ProviderBase
+  include PuppetX::Puppetlabs::Registry::ProviderBase
 
   defaultfor :operatingsystem => :windows
   confine    :operatingsystem => :windows
@@ -160,6 +166,6 @@ def write_value
   end
 
   def path
-    @path ||= Puppet::Modules::Registry::RegistryValuePath.new(resource.parameter(:path).value)
+    @path ||= PuppetX::Puppetlabs::Registry::RegistryValuePath.new(resource.parameter(:path).value)
   end
 end
diff --git a/lib/puppet/type/registry_key.rb b/lib/puppet/type/registry_key.rb
index 0678e91..8ce7f9e 100644
--- a/lib/puppet/type/registry_key.rb
+++ b/lib/puppet/type/registry_key.rb
@@ -1,6 +1,10 @@
 require 'puppet/type'
-require 'pathname' # JJM WORK_AROUND #14073
-require Pathname.new(__FILE__).dirname.dirname.expand_path + 'modules/registry'
+begin
+  require "puppet_x/puppetlabs/registry"
+rescue LoadError => detail
+  require 'pathname' # JJM WORK_AROUND #14073 and #7788
+  require Pathname.new(__FILE__).dirname + "../../" + "puppet_x/puppetlabs/registry"
+end
 
 Puppet::Type.newtype(:registry_key) do
   @doc = <<-EOT
@@ -32,10 +36,10 @@ def self.title_patterns
       prefix.  For example: '32:HKLM\Software'"
 
     validate do |path|
-      Puppet::Modules::Registry::RegistryKeyPath.new(path).valid?
+      PuppetX::Puppetlabs::Registry::RegistryKeyPath.new(path).valid?
     end
     munge do |path|
-      reg_path = Puppet::Modules::Registry::RegistryKeyPath.new(path)
+      reg_path = PuppetX::Puppetlabs::Registry::RegistryKeyPath.new(path)
       # Windows is case insensitive and case preserving.  We deal with this by
       # aliasing resources to their downcase values.  This is inspired by the
       # munge block in the alias metaparameter.
@@ -83,7 +87,7 @@ def self.title_patterns
   # Autorequire the nearest ancestor registry_key found in the catalog.
   autorequire(:registry_key) do
     req = []
-    path = Puppet::Modules::Registry::RegistryKeyPath.new(value(:path))
+    path = PuppetX::Puppetlabs::Registry::RegistryKeyPath.new(value(:path))
     # It is important to match against the downcase value of the path because
     # other resources are expected to alias themselves to the downcase value so
     # that we respect the case insensitive and preserving nature of Windows.
@@ -99,7 +103,7 @@ def eval_generate
 
     # get the "should" names of registry values associated with this key
     should_values = catalog.relationship_graph.direct_dependents_of(self).select {|dep| dep.type == :registry_value }.map do |reg|
-      Puppet::Modules::Registry::RegistryValuePath.new(reg.parameter(:path).value).valuename
+      PuppetX::Puppetlabs::Registry::RegistryValuePath.new(reg.parameter(:path).value).valuename
     end
 
     # get the "is" names of registry values associated with this key
diff --git a/lib/puppet/type/registry_value.rb b/lib/puppet/type/registry_value.rb
index ba2090a..4510296 100644
--- a/lib/puppet/type/registry_value.rb
+++ b/lib/puppet/type/registry_value.rb
@@ -1,6 +1,10 @@
 require 'puppet/type'
-require 'pathname' # JJM WORK_AROUND #14073
-require Pathname.new(__FILE__).dirname.dirname.expand_path + 'modules/registry'
+begin
+  require "puppet_x/puppetlabs/registry"
+rescue
+  require 'pathname' # JJM WORK_AROUND #14073 and #7788
+  require Pathname.new(__FILE__).dirname + "../../" + "puppet_x/puppetlabs/registry"
+end
 
 Puppet::Type.newtype(:registry_value) do
   @doc = <<-EOT
@@ -28,10 +32,10 @@ def self.title_patterns
       '32:HKLM\Software\Value3'"
 
     validate do |path|
-      Puppet::Modules::Registry::RegistryValuePath.new(path).valid?
+      PuppetX::Puppetlabs::Registry::RegistryValuePath.new(path).valid?
     end
     munge do |path|
-      reg_path = Puppet::Modules::Registry::RegistryValuePath.new(path)
+      reg_path = PuppetX::Puppetlabs::Registry::RegistryValuePath.new(path)
       # Windows is case insensitive and case preserving.  We deal with this by
       # aliasing resources to their downcase values.  This is inspired by the
       # munge block in the alias metaparameter.
@@ -119,7 +123,7 @@ def change_to_s(currentvalue, newvalue)
     req = []
     # This is a value path and not a key path because it's based on the path of
     # the value resource.
-    path = Puppet::Modules::Registry::RegistryValuePath.new(value(:path))
+    path = PuppetX::Puppetlabs::Registry::RegistryValuePath.new(value(:path))
     # It is important to match against the downcase value of the path because
     # other resources are expected to alias themselves to the downcase value so
     # that we respect the case insensitive and preserving nature of Windows.
diff --git a/lib/puppet_x/puppetlabs/registry.rb b/lib/puppet_x/puppetlabs/registry.rb
index b6d0477..1b30d6b 100644
--- a/lib/puppet_x/puppetlabs/registry.rb
+++ b/lib/puppet_x/puppetlabs/registry.rb
@@ -1,10 +1,6 @@
-require 'pathname'
-# JJM WORK_AROUND
-# explicitly require files without relying on $LOAD_PATH until #14073 is fixed.
-# https://projects.puppetlabs.com/issues/14073 is fixed.
-require Pathname.new(__FILE__).dirname.expand_path
-
-module Puppet::Modules::Registry
+module PuppetX
+module Puppetlabs
+module Registry
   # For 64-bit OS, use 64-bit view. Ignored on 32-bit OS
   KEY_WOW64_64KEY = 0x100 unless defined? KEY_WOW64_64KEY
   # For 64-bit OS, use 32-bit view. Ignored on 32-bit OS
@@ -86,10 +82,10 @@ def filter_path
 
       case captures[1]
       when '32:'
-        result[:access] = Puppet::Modules::Registry::KEY_WOW64_32KEY
+        result[:access] = PuppetX::Puppetlabs::Registry::KEY_WOW64_32KEY
         result[:prefix] = '32:'
       else
-        result[:access] = Puppet::Modules::Registry::KEY_WOW64_64KEY
+        result[:access] = PuppetX::Puppetlabs::Registry::KEY_WOW64_64KEY
         result[:prefix] = ''
       end
 
@@ -161,3 +157,5 @@ def default?
     end
   end
 end
+end
+end
diff --git a/lib/puppet_x/puppetlabs/registry/provider_base.rb b/lib/puppet_x/puppetlabs/registry/provider_base.rb
index f0c0edb..533bc5f 100644
--- a/lib/puppet_x/puppetlabs/registry/provider_base.rb
+++ b/lib/puppet_x/puppetlabs/registry/provider_base.rb
@@ -1,8 +1,8 @@
-require 'pathname' # JJM WORK_AROUND #14073
-require Pathname.new(__FILE__).dirname.expand_path
-
 # This module is meant to be mixed into the registry_key AND registry_value providers.
-module Puppet::Modules::Registry::ProviderBase
+module PuppetX
+module Puppetlabs
+module Registry
+module ProviderBase
   # This is a class method in order to be easily mocked in the spec tests.
   def self.initialize_system_api
     if Puppet.features.microsoft_windows?
@@ -86,3 +86,6 @@ def name2type(name)
     name2type[name]
   end
 end
+end
+end
+end
-- 
1.7.10

#20 Updated by Andrew Parker almost 2 years ago

  • Target version deleted (2.7.x)

Also available in: Atom PDF