(PE-20308) Pass a literal type and not a string to findresource#761
Merged
thallgren merged 1 commit intopuppetlabs:masterfrom Apr 26, 2017
Merged
(PE-20308) Pass a literal type and not a string to findresource#761thallgren merged 1 commit intopuppetlabs:masterfrom
thallgren merged 1 commit intopuppetlabs:masterfrom
Conversation
thallgren
suggested changes
Apr 25, 2017
|
|
||
| # Workaround for PE-20308 | ||
| if reference.is_a?(String) | ||
| type_name, title = Puppet::Resource.type_and_title(reference, nil) |
Contributor
There was a problem hiding this comment.
The Puppet::Resource#type_and_title class method was introduced in Puppet 4.5.0. Perhaps that's what causing appveyor to fail for 4.2.3?
Contributor
Author
There was a problem hiding this comment.
I'll just fallback to the old behavior pre 4.5.0 as the ticket only reports the behavior on newer puppet versions.
| if reference.is_a?(String) | ||
| type_name, title = Puppet::Resource.type_and_title(reference, nil) | ||
| elsif reference.is_a?(Puppet::Resource) | ||
| type_name, title = Puppet::Resource.type_and_title(reference.type, reference.title) |
Contributor
There was a problem hiding this comment.
Would it not be more efficient to just ask the resource for it's type here?
type = reference.resource_type
Contributor
Author
There was a problem hiding this comment.
I had been looking for something like that originally, but overlooked it. Thanks!
hunner
force-pushed
the
fix_type_loading
branch
2 times, most recently
from
6d1662c to
746ddbb
Compare
- `defined_with_params` calls `findresource(reference.to_s)` - `findresource` is https://github.com/puppetlabs/puppet/blob/4.8.1/lib/puppet/parser/compiler.rb#L407 and points to https://github.com/puppetlabs/puppet/blob/4.8.1/lib/puppet/resource/catalog.rb#L352 - This calls `Puppet::Resource.new` with the type https://github.com/puppetlabs/puppet/blob/4.8.1/lib/puppet/resource/catalog.rb#L366 - This ends up calling `resource_type` via https://github.com/puppetlabs/puppet/blob/4.8.1/lib/puppet/resource.rb#L317-L319 and that ends up declaring the type via the autoloader api at https://github.com/puppetlabs/puppet/blob/4.8.1/lib/puppet/resource.rb#L390 - But why does the autoloader API fail to find it in the current environment? - Okay, so when the autoloader is trying to find the type, it uses the typeloader to look it up in the current environment https://github.com/puppetlabs/puppet/blob/4.8.1/lib/puppet/metatype/manager.rb#L171 - this calls `get_file` and `mark_loaded` https://github.com/puppetlabs/puppet/blob/4.8.1/lib/puppet/util/autoload.rb#L64-L67 Suggested workaround is to pass a literal type instead of a string to `findresource` to fix in stdlib, and also to fix loading/requiring of type in core puppet. This seems to affect more recent versions of puppet, so fallback to original behavior on pre-4.5
hunner
force-pushed
the
fix_type_loading
branch
from
746ddbb to
4f19c27
Compare
thallgren
approved these changes
Apr 26, 2017
Contributor
|
This change doesn't work with defined types, only native types - I filed https://tickets.puppetlabs.com/browse/MODULES-4790 (tests). Unfortunately this affects |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
defined_with_paramscallsfindresource(reference.to_s)findresourceishttps://github.com/puppetlabs/puppet/blob/4.8.1/lib/puppet/parser/compiler.rb#L407
and points to
https://github.com/puppetlabs/puppet/blob/4.8.1/lib/puppet/resource/catalog.rb#L352
Puppet::Resource.newwith the typehttps://github.com/puppetlabs/puppet/blob/4.8.1/lib/puppet/resource/catalog.rb#L366
resource_typeviahttps://github.com/puppetlabs/puppet/blob/4.8.1/lib/puppet/resource.rb#L317-L319
and that ends up declaring the type via the autoloader api at
https://github.com/puppetlabs/puppet/blob/4.8.1/lib/puppet/resource.rb#L390
environment?
typeloader to look it up in the current environment
https://github.com/puppetlabs/puppet/blob/4.8.1/lib/puppet/metatype/manager.rb#L171
get_fileandmark_loadedhttps://github.com/puppetlabs/puppet/blob/4.8.1/lib/puppet/util/autoload.rb#L64-L67
Suggested workaround is to pass a literal type instead of a string to
findresourceto fix in stdlib, and also to fix loading/requiring oftype in core puppet.