Convert ensure_packages to new API and refactor
#1244
Conversation
alexjfisher
force-pushed
the
ensure_packages
branch
from
8be04a2
to
cae344b
Compare
alexjfisher
force-pushed
the
ensure_packages
branch
from
cae344b
to
2dcbfbc
Compare
| @@ -0,0 +1,38 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| # @summary Takes a list of packages and only installs them if they don't already exist. | |||
There was a problem hiding this comment.
Technically you can also ensure they're absent via default_attributes, right?
There was a problem hiding this comment.
yes, or by specifying a hash for the first parameter. But... I really don't know why people do that. IMO it becomes a fairly useless function the moment you start specifying attributes as they likely won't match with any other definition you already have in the catalog for that package.
There was a problem hiding this comment.
Perhaps manages instead of installs is better? Also, due to the behavior of ensure_resource(s) the state must match. So perhaps ensures is even better.
| # @summary Takes a list of packages and only installs them if they don't already exist. | |
| # @summary Takes a list of packages and ensures they exist |
alexjfisher
force-pushed
the
ensure_packages
branch
from
2dcbfbc
to
82b74e2
Compare
|
I suspect this function needs to be an |
|
Containment is an interesting point. I think today it only does something if the resource was not previously defined. Otherwise it may be contained (I don't know). So it's already ambiguous where the package resource ends up. I've always written my code by assuming it was not contained added explicit You could add in containment, but I wonder if that could cause risks with dependency cycles. I'm thinking about this because the package may not be contained within two classes. |
|
Good call. I think it was contained, by the calling class and now it isn't. |
|
See https://gist.github.com/alexjfisher/b0287dc84a5ce451093d43f2de16f4ed I'll look into making this an Internal function to preserve the existing behaviour. Not sure how to write a test case for this. @hlindberg any pointers? Do I just do |
|
@alexjfisher sorry, don't know how to write that test. |
No luck so far. The meta |
|
I also think the function to be called (with the scope passed) needs to be called a special way - IIRC, there should be something like |
|
Not just I'll need to check on Monday morning, but I think that's what my uncommitted change I was working on looks like. Think it works. Need to double check. |
To preserve original behaviour of the created package(s) being contained within the class that called `ensure_packages`, the function needs to be an `InternalFunction` and call `ensure_resource(s)` with the correct scope.
|
@hlindberg Does b565f7f look 'correct' to you? In my testing with the |
|
I'm going to vote pretty hard that we do not try to maintain containment artifacts. At best, the existing behavior is unspecified and depending on its implementation details already causes surprising failures. Given the link that @alexjfisher posted earlier, the graph will already be different if someone includes the Imho, that's the whole point of this function. It explicitly says "I don't care how this package gets declared, just make sure it's in the catalog somewhere." |
|
I do agree with @binford2k, with the note that changing it in a minor/patch version might be surprising. Breaking implicit behavior still is painful for users. |
|
Thanks for the feedback. To summarise...
Once merged, I can then get on with coming up with a fix for making it work no matter if any existing package resource has |
|
I'd be happy with merging it as is, releasing it in a minor version and then doing the breaking change later. Major version bumps in stdlib are painful so I'd rather combine a few things. For example, perhaps we should deprecate the compat data types and drop them in the next major version (looking at you https://github.com/puppetlabs/puppetlabs-stdlib/tree/main/types/compat). |
|
@ekohl are you still good for this change to be merged? |
|
@ekohl Great thanks! |
As a first step before fixing #1196 in a separate PR, this PR converts to the new API and gets rid of some code duplication etc. in the process.