Pass calling scope to stdlib::ensure_packages from shim
#1366
Conversation
alexjfisher
requested review from
a team,
b4ldr,
bastelfreak,
ekohl and
smortex
as code owners
|
@bastelfreak Hold off on this. I should make sure it actually works first! (which it doesn't!) |
| @@ -1,14 +1,13 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| # THIS FILE WAS GENERATED BY `rake regenerate_unamespaced_shims` | |||
|
|
|||
| # @summary DEPRECATED. Use the namespaced function [`stdlib::ensure_packages`](#stdlibensure_packages) instead. | |||
There was a problem hiding this comment.
Hum, maybe also change this line because it is what triggers the rake task that generated this code:
https://github.com/puppetlabs/puppetlabs-stdlib/blob/main/Rakefile#L95
| # @summary DEPRECATED. Use the namespaced function [`stdlib::ensure_packages`](#stdlibensure_packages) instead. | |
| # @summary DEPRECATED. Use the scoped and namespaced function [`stdlib::ensure_packages`](#stdlibensure_packages) instead. |
Very hackish :-/
Otherwise we can search for Puppet::Functions\.create_function\(.*, Puppet::Functions::InternalFunction\) and generate different code if we find it. Also a bit hackish.
There was a problem hiding this comment.
It's more an internal detail. Not sure we really need to spell it out to users. Just not break things for them! :)
Are we likely to be running the rake task anymore?
There was a problem hiding this comment.
@smortex Now that I've updated the rake task to ignore this file, I've just got what you were getting at!
This comment was marked as outdated.
This comment was marked as outdated.
|
Ah, I misunderstood (based on this comment from alexjfisher). The only important part of my (now hidden) comment is that this doesn’t actually fix the main issue in #1365. The commit message (or PR description? not sure if they’re both important) should probably be updated so it doesn’t close the issue. |
|
#1365 really should be two issues. The only bit that is |
|
I can't believe theres been a new release of stdlib and this didnt get included.. Its a serious bug. |
cc @chelnak |
Make the `ensure_packages` shim an Internal function and pass scope to the namespaced version so as to not change the behaviour of where packages are contained. When the function was first ported to the new API, it was discussed that the existing behaviour might not be 'correct', but changing it would be a breaking change that might have consequences for many users. In namespacing the function in 9.0.0 we accidentally created a situation where the namespaced version worked as before, but the non-namespaced version, (the shim), now behaved differently. Fixes puppetlabs#1374
|
@alexjfisher Great catch, This looks good to me! |
|
@jordanbreen28 Should be good now I think. |
Make the
ensure_packagesshim an Internal function and pass scope to the namespaced version so as to not change the behaviour of where packages are contained.When the function was first ported to the new API, it was discussed that the existing behaviour might not be 'correct', but changing it would be a breaking change that might have consequences for many users.
In namespacing the function in 9.0.0 we accidentally created a situation where the namespaced version worked as before, but the non-namespaced version, (the shim), now behaved differently.
Fixes #1374