Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(PUP-3054) Ensure that inherits are absolute #2981

Conversation

@zaphod42
Copy link
Contributor

zaphod42 commented Aug 15, 2014

PUP-121 was supposed to make all references to classes and resource types
absolute, but it looks like it missed a spot. The class referenced in an
inherits clause was not being looked up in an absolute manner, which
caused it to find the wrong parent class in certain cases. This changes
the future parser so that inherited classes are always absolute names.

zaphod42 added 6 commits Aug 15, 2014
The comment had trailing whitespace and a comma. It makes more sense
without either.
The resource_expressions_spec contained a useful way of writing language
specification tests. This extracts those out so that they can be reused
in other places.
PUP-121 was supposed to make all references to classes and resource
types absolute, but it looks like it missed a spot. The class referenced
in an inherits clause was not being looked up in an absolute manner,
which caused it to find the wrong parent class in certain cases. This
changes the future parser so that inherited classes are always absolute
names.
This changes the tests of the puppet language, which should become part
of the language specification's example and test suite, to be entirely
expressed in the puppet language. This removes any dependency on ruby!
Well, any dependency on ruby for specifying the language tests.
This removes some "dev mistake" error checking, which reduces the
overhead of some common calls. It also makes some calls less common,
most notably it caches the environment of a node when it wasn't
explicitly set and it falls back to using Puppet.lookup(:environments).
This can drastically reduce the number of environment lookups in some
situations. For the resource_expression_spec.rb tests, this resulted in
a 0.6 second improvement (~3 seconds to ~2.4 seconds).
One of the slowest thing for tests is setting values. They are reset and
changed often. This speeds up a few of the settings code paths by
removing unneeded method calls, either by introducing local variables,
or using delegators. In addition the ChainedValues objects are held on
to so that if the same environment and section is needed again it
doesn't need to be recreated.
@hlindberg

This comment has been minimized.

Copy link
Collaborator

hlindberg commented on lib/puppet/parser/scope.rb in 3bf56fa Aug 15, 2014

This changes the semantics, it is expected to return the appended value (from line 645), or the value if it was not appended. Either re-assign value on line 645, make lines 645 and 647 return, or continue doing the return table[value]

This comment has been minimized.

Copy link
Owner Author

zaphod42 replied Aug 15, 2014

since I was trying to get rid of an extra lookup, returning from 645 and 647 is probably the right thing

@hlindberg

This comment has been minimized.

Copy link
Collaborator

hlindberg commented on lib/puppet/pops/evaluator/runtime3_support.rb in 9787df8 Aug 15, 2014

This results in default values not being looked up if it is a Puppet::Parser::Resource. Is that change intentional? Since the tests were also changed it is not immediately apparent if it is intentional or not.

This comment has been minimized.

Copy link
Owner Author

zaphod42 replied Aug 15, 2014

Other way around. We can only get default values if it is a Puppet::Parser::Resource because it has a scope method. A Puppet::Resource doesn't have scope. Puppet::Resource is what is created after to_ral or to_resource (I think those are the methods) is called on the catalog.

@hlindberg

This comment has been minimized.

Copy link
Collaborator

hlindberg commented on spec/integration/parser/resource_expressions_spec.rb in 9787df8 Aug 15, 2014

possibly that catalog does not change, but that is hard to do

This comment has been minimized.

Copy link
Owner Author

zaphod42 replied Aug 15, 2014

I think that can actually be checked out by giving [] (an empty array) for the assertion. This triggers it to check that there are no extra resources in the catalog. It wasn't part of my original patch for this, but the way that I cherry-picked it over should allow this now.

@hlindberg

This comment has been minimized.

Copy link
Collaborator

hlindberg commented on spec/integration/parser/resource_expressions_spec.rb in 9787df8 Aug 15, 2014

the return values should also be tested - it should not return references

@hlindberg

This comment has been minimized.

Copy link
Collaborator

hlindberg commented on spec/integration/parser/resource_expressions_spec.rb in 9787df8 Aug 15, 2014

the deleted tests are also needed - they test that the correct value is produced. The change was to instead test only the side effect

This comment has been minimized.

Copy link
Owner Author

zaphod42 replied Aug 15, 2014

No, the deleted tests only checked the side effect. The old tests created the catalog and then asserted that the referenced resources exist in the catalog.

@hlindberg

This comment has been minimized.

Copy link
Collaborator

hlindberg commented on spec/integration/parser/resource_expressions_spec.rb in 9787df8 Aug 15, 2014

same here

@hlindberg

This comment has been minimized.

Copy link
Collaborator

hlindberg commented on spec/integration/parser/resource_expressions_spec.rb in 9787df8 Aug 15, 2014

here

@hlindberg

This comment has been minimized.

Copy link
Collaborator

hlindberg commented on spec/integration/parser/resource_expressions_spec.rb in 9787df8 Aug 15, 2014

and here

@hlindberg

This comment has been minimized.

Copy link
Collaborator

hlindberg commented on spec/integration/parser/resource_expressions_spec.rb in 9787df8 Aug 15, 2014

here

@hlindberg

This comment has been minimized.

Copy link
Collaborator

hlindberg commented on 9787df8 Aug 15, 2014

I think I misread the tests, all the arguments go to the produces... have to look more carefully

This comment has been minimized.

Copy link
Owner Author

zaphod42 replied Aug 15, 2014

Looking over these, I think there is a big gap in the test coverage. None of them test the actual value of the resource expression. :(

This comment has been minimized.

Copy link
Collaborator

hlindberg replied Aug 18, 2014

Well, the tests are for the 3x logic and there no values are produced since the expressions are not r-values. So... there is nothing to test unless future evaluator is used.

@hlindberg

This comment has been minimized.

Copy link
Contributor

hlindberg commented Aug 18, 2014

This PR manually merged at dbf3021 with fixup of the altered semantics of produced appended value in scope.

@hlindberg hlindberg closed this Aug 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.