(PUP-1597) Fix contain functions lookup of class with leading :: #2633
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
When the contain function creates edges it looked up resources without removing the leading :: in the name. The catalog never stores these, and thus an edge was formed between a resource and nil. This commit simply removes the ::. There is no checking that a resource is actually returned since they should all be there due to the call to include them in the catalog (which would fail if it was not possible to include them). A test is also added for the erronous (and now fixed case).
The contain() function made an error when dealing with a relative name. It assumed that the class that was actually included was always the same as the name that it was given. In many cases this is true, but when the name is found via a relative lookup (e.g. contain(bar) inside a class foo, when a foo::bar exists), the assumption breaks down. The source of the problem is that it tries to look up the included class after the fact. Without all of the extra information about namespaces it isn't able to find the right class. To fix this, the include() function has been updated to return the class resources that it included (including those that were already present). This allows the contain() function to create the appriate edges with the exact resources needed. A secondary change in this was to remove the checks for include not being able to include a class. The compiler method that is involved already makes that check and raises an error, so the include functions code was simply redundant.
The evaluate_classes method has a couple different responsibilities and inputs. This is just an attempt to pull them apart some and make the flows a little more straightforward.
|
CLA signed by all contributors. |
|
Looks good, we do need to ensure that future parser does not leak class resource objects into the puppet language. I have to check if it ensure that statement kind functions always result in nil. If that is the case, we should either add that, or make include and contain return PClassType instances to allow further operations on them. That requires a couple of changes to how the two functions interact. Maybe move the actual code into the compiler instead of having it in the functions? |
|
@hlindberg it looks like the runtime3 support already ensures that a statement function cannot return a value. https://github.com/zaphod42/puppet/blob/issue/master/pup-1597-contain-is-just-wrong/lib/puppet/pops/evaluator/runtime3_support.rb?pr=%2Fpuppetlabs%2Fpuppet%2Fpull%2F2633#L251 I'm going to just take one last look at it, but I think we can keep it as is for now. |
scope.resource was called multiple times. Even if this is a simple method, it is still a method call and is slower than just holding onto the result. This also removes some comments that appear to be out of date.
hlindberg
added a commit
that referenced
this pull request
May 12, 2014
…is-just-wrong (PUP-1597) Fix contain functions lookup of class with leading ::
openstack-gerrit
pushed a commit
to openstack-archive/puppet-pacemaker
that referenced
this pull request
Jun 29, 2016
Fix beaker job for pacemaker. When https://tickets.puppetlabs.com/browse/BKR-851 is fixed, remove it. The xenial node definition is now gated for Newton, add it to the mix. Fix puppet puppet-lint-absolute_classname-check. Work around puppetlabs/puppet#2633 where contain cannot have a absolute class name as parameter in puppet 3.6. For Ubuntu Xenial we are bitten by this https://tickets.puppetlabs.com/browse/BKR-821, where puppet is installed in /opt/puppetlabs/bin The package pacemaker-mgmt is not available on Ubuntu Xenial, adjust to this. Dbus is required by pacemaker and not installed as a dependency: http://logs.openstack.org/36/332836/7/check/gate-puppet-pacemaker-puppet-beaker-rspec-ubuntu-xenial/5e1ad11/console.html#_2016-06-23_16_03_00_952026 Add necessary support files for reno. Adjust the metadata.json. Change-Id: I22e207f758b94586ebf2333668c092fff3aaebac
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.
When the contain function creates edges it looked up resources without
removing the leading :: in the name. The catalog never stores these, and
thus an edge was formed between a resource and nil.
This also uncovered that contain didn't work correctly with relative names.
Both of these issues are now fixed.