(MODULES-3529) add deprecation function

[tphoney]

No description provided.

[DavidS]

Doesn't tell reader how strict affects the function.

[hlindberg]

This is not good - no @@vars please - they hold on to the data forever.

[hlindberg]

There is yet another possibility - there is a migration support class that can be used to report issues during runtime in a more structured way. That API is then used by the catalog preview tool to collect and present issues. That makes it a lot easier to do migration of old code. In 3.8, that API has a fairly rich set of methods. They are mostly removed in 4.x. It is possible to introduce new methods akin to the 3.8.x -> 4.x migration checks. The result of using the migration checking issues makes it possible to offer a better user experience than just "stuff logged with messages".

[tphoney]

@hlindberg @DavidS I think that your last recommendation about using catalog_preview may be outside of our requirements.

I have a question: if Puppet.settings[:strict] = :error, what logging / error method would you recommend i use. http://www.rubydoc.info/gems/puppet/4.4.1/Puppet/Util/Logging
We still only want to display the message once

[tphoney]

@hlindberg @DavidS i have made the suggested changes, what do you think of the README, i pulled in your comments @hlindberg .

[hlindberg]

there is an :err (or is it :error - I mix them up all the time) log level. This level does not cause puppet to fail. The spirit of {{--strict=error}} is that you want the evaluation to actually stop as you would use that level to be pedantic and enforce that all deprecations have been dealt with. I suggest failing in that case rather than just logging an error.

[hlindberg]

adding to disabled_warnings requires an update to puppet as it lists all keys that can be disabled - so that part will not work without a change to that setting in puppet.

[DavidS]

That is very unfortunate, as we hoped to expose this functionality to the user. Specifically, allowing them(us) to specify finegrained deprecations, at least at the module level.

[hlindberg]

Actually what I said is misleading, the first parameter kind to warn once, is the key to disabled_warnings. So, kind can be set to 'deprecation' which means it can be disabled. Here is an example from puppet source: Puppet.warn_once('deprecation', 'symbol_comparison', 'Comparing Symbols to non-Symbol values is deprecated'). If a new kind is wanted and that it is also wanted to be able to disable that kind then puppet must be updated. (It would not be unreasonable to support any kind in the disable_warnings (with the only downside that typos will not be caught)).

[DavidS]

Given the vague descriptions of the core infrastructure we're using here, a few integration tests exercising the whole stack would be great.

Also, please put the ticket number in the Commit and PR title for automatic linking to JIRA.

[tphoney]

@hlindberg @DavidS changes made.

[tphoney]

@DavidS i didnt know you were pointing that comment at me. What you mean by full stack testing ?

[DavidS]

I was thinking about something like this: https://github.com/tphoney/puppetlabs-stdlib/compare/add_deprecate_function...DavidS:MODULES-3529-acceptance-tests?expand=1

[tphoney]

@DavidS i changed the check for puppet4 in the acceptance test to

if: get_puppet_version =~ /^4/ do
following other acceptance tests.

[DavidS]

@tphoney please apply shellquoting and then fix the fialing tests:

diff --git a/spec/acceptance/deprecation_spec.rb b/spec/acceptance/deprecation_spec.rb
index 54cdfdb..4215e6c 100644
--- a/spec/acceptance/deprecation_spec.rb
+++ b/spec/acceptance/deprecation_spec.rb
@@ -1,5 +1,6 @@
 #! /usr/bin/env ruby -S rspec
 require 'spec_helper_acceptance'
+require 'shellwords'

 describe 'deprecation function' do
   before :each do
@@ -12,7 +13,7 @@ describe 'deprecation function' do
       deprecation('key', 'message')
       file { '/tmp/deprecation': ensure => present }
       EOS
-      @result = on(default, puppet('apply', '--strict=error', '-e', pp), acceptable_exit_codes: (0...256))
+      @result = on(default, puppet('apply', '--strict=error', '-e', Shellwords.shellescape(pp)), acceptable_exit_codes: (0...256))
     end

     it "should return an error" do
@@ -34,7 +35,7 @@ describe 'deprecation function' do
       deprecation('key', 'message')
       file { '/tmp/deprecation': ensure => present }
       EOS
-      @result = on(default, puppet('apply', '--strict=warning', '-e', pp), acceptable_exit_codes: (0...256))
+      @result = on(default, puppet('apply', '--strict=warning', '-e', Shellwords.shellescape(pp)), acceptable_exit_codes: (0...256))
     end

     it "should not return an error" do
@@ -56,7 +57,7 @@ describe 'deprecation function' do
       deprecation('key', 'message')
       file { '/tmp/deprecation': ensure => present }
       EOS
-      @result = on(default, puppet('apply', '--strict=off', '-e', pp), acceptable_exit_codes: (0...256))
+      @result = on(default, puppet('apply', '--strict=off', '-e', Shellwords.shellescape(pp)), acceptable_exit_codes: (0...256))
     end

     it "should not return an error" do

[tphoney]

@DavidS boom !

[DavidS]

retriggered jenkins to pickup the new beaker version

[tphoney]

@DavidS tests are now passing

[DavidS]

Wohoo!! 🎉