.gitignore

@@ -1,2 +1,3 @@
 pkg/
 *.swp
+aliases.sh

CHANGELOG.md

@@ -1,9 +1,16 @@
-v2.0.2
--------
+v2.1.0
+------
+- Use `post_resource_eval` hook instead of monkey patching if possible.
+- Some parameter validation moved into the Type (so it is caught earlier and
+  gives a better error message).
+- The validate stage now checks for multiple resources managing the same environment
+  variable in an incompatible way (e.g. two resources in clobber mergemode) and raises
+  an error if such conflicts are found.
+
+### v2.0.2
 - Fix formatting for puppetforge's markdown interpreter (version bump needed to push to puppetforge)
 
-v2.0.1
-------
+### v2.0.1
 - Fix documentation (version bump needed to push to puppetforge)
 
 v2.0.0

README.md

@@ -14,12 +14,10 @@ Install from git (do this in your modulepath):
 
     git clone https://github.com/badgerious/puppet-windows-env windows_env
 
-It is important that the folder where this module resides is named windows_env, not puppet-windows-env.
-
 Changes
 -------
 
-Please see [CHANGELOG.md](https://github.com/badgerious/puppet-windows-env/blob/master/CHANGELOG.md)
+[CHANGELOG.md](https://github.com/badgerious/puppet-windows-env/blob/master/CHANGELOG.md)
 
 Usage
 -----
@@ -175,24 +173,6 @@ or refresh their environment by some other means.
 
 ```
 
-### Things that won't end well
-Certain conflicts can occur which may cause unexpected behavior (which you may not be warned about):
-
-- Multiple resource declarations controlling the same environment variable with
-  at least one in `mergemode => clobber`. Toes will be stepped on. 
-- Multiple resource declarations controlling the same environment variable with
-  different `type`s. More squished toes. 
-
-If you find yourself using `mergemode => clobber` or `type`, I recommend using
-the environment variable name as the resource title (like the example 'Title
-type #2' above) if you can; this way puppet will flag duplicates for you and
-help identify the conflicts. 
-
-
-Compatibility
--------------
-This module has been tested against a 3.2.x puppetmaster, 2.7.x and 3.2.x agents. 
-
 Acknowledgements
 ----------------
 The [puppet-windows-path](https://github.com/basti1302/puppet-windows-path) module by Bastian Krol was the starting point for this module. 

lib/puppet/provider/windows_env/windows_env.rb

@@ -3,6 +3,7 @@
 # non Windows nodes that have had this module pluginsynced to them. 
 if Puppet.features.microsoft_windows?
   require 'Win32API'  
+  require 'puppet/util/windows/error'
   require 'puppet/util/windows/security'
   require 'win32/registry' 
   require 'windows/error'
@@ -13,16 +14,23 @@ class Registry
   end
 end
 
-# This is apparently the "best" way to do unconditional cleanup for a provider.
-# see https://groups.google.com/forum/#!topic/puppet-dev/Iqs5jEGfu_0
-module Puppet
-  class Transaction
-    # added '_xhg62j' to make sure that if somebody else does this monkey patch, they don't
-    # choose the same name as I do, since that would cause ruby to blow up. 
-    alias_method :evaluate_original_xhg62j, :evaluate
-    def evaluate
-      evaluate_original_xhg62j
-      Puppet::Type::Windows_env::ProviderWindows_env.unload_user_hives
+if Puppet.version < '3.4.0'
+  # This is the best pre-3.4.0 way to do unconditional cleanup for a provider.
+  # see https://groups.google.com/forum/#!topic/puppet-dev/Iqs5jEGfu_0
+  module Puppet
+    class Transaction
+      # The alias name (evaluate_orig_windows_env) should be unique to make
+      # sure that if somebody else does this monkey patch, they don't choose
+      # the same name and cause ruby to blow up.
+      alias_method :evaluate_orig_windows_env, :evaluate
+      def evaluate
+        evaluate_orig_windows_env
+        begin
+          Puppet::Type::Windows_env::ProviderWindows_env.post_resource_eval
+        rescue => detail
+          Puppet.log_exception(detail, "post_resource_eval failed for provider windows_env")
+        end
+      end
     end
   end
 end
@@ -39,7 +47,6 @@ def evaluate
     self::SendMessageTimeout = Win32API.new('user32', 'SendMessageTimeout', 'LLLPLLP', 'L')
     self::RegLoadKey = Win32API.new('Advapi32', 'RegLoadKey', 'LPP', 'L')
     self::RegUnLoadKey = Win32API.new('Advapi32', 'RegUnLoadKey', 'LP', 'L')
-    self::FormatMessage = Win32API.new('kernel32', 'FormatMessage', 'LLLLPL', 'L')
   end
 
   # Instances can load hives with #load_user_hive . The class takes care of
@@ -49,7 +56,7 @@ class << self
     attr_reader :loaded_hives
   end
 
-  def self.unload_user_hives
+  def self.post_resource_eval
     Puppet::Util::Windows::Security.with_privilege(Puppet::Util::Windows::Security::SE_RESTORE_NAME) do
       @loaded_hives.each do |hash| 
         user_sid = hash[:user_sid]
@@ -61,14 +68,6 @@ def self.unload_user_hives
   end
 
   def exists?
-    if @resource[:ensure] == :present && [nil, :nil].include?(@resource[:value])
-      self.fail "'value' parameter must be provided when 'ensure => present'"
-    end
-    if @resource[:ensure] == :absent && [nil, :nil].include?(@resource[:value]) && 
-      [:prepend, :append, :insert].include?(@resource[:mergemode])
-      self.fail "'value' parameter must be provided when 'ensure => absent' and 'mergemode => #{@resource[:mergemode]}'"
-    end
-
     if @resource[:user]
       @reg_hive = Win32::Registry::HKEY_USERS
       @user_sid = Puppet::Util::Windows::Security.name_to_sid(@resource[:user])
@@ -94,10 +93,6 @@ def exists?
     @reg_types = { :REG_SZ => Win32::Registry::REG_SZ, :REG_EXPAND_SZ => Win32::Registry::REG_EXPAND_SZ }
     @reg_type = @reg_types[@resource[:type]]
 
-    if @resource[:value].class != Array
-      @resource[:value] = [@resource[:value]]
-    end
-
     begin
       # key.read returns '[type, data]' and must be used instead of [] because [] expands %variables%. 
       @reg_hive.open(@reg_path) { |key| @value = key.read(@resource[:variable])[1] } 
@@ -258,10 +253,7 @@ def load_user_hive
     Puppet::Util::Windows::Security.with_privilege(Puppet::Util::Windows::Security::SE_RESTORE_NAME) do
       result = self.class::RegLoadKey.call(Win32::Registry::HKEY_USERS.hkey, @user_sid, ntuser_path)
       unless result == 0
-        _FORMAT_MESSAGE_FROM_SYSTEM = 0x1000
-        message = ' ' * 512
-        self.class::FormatMessage.call(_FORMAT_MESSAGE_FROM_SYSTEM, 0, result, 0, message, message.length)
-        self.fail "Could not load registry hive for user '#{@resource[:user]}'. RegLoadKey returned #{result}: #{message.strip}"
+        raise Puppet::Util::Windows::Error.new("Could not load registry hive for user '#{@resource[:user]}'", result)
       end
     end
 

lib/puppet/type/windows_env.rb

@@ -1,10 +1,33 @@
 Puppet::Type.newtype(:windows_env) do
   desc "Manages Windows environment variables"
 
-  ensurable do
-    newvalue(:present) { provider.create }
-    newvalue(:absent) { provider.destroy }
-    defaultto(:present)
+  # Track resources that are managing the same environment variable so we can
+  # detect mergemode/type conflicts
+  @mergemode = {}
+  @type = {}
+  def self.check_collisions(resource)
+    user = resource[:user] || :SYSTEM
+    var = resource[:variable].downcase
+
+    # Cannot have two resources in clobber mode on the same var
+    @mergemode[user] ||= {}
+    last = @mergemode[user][var]
+    if (resource[:mergemode] == :clobber && last) || (last && last[:mergemode] == :clobber)
+      fail "Multiple resources are managing the same environment variable but at least one is in clobber mergemode. (Offending resources: #{resource}, #{last})"
+    else
+      @mergemode[user][var] = resource
+    end
+
+    # Cannot have two resources with different types on the same var
+    if ![nil, :undef].include?(resource[:type])
+      @type[user] ||= {}
+      last = @type[user][var]
+      if last && last[:type] != resource[:type]
+        fail "Multiple resources are managing the same environment variable but their types do not agree (Offending resources: #{resource}, #{last})"
+      else
+        @type[user][var] = resource
+      end
+    end
   end
 
   # title will look like "#{variable}=#{value}" (The '=' is not permitted in 
@@ -16,6 +39,12 @@ def self.title_patterns
      [/^([^=]+)$/   , [[:variable, proc{|x| x}]]]]
   end
 
+  ensurable do
+    newvalue(:present) { provider.create }
+    newvalue(:absent) { provider.destroy }
+    defaultto(:present)
+  end
+
   newparam(:variable) do
     desc "The environment variable name"
     isnamevar
@@ -24,6 +53,14 @@ def self.title_patterns
   newparam(:value) do
     desc "The environment variable value"
     isnamevar
+
+    munge do |val|
+      if val.class != Array
+        [val]
+      else
+        val
+      end
+    end
   end
 
   newparam(:user) do
@@ -60,4 +97,16 @@ def self.title_patterns
     desc "What type of registry key to use for the variable. Determines whether interpolation of '%' enclosed names will occur"
     newvalues(:REG_SZ, :REG_EXPAND_SZ)
   end
+
+  validate do
+    if self[:ensure] == :present && [nil, :undef].include?(self[:value])
+      fail "'value' parameter must be provided when 'ensure => present'"
+    end
+    if self[:ensure] == :absent && [nil, :undef].include?(self[:value]) &&
+      [:prepend, :append, :insert].include?(self[:mergemode])
+      fail "'value' parameter must be provided when 'ensure => absent' and 'mergemode => #{self[:mergemode]}'"
+    end
+
+    self.class.check_collisions(self)
+  end
 end

metadata.json

@@ -0,0 +1,19 @@
+{
+  "name": "badgerious-windows_env",
+  "version": "2.1.0",
+  "author": "badgerious",
+  "summary": "Manages Windows environment variables",
+  "license": "Apache License, Version 2.0",
+  "source": "git://github.com/badgerious/puppet-windows-env",
+  "project_page": "https://github.com/badgerious/puppet-windows-env",
+  "issues_url": "https://github.com/badgerious/puppet-windows-env/issues",
+  "description": "Create, delete, and insert values into Windows environment variables",
+  "dependencies": [
+
+  ],
+  "operatingsystem_support": [
+  {
+    "operatingsystem": "Windows"
+  }
+  ]
+}