Add function ensure_resource and defined_with_params #86
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
it looks like these tests won't pass until this pull request gets merged into rspec puppet: |
|
the test dependencies are now merged into rspec-puppet and available in version: 0.1.4 |
|
I have a number of concerns that need to be addressed before this gets merged. I've put some effort into addressing my own concerns with discrete commits. Please bring in the following topic branch and review my commits for concrete examples: https://github.com/jeffmccune/puppetlabs-stdlib/tree/feature/2.4.x/ensure_resource_function Here they are: Housekeeping related:
Conceptual: Reading the code, it appears that the Implementation: If the user calls this function with a variable set to |
ghost
assigned
|
Also, I'm not sure the spec tests are working. I have rspec-puppet 0.1.4 and I'm getting this: Here's my integration matrix: |
|
Finally, the tests added in this pull request appear to have behavior that depends on the current working directory of the process. While I got 5 of 6 failures above, I get 6 of 6 failures with undefined |
added 4 commits
August 13, 2012 17:09
This commit adds 2 new functions with unit tests.
defined_with_params works similarily to puppet's defined
function, except it allows you to also specify a hash of
params. defined_with_params will return true if a resource
also exists that matches the specified type/title (just like
with defined) as well as all of the specified params.
ensure_resource is a function that basically combines defined_with_params
with create_resources to conditionally create resources only if the
specified resource (title, type, params) does not already exist.
These functions are created to serve as an alternative to using
defined as follows:
if ! defined(Package['some_package']) {
package { 'some_package': ensure => present,
}
The issue with this usage is that there is no guarentee about
what parameters were set in the previous definition of the package
that made its way into the catalog.
ensure_resource could be used instead, as:
ensure_resource('package', 'some_package', { 'ensure' => 'present' })
This will creat the package resources only if another resource does
not exist with the specified parameters.
This commit adds better handling of the case where undef is passed as the parameter value. This works by converting '' into []
This commit adds better inline documentation explaining how replicate resource definitions can occur if the resource exists and does not have matching parameters.
This commit refactors to ensure 80 character lines.
|
the following commits resolve most of the above mentioned problems. The first commit has been modified to include the previous changes that existed in your branch. The additional commits in this pull request handle other points brought up in this discussion.
|
|
This still seems to be breaking, even when I'm using the pull request you mentioned on rspec-puppet. Here's what I'm getting: Could we pair on this quickly? I think I can help get this merged in today if I'm able to drive for a short while. -Jeff |
|
This is after modifying the Rakefile to also run tests against the functions sub directory: |
jeffmccune
pushed a commit
that referenced
this pull request
Aug 15, 2012
Add function ensure_resource and defined_with_params
|
OK, I went ahead and merged this in, but it's failing now against Puppet 2.6 so I'm going to revert this. Please try and fix up the error logged here |
|
@jeffmccune I tried to recreate this issue (by checking out tag 2.6.17 from my local puppet installation) I initially ran into the following error (every test failed with this error) The issue was that the monkey patching code for that LogCollector was never evaluated b/c the namespace Puppet::Test::TestHelper existed. I modified the conditional statement from puppet_spec_helper.rb to get around the initial issue and the saw all tests (including the one that is failing on that jenkins instance) passing on 2.6.17 I have a feeling that I am missing something in the dependency matrix. |
|
@bodepd Ah, thanks for catching that. I'll take a look at this when I get into the office. We added the beginnings of a "real" API in Puppet itself to initialize Puppet for testing and it smells like the conditional logic in the spec helper isn't correctly determining if that API is present or not. |
|
Something else is wrong. I'm not seeing that issue at all and I don't need to modify the conditional with Puppet 2.6.17. Are you sure you're running the correct versions of this integration matrix? Integration: |
jeffmccune
pushed a commit
to jeffmccune/puppetlabs-stdlib
that referenced
this pull request
Aug 15, 2012
…esource"" This reverts commit 1e09833.
|
@bodepd There's definitely an order-dependent failure here, so this is an issue with the spec helper not initializing some global state in Puppet properly. I'll take care of this issue and update the ticket accordingly. While I'm also in here I'll fix up the legitimate failure in |
|
For posterity this random seed triggers 180+ false positive failures: 44365 This seed isolates the failure to the singe legit failure: 10252 To run: |
bodepd
pushed a commit
to bodepd/puppetlabs-stdlib
that referenced
this pull request
Aug 15, 2012
…esource"" This reverts commit 1e09833.
jeffmccune
pushed a commit
that referenced
this pull request
Aug 16, 2012
* bodepd-ensure_resource_attempt_2: Explicitly load functions used by ensure_resource Revert "Revert "Merge pull request #86 from bodepd/ensure_resource""
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit adds 2 new functions with unit tests.
defined_with_params works similarily to puppet's defined
function, except it allows you to also specify a hash of
params. defined_with_params will return true if a resource
also exists that matches the specified type/title (just like
with defined) as well as all of the specified params.
ensure_resource is a function that basically combines defined_with_params
with create_resources to conditionally create resources only if the
specified resource (title, type, params) does not already exist.
These functions are created to serve as an alternative to using
defined as follows:
The issue with this usage is that there is no guarentee about
what parameters were set in the previous definition of the package
that made its way into the catalog.
ensure_resource could be used instead, as:
This will creat the package resources only if another resource does
not exist with the specified parameters.