namespacing ensure_packages breaks a lot of modules

[robertc99]

The recent release of stdlib 9 namespaced a lot of functions.
However, namespacing ensure_packages breaks a lot of existing puppet forge modules.

Other namespaced functions like merge and the dropped has_keys have good alternatives.
I don't think ensure_packages does. So I don't think its a good idea to namespace it if the non namespaced version generates
an error.

So I think it would be better to namespace it, but leave the non namespaced version alone or at worst make it a warning.
Possibly the same reasoning applies to ensure_resources. But that doesn't seem to be as much of a problem.

[smoeding]

In my Puppet-8 environment the upgrade to Stdlib-9 caused Puppet Evaluation Errors terminating every Puppet run.

According to https://github.com/puppetlabs/puppet/wiki/Puppet-8-Compatibility the parameter strict now defaults to error (unfortunately the official docs at https://www.puppet.com/docs/puppet/8/configuration.html#strict still shows warning as its default).

If strict=error then the function deprecation (see lib/puppet/functions/deprecation.rb) will raise an exception if any of the deprecated functions are used. So every Puppet run will fail.

So how should the transition from the non-namespaced to the namespaced implementation of ensure_packages work? Stdlib-8 only has the non-namespaced function and Stdlib-9 fails if the non-namespaced function is called (using Puppet-8).

Running Puppet-8 with Stdlib-9 is only possible if all deprecated functions have been removed from all included modules, which probably will take some time. Setting strict=warning on the Puppetserver seems to be the only current workaround. This isn't obvious unless you dig into the code.

[robertc99]

ok, I didnt realise that it was puppet 8's fault. That timing sucks. The fact that puppet has gone warnings=error by default at the exact same time that its been decided to namespace the stdlib functions is very unfortunate.

The only workaround I can see and I don't know if its practical would be as follows.
Create your own "deprecation_notify" function that works like deprecation but calls notify instead of warning.

[smoeding]

ok, I didnt realise that it was puppet 8's fault. That timing sucks. The fact that puppet has gone warnings=error by default at the exact same time that its been decided to namespace the stdlib functions is very unfortunate.

Exactly my opinion.

The only workaround I can see and I don't know if its practical would be as follows. Create your own "deprecation_notify" function that works like deprecation but calls notify instead of warning.

I reverted the changed default in my puppet.conf on the Puppetserver:

[main]
strict = warning

The error is gone with that setting.

[robertc99]

Yeah, theres something not right.
I tried that setting, it now produces a warning rather than an error. But the module still isnt working correctly.
I dont understand whats going wrong though. Its getting a dependency cycle which is just weird.

[robertc99]

definitely ensure_packages and stdlib::ensure_packages do not work the same (except for producing a warning).
I dont know what ensure_packages is doing. But its not the right thing...

[alexjfisher]

Interesting...

maybe it's because it's an Internal function and is now being invoked with a different scope???

[alexjfisher]

@robertc99 Are you able to test #1366 and verify that it does indeed fix your issue?

[alexjfisher]

FWIW, it wouldn't have done, because it had a bug in it... Should be good to go now though.

[danielparks]

Perhaps I should file a separate issue, but I believe the same basic problem causes problems for other functions as well, and it’s going to cause problems for a lot of modules.

For example, I have a puppet module that uses shell_escape() (presumably all similar functions have the same issue). At this point I have two options:

• Limit my dependency to puppetlabs/stdlib ">= 6.0.0 < 9.0.0" and use the old shell_escape() syntax. This would maintain support with Puppet 6 through 8, but breaks for anybody wanting to update stdlib on Puppet 8 who doesn’t know about this problem.
• Update my metadata to only support Puppet ≥ 7 and puppetlabs/stdlib ≥ 9 and use the new stdlib::shell_escape() syntax. This will cause problems for anybody who needs to use an old version of puppetlabs/stdlib, which is particularly likely because of this issue.

Strictly speaking I could also roll my own shell_escape() function, or I could write a wrapper that checks for the existence of stdlib::shell_escape and calls the correct function based on that. Those are likely not good options for other people though.

To be clear, I’m only mentioning my module as an example — I can work around this. This same basic issue will likely cause problems for a lot of people as they upgrade to stdlib 9.0.0.

[danielparks]

Also, I personally think that the “real” problem is the change in behavior in Puppet 8 — deprecation warnings should not be treated as errors by default. Of course, that is likely outside of any of our power to change.

I just figured that I should clarify that I don’t think this was a bad change in stdlib. Rather, it was the right change, but Puppet 8 broke things.

[danielparks]

Sorry, last comment today. Should have gotten all my thoughts together before I started writing. I filed two tickets with Puppet:

• PUP-11868: New default strict=error breaks all deprecation warnings
• PUP-11867: Documentation: default is now strict=error

Perhaps I’m wrong about all of this and #1366 will fix this, though?

[alexjfisher]

No, #1366 only fixes the change in behaviour when strict mode is off.

You're right that we need to see how to improve the issue with deprecation warnings being more than just warnings.
Maybe in 9.x we should have only done a 'soft' deprecation and not called the deprecate method until 10.x?

Or can we add a setting specific to deprecations instead of using the existing strict setting?? I'm not sure this would be possible for a module to add a setting to core Puppet though. (I actually doubt it). @joshcooper ?

cc @binford2k @bastelfreak @ekohl

[danielparks]

I was a little confused, so to clarify, it sounds like there are two issues:

1. Puppet 8 changes the default to strict=error, which means that non-namespaced functions with deprecation warnings now cause Puppet runs to fail by default. This is particularly a problem with ensure_packages() because it is used by so many modules.
2. ensure_packages() in particular doesn’t work the same way as stdlib::ensure_packages() when strict=warning (I’m not clear on how it’s different). This is what’s fixed by Pass calling scope to stdlib::ensure_packages from shim #1366.

Should there be a separate issue for one or both of these?

[alexjfisher]

I've opened #1373 and #1374 to replace this issue with the 2 specific issues being discussed.