(FM-7547) move function definitions out of Puppet function

[binford2k]

In Puppet 5.5.7, legacy functions that had ruby functions defined in the
file no longer work. This puts it into a library class.

[binford2k]

@hlindberg Is this the recommended solution?

[hlindberg]

Best is to implement this using the 4x function API and have the extra methods inside the function body.
By placing the logic outside in Puppetx you make that code subject to environment leakage (must have same version of this module in all environments).

[hlindberg]

Change to using the 4x function API - you can then keep the extra methods inside the function definition.

[joshcooper]

Should the FM ticket be moved to MODULES (and this PR updated) since only MODULES is public?

[binford2k]

kk. Porting to the new API is a little more work than I've got time for today. Someone else should pick this up and run with it then :)

[hlindberg]

It is really easy to convert to 4.x - Here is the code (without using any refactoring other than making it be implemented with the 4x API). Simply put this in lib/puppet/functions:

# @summary Recursively merges two or more hashes together and returns the resulting hash.
#
# @example
#   $hash1 = {'one' => 1, 'two' => 2, 'three' => { 'four' => 4 } }
#    $hash2 = {'two' => 'dos', 'three' => { 'five' => 5 } }
#    $merged_hash = mysql_deepmerge($hash1, $hash2)
#    # The resulting hash is equivalent to:
#    # $merged_hash = { 'one' => 1, 'two' => 'dos', 'three' => { 'four' => 4, 'five' => 5 } }
#
# - When there is a duplicate key that is a hash, they are recursively merged.
# - When there is a duplicate key that is not a hash, the key in the rightmost hash will "win."
# - When there are conficting uses of dashes and underscores in two keys (which mysql would otherwise equate),
#   the rightmost style will win.
#
# @return [Hash]
#
Puppet::Functions.create_function(:mysql_deepmerge) do
  dispatch :mysql_deepmerge do
    repeated_param 'Any', :args
  end

  def mysql_deepmerge(args)
    if args.length < 2
      raise Puppet::ParseError, _('mysql_deepmerge(): wrong number of arguments (%{args_length}; must be at least 2)') % { args_length: args.length }
    end

    result = {}
    args.each do |arg|
      next if arg.is_a?(String) && arg.empty? # empty string is synonym for puppet's undef
      # If the argument was not a hash, skip it.
      unless arg.is_a?(Hash)
        raise Puppet::ParseError, _('mysql_deepmerge: unexpected argument type %{arg_class}, only expects hash arguments.') % { args_class: args.class }
      end

      # Now we have to traverse our hash assigning our non-hash values
      # to the matching keys in our result while following our hash values
      # and repeating the process.
      overlay(result, arg)
    end
    return(result)
  end

  def normalized?(hash, key)
    return true if hash.key?(key)
    return false unless key =~ %r{-|_}
    other_key = key.include?('-') ? key.tr('-', '_') : key.tr('_', '-')
    return false unless hash.key?(other_key)
    hash[key] = hash.delete(other_key)
    true
  end

  def overlay(hash1, hash2)
    hash2.each do |key, value|
      if normalized?(hash1, key) && value.is_a?(Hash) && hash1[key].is_a?(Hash)
        overlay(hash1[key], value)
      else
        hash1[key] = value
      end
    end
  end
end

As you can see - any 3.x function is trivially changed to 4.x by making it have a dispatcher accepting a variable number of Any arguments, then wrapping the function's code in a def - any extra methods simply come after that. You also take out the documentation and place that in a comment before the call.

Naturally more refactoring is possible since checking of the input parameters can be delegated to the dispatcher - but I did not spend time figuring out what this function actually accepts.

You may need adjustments to the testing of the function - I did not look at that...

[binford2k]

@hlindberg actually, I just realized that all that work has been done. https://github.com/puppetlabs/puppetlabs-mysql/blob/master/lib/puppet/functions/mysql/deepmerge.rb

These legacy functions are to keep from breaking existing code.

[binford2k]

@HelenCampbell Is this a good idea?

[HelenCampbell]

Looks good @binford2k ! I'd suggest looking in to using the deprecation function in stdlib to see if it'd makes sense to use it for this as it can be turned off by users (I'll link it below), but otherwise I'm happy with this change 👍

https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/lib/puppet/functions/deprecation.rb

[binford2k]

@HelenCampbell I think this has to be a hard failure, because if the loader loads the file, then it crashes before the function is executed.

Also, I'm not sure what to do about the test that's failing (appropriately). As soon as the spec loads, it will load the file and raise the exception. @hlindberg, do you have suggestions here? (besides burn it with 🔥)

[hlindberg]

@binford2k the 4x version of the function has a different name and requires users to shift to using it. Why not let it have the same name as the 3.x version (i.e. not namespaced). That way the 3.x version of the function will never be loaded. If you want to deprecate the non namespaced, then add a 4.x function that performs the deprecation - it can naturally also call the namespaced version. After that, the only reason to keep the 3.x version of the function is to support Puppet < 4.0.0

[hlindberg]

Maybe my earlier comment was not that clear - what I am suggesting is:

• Add a new 4.x function with the same name as the 3.x function - i.e. the not namespaced mysql_deepmerge
• Have that function accept any number of Any parameters
• Have one method in that function that takes all the parameters, and issues a deprecation warning
• The method then uses call_function to call the namespaced version mysql::deepmerge giving it the arguments (variable number of Any) - thus letting the real/new namespaced function perform the validation of the arguments.
• If you no longer support Puppet < 4.0.0 you can remove the 3.x implementation

Then, if there are tests that rely on mysql_deepmerge being a 3.x function, then those have to be changed to not care how the function is implemented. The same tests should then work on both older puppet versions and newer.

[david22swan]

@binford2k I am closing this PR as the function that it references, 'deep_merge', has since been removed from the code in #1145 as it is a copy of a function that exist's within puppet itself.
Of the remaining three functions, 'dirname' has also been removed as it exists within the stdlib while a pr (#1151) has been put up to create wrapper functions for the remaining two, to allow people to continue to use the puppet 3.x function call names without breaking their code, while at the same time giving them a deprecation message to alert them to the change in naming.
Thank you for creating this PR and helping to bring this problem to our attention and I hope the solution that we have implemented in agreeable to you.
Best Wishes