The deprecation function and Puppet 8 are not compatible

[joshcooper]

Describe the Bug

I wanted to discuss the issue of puppet's strict mode and the deprecation function in the puppetlabs-stdlib module. This was discussed previously in:

#1365
#1373
https://tickets.puppetlabs.com/browse/PUP-11868

As described in those issues, the problem is that all deprecations become hard errors when "strict=error", which is the default in puppet8. In practice, this means you can't really use "strict=error" and need to relax the setting to "strict=warning".

Expected Behavior

I would expect the deprecation function to behave similarly to puppet when its Puppet.deprecation_warning method is called. That is, the strict setting should not control whether the deprecation warning is displayed or not. Instead, it should be controlled by the disable_warnings=deprecations setting, similar to how -Wno-deprecated-declarations can silence warnings in GCC.

From my perspective, the problem is that both puppet and stdlib are trying to decide how to handle deprecation warnings and the two approaches are not compatible. To understand the disconnect, suppose you call Puppet.deprecation_warning('message', 'key'). The warning is always displayed when strict is set to error, warning and off, respectively:

$ bundle exec ruby -rpuppet -e " \
  Puppet.initialize_settings; \
  Puppet::Util::Log.newdestination(:console); \
  Puppet[:strict] = 'error'; \
  Puppet.deprecation_warning('message', 'key')"
Warning: message
   (location: -e:1:in `<main>')

$ bundle exec ruby -rpuppet -e " \
  Puppet.initialize_settings; \
  Puppet::Util::Log.newdestination(:console); \
  Puppet[:strict] = 'warning'; \
  Puppet.deprecation_warning('message', 'key')"
Warning: message
   (location: -e:1:in `<main>')

$ bundle exec ruby -rpuppet -e " \
  Puppet.initialize_settings; \
  Puppet::Util::Log.newdestination(:console); \
  Puppet[:strict] = 'off'; \
  Puppet.deprecation_warning('message', 'key')"
Warning: message
   (location: -e:1:in `<main>')

In order to silence the deprecation, you can set disable_warnings=deprecations and this works even when strict=error:

$ bundle exec ruby -rpuppet -e " \
  Puppet.initialize_settings; \
  Puppet::Util::Log.newdestination(:console); \
  Puppet[:strict] = 'error'; \
  Puppet[:disable_warnings] = 'deprecations'; \
  Puppet.deprecation_warning('message', 'key')"
$ 

Steps to Reproduce:

I would expect the following to print a deprecation warning instead of failing compilation:

$ bundle exec puppet apply --strict=error puppet apply -e "deprecation('key', 'message')" 
Error: Evaluation Error: Error while evaluating a Function Call, deprecation. key. message at ["unknown", 1]:["unknown", 0] (line: 1, column: 1) on node localhost

I would expect the deprecation warning to be silenced and for compilation to succeed:

$ bundle exec puppet apply --strict=error --disable_warnings=deprecations puppet apply -e "deprecation('key', 'message')"
Error: Evaluation Error: Error while evaluating a Function Call, deprecation. key. message at ["unknown", 1]:["unknown", 0] (line: 1, column: 1) on node localhost

Proposal

1. If the deprecation function is called with use_strict_setting=true and strict != :off, then only raise if disable_warnings doesn't include deprecations.

OR

2. A bigger change would be to remove this line:

    raise("deprecation. #{key}. #{message}") if use_strict_setting && Puppet.settings[:strict] == :error

and always just call Puppet.deprecation_warning. It's possible that folks could be relying on the raise behavior? Though I think it's doubtful because strict=error wasn't actually usable prior to puppet8, see https://github.com/puppetlabs/puppet/wiki/Puppet-8-Compatibility#strict-mode

[ekohl]

Or taking a step back: a warning is only useful if someone ends up acting on it, but with a deprecation there is no immediate need.

So which ways are there that a user could act on it?

CI is a good place. Probably the best if there is coverage, but we all know there isn't perfect coverage. So you need tooling (likely in rspec-puppet) to force deprecations to be fatal.

But what if you don't have coverage? How does this show up in reports? I come from a Foreman background so for me the way to see how my fleet is doing is by Puppetserver sending reports to Foreman which I then see. Not that I checked, but I don't think that's handled today.

Overall I do agree it should be configurable using the disable_warnings setting and rspec-puppet should have an easy way to deal with it.

[jstraw]

I agree with @ekohl about needing the ability to know a deprecation is happening and alert/error in testing but I would also say we should make how puppet and how stdlib handle it the same for catalog compilation. I'd go so far as to say that the stdlib function should just be a shorthand/easy way to call the Puppet namespaced function.

[joshcooper]

So you need tooling (likely in rspec-puppet) to force deprecations to be fatal.

Yes I completely agree.

I'd also add that "force deprecations to be fatal" should be opt-in and is a major blocker for puppet8 adoption, as currently puppetserver8 in its default configuration will fail compilation when the deprecation function is called.

Is there a non-hacky way for functions to know they are executing in the context of rspec-puppet now? If so, the deprecation function could be modified to only raise when running in that context and strict=error. If not, I suppose we could add an environment variable similar to what was done with STDLIB_LOG_DEPRECATIONS?

I'd go so far as to say that the stdlib function should just be a shorthand/easy way to call the Puppet namespaced function.

I agree that the deprecation function should (ideally) have been a wrapper around puppet's Puppet.deprecation_warning from the start. Puppet already supports silencing warnings, via disable_warnings=deprecations, but it's missing the ability to cause warnings to be fatal (analogous to to gcc's -Werror). We could add a Puppet setting for that, something like raise_warnings=deprecations. It should be multi-valued like disable_warnings. But it doesn't help the issue of trying to test modules against multiple versions of puppet, so I think we're back to fixing the deprecation function.

[ekohl]

So you need tooling (likely in rspec-puppet) to force deprecations to be fatal.

Yes I completely agree.

I'd also add that "force deprecations to be fatal" should be opt-in and is a major blocker for puppet8 adoption, as currently puppetserver8 in its default configuration will fail compilation when the deprecation function is called.

Is there a non-hacky way for functions to know they are executing in the context of rspec-puppet now? If so, the deprecation function could be modified to only raise when running in that context and strict=error. If not, I suppose we could add an environment variable similar to what was done with STDLIB_LOG_DEPRECATIONS?

As a user I think the most logical place is the compile matcher. Today you have:

it { is_expected.to compile }
# or
it { is_expected.to compile.with_all_deps }
# or
it { is_expected.to compile.and_raise_error(/the error/) }

Perhaps that could be modified to also check for deprecations?

You then have to choose whether it's opt-in or opt-out. So compile.without_deprecations or compile.with_deprecations(/some regex/) (to allow certain deprecations).

I'm leaning to the latter because it requires fewer changes to existing specs to also check for deprecations. You could also implement a setting for it.