would have been nice to know earlier when Puppet renamed master → main, but at least now I caught the last few references I had

hub uses the HEAD reference for the correct branch when using hub pull-request (to create a PR)

ewoud: I didn't pay enough attention to the PR at the time. I read your "because for end users it doesn't change anything." and assumed it was just an issue confined to rspec-puppet. Annoying, but something you were quickly fixing up.

afisher: well, fair enough - I didn't forsee too many issues

overall I wasn't quite sure of the benefit

@josh As per @bastelfreak's suggestion, is there anything that can be done in core puppet such that 'installed' and 'present' are more 'fully' aliased? (I note the code currently has

aliasvalue(:installed, :present)

but this isn't enough to stop a duplicate resource declaration).

perhaps defined_with_params needs to be smarter and understand aliases

maybe the fix can be made in ensure_packages itself. Look at what's already in the catalog??

https://github.com/puppetlabs/puppetlabs-stdlib/blob/main/lib/puppet/parser/functions/defined_with_params.rb that determines if it's already in the catalog

can I get a review for https://github.com/voxpupuli/puppet-augeasproviders_apache/pull/13

@Alex Fisher I have stdlib pinned to 7.1.0 because of that

I don't think that

ensure_packages

should care about the value of the

ensure

parameter so long as it isn't

absent

. We probably need a replacement that introspects on the catalog.

For starters, let's convert to the new API and remove some of the code duplication.

ie https://github.com/puppetlabs/puppetlabs-stdlib/pull/1244 🙂

ie https://github.com/puppetlabs/puppetlabs-stdlib/pull/1244 🙂

There are fewer lines of code now that hopefully makes it easier to discuss ideas for adding the new functionality.

Is it possible to get the catalog object from a function?

yeah, I think there are quite a few examples around.

Re the PR. I think if the default_attributes param was removed for now it would be a slam dunk to be merged.

I currently don't like the 2 code paths for arg 1 being a Hash or Array of packages and calling either

ensure_resource

in a loop or

ensure_resources

The second parameter didn't have a name in the old API, but it did exist.

IMO, it's not very useful as the moment you add extra parameters the whole point of the function becomes moot as you'll likely still get duplicate resources.

I've still seen it used quite a lot by people who don't know better or just cargo cult some example into code they write. ie

ensure_packages('some_package', { 'ensure' => 'present' })

seems to crop up a lot in the code I get to deal with in my day job.

omg, there is an arguments[1]

I guess I've never even read the docstring

yep. The only tests I've removed are the ones that have been made redundant by adding the data types. eg. empty package names.

If we convert the array (or single string) case to a the hash case, we then iterate over each package to see if there's a value for ensure already in the catalog that we need to use (instead of arguing over

present

vs

installed

). Since we're already iterating, we do all the merging of

default_attributes

at this point instead of passing them to

ensure_resources

. Probably best to ignore any other parameters we could at this point find from an existing catalog resource, and just let the original duplicate declaration error hit if there were any that didn't match.

I would suggest doing just the 3.x -> 4.x conversion in a single PR

and then build on top of that

yep, that's the plan. It's the end of my working week now anyway, so it's also a good cut off point for me.