Puppet versions >4 do not support full modulepath

[dylanratcliffe]

Logic here shows that for some reason we are not splitting the modulepath on a ':' if we are using a Puppet version greater than 4. Why are we doing this? I'm getting broken tests now as I cannot set more than one path as the modulepath.

My tests using older version: 2.2.0 My tests using current version: 2.3.0

[DavidS]

That slipped through the cracks when refactoring this section in https://github.com/rodjek/rspec-puppet/commit/e44da91788284fe61939adce006b6b3704725f21 . I guess it doesn't hurt splitting RSpec.configuration.module_path on colons.

I'm tempted to add that puppet 4 has deprecated the global modulepath setting (https://docs.puppetlabs.com/references/latest/configuration.html#modulepath) , but I realize that it's not relevant to this bug.

[DavidS]

/cc @adrienthebo

[adrienthebo]

See #349, @dylanratcliffe could you test that pull request?

[dylanratcliffe]

@adrienthebo Sorry about the delay, been out training. Seems like more has broken now, here are the results of my tests. And the actual error.

Here is what the spec_helper.rb looks like:

require 'puppetlabs_spec_helper/module_spec_helper'

RSpec.configure do |c|
  c.parser = 'future'
  c.module_path = '/var/folders/tv/svz3wzy550j8dktpxs_vqprm0000gn/T/r10k20160118-54626-g4wxeq/etc/puppetlabs/code/environments/production/site:/var/folders/tv/svz3wzy550j8dktpxs_vqprm0000gn/T/r10k20160118-54626-g4wxeq/etc/puppetlabs/code/environments/production/modules'
  c.hiera_config = '/var/folders/tv/svz3wzy550j8dktpxs_vqprm0000gn/T/r10k20160118-54626-g4wxeq/etc/puppetlabs/code/environments/production/hiera.yaml'
end

The test it is running is just an it { should compile } test

[DavidS]

Puppet 4 needs an environmentpath. Rspec-puppet should warn when module_path, but not environmentdir is set.

mobile

On Jan 17, 2016 23:26, "Dylan" notifications@github.com wrote:

@adrienthebo https://github.com/adrienthebo Sorry about the delay, been out training. Seems like more has broken now, here are the results of my tests https://travis-ci.org/dylanratcliffe/puppet_controlrepo/builds/103006059. And the actual error https://travis-ci.org/dylanratcliffe/puppet_controlrepo/jobs/103006062#L376.

Here is what the spec_helper.rb looks like:

require 'puppetlabs_spec_helper/module_spec_helper' RSpec.configure do |c| c.parser = 'future' c.module_path = '/var/folders/tv/svz3wzy550j8dktpxs_vqprm0000gn/T/r10k20160118-54626-g4wxeq/etc/puppetlabs/code/environments/production/site:/var/folders/tv/svz3wzy550j8dktpxs_vqprm0000gn/T/r10k20160118-54626-g4wxeq/etc/puppetlabs/code/environments/production/modules' c.hiera_config = '/var/folders/tv/svz3wzy550j8dktpxs_vqprm0000gn/T/r10k20160118-54626-g4wxeq/etc/puppetlabs/code/environments/production/hiera.yaml'end

The test it is running is just an it { should compile } test

— Reply to this email directly or view it on GitHub https://github.com/rodjek/rspec-puppet/issues/343#issuecomment-172395142 .

[dylanratcliffe]

Okay so even if I update my spec_helper.rb with an environmentpath I'm still getting the same issue:

require 'puppetlabs_spec_helper/module_spec_helper'

RSpec.configure do |c|
  c.parser = 'future'
  c.environmentpath = '/var/folders/tv/svz3wzy550j8dktpxs_vqprm0000gn/T/r10k20160119-25736-rn76b3/etc/puppetlabs/code/environments'
  c.module_path = '/var/folders/tv/svz3wzy550j8dktpxs_vqprm0000gn/T/r10k20160119-25736-rn76b3/etc/puppetlabs/code/environments/production/site:/var/folders/tv/svz3wzy550j8dktpxs_vqprm0000gn/T/r10k20160119-25736-rn76b3/etc/puppetlabs/code/environments/production/modules'
  c.hiera_config = '/var/folders/tv/svz3wzy550j8dktpxs_vqprm0000gn/T/r10k20160119-25736-rn76b3/etc/puppetlabs/code/environments/production/hiera.yaml'
end

[adrienthebo]

@dylanratcliffe could you provide steps to reproduce this? I don't have a readily available environment with multiple directories in the environmentpath so an example would really help to chase this one down.

[dylanratcliffe]

@adrienthebo Yeah that will probably help. I'm getting the issue when using a tool that I wrote for testing controlrepos here. I am trying to use it with the following controlrepo

In order to reproduce this:

1. Clone my repo (production)
2. bundle install
3. bundle exec rake controlrepo_spec This will create a temp directory and install all of the modules from the Puppetfile in there, it will then dynamically generate tests under that directory and run them with rake spec. For debugging I recommend looking at where the temp dir is e.g. /var/folders/tv/svz3wzy550j8dktpxs_vqprm0000gn/T/r10k20160119-25736-rn76b3 and then checking on the tests under spec/...

Does that make sense?

[adrienthebo]

@dylanratcliffe that's actually an enormous help; I've been able to make some solid progress with this.

I've been keeping a bunch of information in my head about the behavior of Puppet environments, the per-environment manifest file, and version specific behaviors. I also have a terrible memory and this information might be useful to other people. To get this information into a shareable format the following is a dump of what I've learned about Puppet environments, versions, per-environment manifests, and how all of this relates to rspec-puppet.

In the very early days of Puppet, there were no environments. You got one modulepath, one manifest, etc. Eventually someone plumbed in support for environments (generally referred to as config environments), but this was not really well fleshed out. Information leaked between environment like water leaks through a net, and it caused all sorts of neat and hard to fix bugs. Further complicating things was the pattern of interpolating the $environment setting in other Puppet settings, which made a bad situation worse. Interpolating the $environment variable allowed environments to be discovered at runtime but the system itself was completely unprepared for environments to be constantly switched.

Puppet 3.5.0 added the concept of directory environments, which included a large amount of work to make it easier to create environments on the fly while not leaking per-environment information across the system. This system was generally a great improvement but was much more complex - owing to the fact that it didn't completely gloss over the fact that storing per-environment configuration is pretty tricky. However this did make a bunch of changes to the internals of Puppet, so to preserve backwards compatibility the old mechanism of creating environments was preserved.

Since rspec-puppet needs to be able to control what code is tested and needs to add things like fixture modules, rspec-puppet itself has to monkey quite heavily with environment configuration. In Puppet 3.x this was pretty easy because it could rely on the old config file base environments - which handled singleton environments pretty well. Until Puppet 4.0 this more or less swept the API changes for directory environments under the rug.

In Puppet 4.0 the old config environment code was largely removed, which brought this issue to front and center. This broke a ton of things because it yanked the assumptions about global configuration. Being able to configure Puppet by stuffing settings into Puppet[:some_random_setting] fell flat. rspec-puppet had to preserve support for the old config file based environments as well as the Puppet 4.0 only directory environments. As it turns out this was not a simple task and had a number of touch points. Unfortunately the version specific code paths were scattered across the code paths, and there wasn't a lot of sharing between Puppet < 4.0 and Puppet >= 4.0.

The duplication of code paths meant that it was very easy for functionality to be added for one set of Puppet versions and completely break for the other set of versions. The biggest issue of this was having various colon separated paths behave correctly - in some cases we supported colon separated paths but as this ticket demonstrated this wasn't comprehensive.

The work I did earlier on abstracting differences in Puppet versions out the adapter classes was a good start, but it was only meant to be a start - there's a lot more work to be done.

Thanks to @dylanratcliffe's help, I've been able to add support multi-value environmentpaths and modulepaths across Puppet versions on a local branch. However this is continuing to uncover more issues in how Puppet behaves across different versions and how rspec-puppet interacts with Puppet.

The issue that Dylan discovered with the fixes I've proposed before is actually an improvement in how Puppet handles manifests. In Puppet 3.x and earlier, Puppet had a bunch of special cased behavior around the manifest. If the file specified by Puppet[:manifest] didn't exist, Puppet would magically ignore it - this allowed things like ENCs with no site.pp files to work. However it's magic, and since we don't live in Hogwarts magic is generally a source of sorrow rather than a way to get things done. The Puppet directory environments added more concrete support for a missing manifest file - but this was done by adding proper handling where a per-environment manifest didn't exist. If the manifest file wasn't given, it was signified by setting an environment manifest to the symbol :no_manifest and the constant Puppet::Node::Environment::NO_MANIFEST. In terms of sanity this is better in that we aren't magically ignoring a value, but it turns out that calling File.exist?(:no_manifest) can and will raise an exception.

This is the point where we wrap around to the relevant point.

In Puppet 3.x, we could always supply a random path to the Puppet[:manifest] setting and we could rely on Puppet blindly ignoring it where it was convenient, and rspec-puppet has been build to replicate the same sort of "see no evil, hear no evil, do no evil" philosophy of engineering. In Puppet 4.0 we no longer have this luxury - any code that needs to interact with an environment's manifest needs to account for the fact that this value is unset.

Here's how I see we should continue forward.

1. The RSpec::Puppet::Adapters::Base#manifest method needs to be either a path, or nil. This should be explicitly documented so we don't run into this silliness again.
2. The Puppet 2.7 and 3.x adapters should see if Puppet[:manifest] setting is actually a file, and if it's not then the #manifest function should return nil.
3. The Puppet 4.x adapter needs to see if the manifest setting is nil, and when it is then the created environment needs to be passed an environment of Puppet::Node::Environment::NO_MANIFEST setting. The #manifest method needs to check if the environment has a manifest file set and that it's a file, and not a symbol.
4. All code that calls #manifest needs to account for the returned value for be nil, and behave intelligently.

This has been a lot of words, so I should probably summarize things.

1. Puppet environments were originally not well integrated into the Puppet code base.
2. Code in the Puppet ecosystem, including rspec-puppet, began to rely on Puppet's (lack of) handling of environments.
3. Puppet's handling of the manifest setting on a global and per-environment level in <= 3.x was magical.
4. Code in the Puppet ecosystem, including rspec-puppet, replicated Puppet's magical handling of the manifest setting.
5. Puppet directory environments fixed a great deal of these issues but introduced a number of API changes, but Puppet 3.x supported the old APIs for compatibility.
6. rspec-puppet relied on the backward compatibility in Puppet 3.x to make things work.
7. Puppet 4.0 removed the backwards compatibility code, which made rspec-puppet have to deal with the API changes.
8. The rspec-puppet support for Puppet 4.0 was added incrementally and added a number of separate code paths, which caused bugs to build up depending on what Puppet version was used.
9. There has been some partial deduplication of code paths and extractions of version specific code which has improved the situation
10. The improvements in 9. need to be expanded to colon separated paths
11. The improvements in 9. need to be expanded to version specific handling of the Puppet manifest setting.

[dylanratcliffe]

Wow, this is awesome (the write up not the situation, the situation is gross). I see where the confusion is coming from (I think...). How can I help? I'm happy to put a workaround in my code for the time being as it won't be customer facing and I can change it once rspec-puppet is fixed, whatever I can do to help.

[adrienthebo]

Thanks :) I mainly wanted to get all of this out of my head before I forgot it. The actual code changes aren't too heavy; at this point it's mainly a matter of fixing a bunch small chunks of code with bad assumptions. I should get to this "real soon" (TM).

[DavidS]

Thanks for keeping the ball rolling here, @adrienthebo !

One thing about environments and manifests I only learned yesterday is that with puppet 4 and directory environments all files inside the environment's manifests directory are loaded. I'm not sure yet how/if this impacts your work here.

[dylanratcliffe]

@adrienthebo I had a bit more of a look into this, turns out there was combination of two issues. Your branch here: https://github.com/adrienthebo/rspec-puppet/tree/issue-343-puppet-4-modulepath fixes the modulepath issue, I was coming across a second issue where I wasn't setting the manifest setting and was therefore getting errors. I have updated by code to pass this in from environment.conf and it all works perfectly. If you could go ahead and merge your branch would be great, I'll update the dependencies on my side too

[dylanratcliffe]

Test Results

[adrienthebo]

@dylanratcliffe you shouldn't have needed to set the manifest; that's just a change in how Puppet 4 handles unset manifest values for an environment. I've updated my PR, could you give the PR another test?

[adrienthebo]

Errr, updated my branch, not my PR. I'll submit a PR as well

[adrienthebo]

OH COME ON I DO HAVE A PULL REQUEST FILED anywho try out https://github.com/rodjek/rspec-puppet/pull/349 and if you're happy with it (and @DavidS approves) we can merge it.

[dylanratcliffe]

Both Ranjit and Chris Roddy came across this today, switched to your branch and it was fixed, so I can say pretty confidently that it's good.

[DavidS]

349 seems to fix this and I just merged it.