[PUP-2711] The manifests directory should be recursively loaded when using directory environments Created: 2014/06/02  Updated: 2019/04/04  Resolved: 2014/07/08

Status: Closed
Project: Puppet
Component/s: Compiler, Docs
Affects Version/s: None
Fix Version/s: PUP 3.7.0

Type: New Feature Priority: Normal
Reporter: William Van Hevelingen Assignee: Joshua Partlow
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Template:
Story Points: 1
Sprint: Week 2014-6-25 to 2014-7-9
QA Contact: Kurt Wall

 Description   

We have a manifest directory with multiples subdirectories full of node definitions. We would prefer to stay with this layout rather than moving all the node definition files into the manifests directory.

It would also make migration easier to directory environments as all one would need to do is remove the import function calls.

manifests
    domain
         cs.pp
         ece.pp
         mme.pp
   [...]

This is documented in the following link: "Puppet will only read the first level of files in a manifest directory; it won’t descend into subdirectories."

http://docs.puppetlabs.com/puppet/latest/reference/dirs_manifest.html#directory-behavior-vs-single-file



 Comments   
Comment by Henrik Lindberg [ 2014/06/02 ]

This is certainly doable, all we need to do is to define the order in which all the results are concatenated. Currently it is alphanumerically ascending based on file name. I can think of a few schemes.

  • Alphanumerically per directory, if an entry is a directory it recurses
  • Files in a directory in alphanumerical order, before any Directories in alphanumerical order

Since this opens up for symlinks, and potentially to circular definitions, we must also decide how to deal with them:

  • Not allow symlinks to directories
  • Stop traversal if circular (silent)
  • Fail on circular

The recursion is easy to implement, the circular checking requires a bit more work. Assigning 3 story points.

Comment by Henrik Lindberg [ 2014/06/23 ]

Turned out to be much simpler. Have to delay this to 4.0 though since it otherwise will create the risk of people getting files from subdirectories that they did not anticipate being included.

Comment by Henrik Lindberg [ 2014/06/24 ]

I have an implementation of this in a branch. The main problem is one of the tests that results in manifestdir pointing to the root of puppet which means that all .pp files under puppet root in the source tree are imported - and since many of these are for testing of failures and thus contain deliberate errors, the result is that none of those tests pass.

Work should continue with fixing the node face tests.

Comment by Henrik Lindberg [ 2014/06/24 ]

Added link to branch with WIP. The works should be rebased to puppet-4 branch and the failing tests needs to be fixed.

Comment by Andrew Parker [ 2014/06/30 ]

After talking with William Van Hevelingen in IRC, I think we should try to get this into puppet 3.7, but gate it with the parser future so that users get the Puppet 4 behavior. The problem is that he can't upgrade and use directory environments until it has this functionality and by leaving it out of 3.7 it means that there isn't actually a good upgrade path for him.

I'm concerned about adding this functionality without the feature flag because of the possibility of code being loaded that would not have been before this change.

Comment by Henrik Lindberg [ 2014/06/30 ]

ok, we can do that (and the test that slurps the entire puppet code base for .pp files needs to be fixed).

Comment by Henrik Lindberg [ 2014/07/02 ]

PR added where the recursive behavior is gated on future parser. I also rebased it on master, and now the test that used to gobble up the entire world when recursive loading was on no longer does that.

Comment by Henrik Lindberg [ 2014/07/02 ]

The behavior is now 'alphabetical depth first order' - files and directories are processed in alphabetical order, a directory is processed by processing all of its content before processing the next entry at the same level.

Comment by Henrik Lindberg [ 2014/07/02 ]

Note added to predocs for future, and for BREAKS for 4.0

Comment by Henrik Lindberg [ 2014/07/03 ]

When doing FR for this, it is of interest to test symbolic links.

Comment by Andrew Parker [ 2014/07/07 ]

Merged into master in bb4608.

Comment by Joshua Partlow [ 2014/07/08 ]

Henrik Lindberg, working on FR for this, and noticed that having a directory in $manifest with a terminal '.pp' causes this to fail, both for current and future parser. While having a foo.pp directory is a bit pathological, I don't know that it should fail. The error is reasonably clear though: "Error: Could not retrieve catalog from remote server: Error 400 on SERVER: Could not parse for environment production: Is a directory - /home/jpartlow/test/master/recusive-manifestdir/manifests/foo.pp on node percival.corp.puppetlabs.net"

Thoughts?

Comment by Joshua Partlow [ 2014/07/08 ]

It does seem to be ignoring symlinked directories; this is because Ruby's Dir.glob ignores symlinks so as to avoid recursive loops. So we get option your original option 1 for dealing with circular directory references; was that the intention, Henrik Lindberg?

Comment by Henrik Lindberg [ 2014/07/08 ]

It was deliberate that it avoids symlinked directories (for the recursion problem). You have to use individually symlinked files if you are into sym-linking (and if people really do need symlinked directories, they can always come back and ask for it).

Regarding a directory that ends with .pp... hm, don't know what I think, on the one hand it is bad practice, as a simple listing makes you think it is a manifest (imagine a directory called site.pp. On the other hand it would be easy to filter them out.

Comment by Joshua Partlow [ 2014/07/08 ]

Just chatted with Henrik about about '*.pp' dirs, and symlink directories. The decisions were to leave both cases as is, and update pre-docs to make it clear that directories will be recursed, but symlinked directories will not, as a simple expedient to avoid circular references.

Comment by Joshua Partlow [ 2014/07/08 ]

Verified that with future parser, now loads recursively from directories. Updated pre-docs with a note about symlinked directories being skipped.

Generated at Tue Aug 11 00:47:23 PDT 2020 using Jira 8.5.2#805002-sha1:a66f9354b9e12ac788984e5d84669c903a370049.