Make ensure_packages work with ensure => present
#1300
Conversation
|
I hate to bump stuff, but just got hit by this. And is somewhat starting to break stuff. 😭 Are we any closing in getting this merged and perhaps release a new patch version that includes it? Again, sorry for the bump. 🥺 |
|
@Sharpie do you have thoughts on this? |
| private | ||
|
|
||
| def default_ensure(package_name) | ||
| if call_function('defined_with_params', "Package[#{package_name}]", { 'ensure' => 'present' }) |
There was a problem hiding this comment.
The root cause of many problems is that defined_with_params doesn't understand this bit: https://github.com/puppetlabs/puppet/blob/523d881ecdee777d7bec46cea5b26fd6621f558c/lib/puppet/type/package.rb#L113-L114
I was thinking about this too and wondering about teaching defined_with_params about aliasvalue. Though I really wonder how you'd implement that, given a lot of those mechanics are private APIs.
There was a problem hiding this comment.
yeah, I did try and figure out a more generic solution, but also figured the less I touch, the less chance of subtly breaking anything!
This is effectively my 2nd PR on the way to fixing this issue. The first one was a while back and turned the function into a modern API version.
Whilst I'm not super happy with the hackiness of this solution, as a whole with the refactoring in this PR combined with the work done when moving from the old API I think the final result is still a lot nicer than what we originally had.
The breaking change in 8.0.0 has been a blocker for several people in upgrading stdlib, so hopefully this is a good enough fix and can be merged.
There was a problem hiding this comment.
I also ran into it myself, so I think this would be good start.
There was a problem hiding this comment.
I drilled into this for a bit and got closer, but not completely there. I got stuck trying to figure out how to get a list of known aliases and couldn't get there in this context. What I'd like to do is update this check,
to do something smarter, like res[key].aliases.include? value instead of the straight equality check it does now.
In other contexts, I can get a list of aliases using Puppet::Parameter::Value.aliases but I can't quite get there from here yet.
There was a problem hiding this comment.
@binford2k that's exactly what my line of thought was, except I gave up on figuring out the .aliases bit. As I said: those parts look like private APIs.
|
|
alexjfisher
force-pushed
the
ensure_packages_improvements
branch
from
cd6ec5f to
a30e8b1
Compare
|
Rebased. Build failure doesn't look related. |
|
Test failures are because of the changes to |
This unbreaks the breaking change made in puppetlabs#1196 Also refactored to create a separate dispatch method for the case when `packages` is a `Hash`, and having that call the main `ensure_packages` method. This simplifies the code by only ever calling `ensure_resource` instead of calling `ensure_resources` for hashes. Defaulting `default_attributes` to an empty hash instead of `nil` in the method signatures further simplifies the code. Fixes puppetlabs#1252
alexjfisher
force-pushed
the
ensure_packages_improvements
branch
from
a30e8b1 to
837f988
Compare
This unbreaks the breaking change made in #1196
Also refactored to create a separate dispatch method for the case when
packagesis aHash, and having that call the mainensure_packagesmethod. This simplifies the code by only ever callingensure_resourceinstead of callingensure_resourcesfor hashes.Defaulting
default_attributesto an empty hash instead ofnilin the method signatures further simplifies the code.Fixes #1252