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

Add a transitive test property #821

Closed
wants to merge 36 commits into from
Closed

Conversation

ekohl
Copy link
Contributor

@ekohl ekohl commented Mar 15, 2021

This is a minimal reproducer that's essentially A ~> B ~> C. The that_subscribes_to matcher shows that this is equal to A ~> C but in practice Puppet doesn't work that way. The service is not restarted if the file is modified.

I don't know what is the expected behavior here, but it does mean that today it's not possible to test for real world behavior.

I've submitted this as a PR since it's the easiest way to submit code and show CI results for it.

@sanfrancrisko
Copy link
Contributor

Thanks for highlighting this @ekohl. Let me run this past a few colleagues, but it would seem logical that the test framework is behaving the same way as Puppet behaves, at least. However, we can start a separate conversation with the Puppet core team if you feel there's an issue with how chained notifies are working at the moment?

@ekohl
Copy link
Contributor Author

ekohl commented May 10, 2021

I would like a formal statement from the core team how they think it should work. From there we can decide how to approach this.

@sanfrancrisko
Copy link
Contributor

I pinged some of my colleagues from the agent team and they believe that A should notify both B and C (C being triggered by B) in the case outlined in theforeman/puppet-foreman_proxy#653. I'll relay your manifest here to them and see if I can reproduce it too. If so, we'll get a bug raised as it seems to be contradicting the expected behaviour: https://puppet.com/docs/puppet/7.6/lang_relationships.html#lang_rel_chaining_arrows

@ekohl
Copy link
Contributor Author

ekohl commented May 11, 2021

It is my impression that it relies on some resource triggering the notify. So if you have:

Class['A'] ~> Class['B'] ~> Class['C']

Then it works if there's a resource inside Class['B'] that triggers a refresh. However, we've seen the edge case where it wasn't. Perhaps classes also behave different than resources.

da-ar and others added 21 commits July 26, 2021 16:36
Starting with versions 7.12.0/6.25.0, Puppet was changed not to directly
depend on Facter anymore, but to use a `Puppet::Runtime` implementation
instead (e.g. calls to `Facter` were changed to
`Puppet.runtime[:facter]` to allow for pluggable Facter backends).

rspec-puppet stubs facts from facterdb by setting custom facts with
higher weights, meaning that Facter 4 will still resolve the underlying
core facts (which is by design), leading to noticeable performance hits
when compiling catalogs with Facter 4 as opposed to Facter 2 (which just
returned the custom fact).

This means we can achieve a pretty big performance improvement with
rspec-puppet by registering a custom Facter implementation that bypasses
Facter altogether and just saves facts to hash.

This behavior cand be activated by setting `facter_implementation` to
`rspec` in `RSpec.configure`. By default, the setting has the value of
`facter` which maintains the old behavior of going through Facter. If
`rspec` is set but the Puppet version does not support Facter
implementations, rspec will warn and fall back to using Facter.
Add setting to use custom Facter implementation
Using the 'rspec' facter implementation we could sometimes get in a case
where the values of FacterImpl and Puppet.runtime[:facter] diverged
between example groups.

Because rspec-puppet overrides facts using the FacterImpl constant
(which is only set once), Puppet.runtime[:facter] would point to a
different instance of FacterTestImpl with no available facts, causing
calls to Puppet.runtime[:facter].value to fail.

To prevent this from happening, set Puppet.runtime[:facter] to the value
of FacterImpl; this way we make sure Puppet.runtime[:facter] and
FacterImpl operate on the same instance of FacterTestImpl.
Ensure FacterImpl consistency between example groups
…p_2_11_1

(MAINT) Release prep for v2.11.1
…gelog_index

(MAINT) Add v2.11.1 entry to docs/changelog/index.md
ekohl added a commit to ekohl/puppet-foreman that referenced this pull request Dec 16, 2021
With Puppet you can write:

    Class['A'] ~> Class['B'] ~> Class['C']

This implies Class['A'] ~> Class['C'] and rspec-puppet actually will
tell you that chaining is there, but it doesn't actually happen. This
was reported in rodjek/rspec-puppet#821.

In this particular case it means that if DB seeding doesn't happen then
the apipie caches indexes aren't refreshed. By chaining it to db:migrate
there's a much bigger chance it actually happens.

Normally apipie:cache:index needs to run after a package is updated so
ideally this would actually be done in packaging, but this is the
workaround we've been using for a long time.
@evgeni
Copy link

evgeni commented Dec 17, 2021

@sanfrancrisko was there ever any progress on this discussion? We've just hit this problem again and while re-reading https://puppet.com/docs/puppet/7/lang_relationships.html#lang_rel_chaining_arrows I can't really tell if A ~> B ~> C should be equivalent to A ~> B, A ~> C, B ~> C or A ~> B, B ~> C?

More explicitly, the chaining example says "Resource declarations can be chained. That means you can use chaining arrows to make Puppet apply a section of code in the order that it is written. This example applies configuration to the package, the file, and the service, in that order, with each related resource notifying the next of any changes" (emph. mine) where one could interpret "any changes" as "any changes to itself" and not "any changes in the whole chain".

seanmil and others added 3 commits January 11, 2022 13:50
When encountering results containing nil from an autorequire-family
call, Puppet will skip over them without error but rspec-puppet
results in an error. This aligns rspec-puppet with Puppet's
behavior.

Also, rename the autorelationships spec test so that it runs
properly as part of a 'rake spec' invocation.
Use notify, an OS-neutral resource type, instead of File for
the testing.
cegeka-jenkins pushed a commit to cegeka/puppet-foreman that referenced this pull request Feb 3, 2022
With Puppet you can write:

    Class['A'] ~> Class['B'] ~> Class['C']

This implies Class['A'] ~> Class['C'] and rspec-puppet actually will
tell you that chaining is there, but it doesn't actually happen. This
was reported in rodjek/rspec-puppet#821.

In this particular case it means that if DB seeding doesn't happen then
the apipie caches indexes aren't refreshed. By chaining it to db:migrate
there's a much bigger chance it actually happens.

Normally apipie:cache:index needs to run after a package is updated so
ideally this would actually be done in packaging, but this is the
workaround we've been using for a long time.
binford2k and others added 6 commits July 20, 2022 15:15
This allows you to check the return type of functions, like this
example:

```
is_expected.to run.with_params('mymod/template.epp', { 'foo' => 'bar' }).and_return(kind_of String)
```
Add the ability to use `kind_of` matchers
@ekohl
Copy link
Contributor Author

ekohl commented Sep 27, 2022

Please continue the discussion in puppetlabs#29

binford2k and others added 6 commits October 20, 2022 15:52
(MAINT) Merging old master in to main
Update README.md to include sensitive function
This is a minimal reproducer that's essentially A ~> B ~> C. The
that_subscribes_to matcher shows that this is equal to A ~> C but in
practice Puppet doesn't work that way. The service is not restarted if
the file is modified.

I don't know what is the expected behavior here, but it does mean that
today it's not possible to test for real world behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.