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-2711) Make manifest-directory load recursively when future parser is on #2834

Merged

Conversation

@hlindberg
Copy link
Contributor

hlindberg commented Jul 3, 2014

This makes recursive loading of manifests available when the future parser is on. All .pp files in the manifestdir and its subdirectories will be loaded in alphabetical depth first order (a directory's entries are processed alphabetically, if it is a directory, it is processed before the remaining files in the current directory).

@hlindberg

This comment has been minimized.

Copy link
Contributor Author

hlindberg commented Jul 3, 2014

Fixtures for testing missing in last commit. Fixed.

@puppetcla

This comment has been minimized.

Copy link

puppetcla commented Jul 3, 2014

CLA signed by all contributors.

parser.file = File.join(file, pp_file)
parser.parse
if Puppet[:parser] == 'future'
parse_results = Dir.glob(File.join(file, '**/**.pp')).sort.map do | file_to_parse |

This comment has been minimized.

Copy link
@zaphod42

zaphod42 Jul 3, 2014

Contributor

Shouldn't this be **/*.pp? I'm not sure what the **.pp will end up doing.

This comment has been minimized.

Copy link
@zaphod42

zaphod42 Jul 3, 2014

Contributor

On windows this is going to have problems. The file separator is \ which ends up expanding the path so something like ...\**/*.pp which escapes the first * and stops it from recursing.

This comment has been minimized.

Copy link
@joshcooper

joshcooper Jul 3, 2014

Member

I do get different results if file has forward vs backwards slashes:

C:\work\puppet>irb
irb(main):001:0> Dir.glob(File.join('c:\work\puppet\**', '*.rb'))
=> []
irb(main):002:0> Dir.glob(File.join('c:/work/puppet/**', '*.rb'))
=> ["c:/work/puppet/acceptance/config/git/options.rb","c:/work/puppet/acceptance/config/packages/options.rb","c:/work/puppet/acceptance/lib/acceptance_spec_helper.rb"....]

This comment has been minimized.

Copy link
@zaphod42

zaphod42 Jul 3, 2014

Contributor

This should probably be going through the Puppet::FileSystem layer and then it can deal with differences correctly. A good api might be something like Puppet::FileSystem.glob(*parts) which allows it to handle any unwanted escaping itself.

This comment has been minimized.

Copy link
@hlindberg

hlindberg Jul 3, 2014

Author Contributor

The **.p does the same as *.p, and should be changed to just *.p.

This comment has been minimized.

Copy link
@zaphod42

zaphod42 Jul 3, 2014

Contributor

On the topic of the FileSystem abstraction, there is Puppet::FileSystem::PathPattern, which is used to make safe path patterns.

This comment has been minimized.

Copy link
@zaphod42

zaphod42 Jul 3, 2014

Contributor

I just tried this out on windows. Somehow it works. Based on some error output I got at one point there is something that is translating windows path separators into unix path separators.

This comment has been minimized.

@zaphod42

This comment has been minimized.

Copy link
Contributor

zaphod42 commented Jul 3, 2014

@hlindberg the message for commit cee805c references that it is a WIP. I think that isn't true anymore and so the message needs to be fixed up.

I think that the glob should either use the PathPattern instance or should go through the Puppet::Filesystem layer in order to get more of our file handling going through that layer.

hlindberg added 4 commits Jun 23, 2014
This changes the loaded of a manifest reference that is a directory
to include all .pp recursively in this directory. The content is
sorted so that all content of directory b comes before file c in its
parent directory (rule applied recursively).
This makes the ability to load all .pp files under a manifest dir
recursively conditional to parser == future. (It will become the
standard in Puppet 4.0). This is done to avoid slurping in lots of files
in case users kept other files around in a subdirectory in the belief
that they would not be loaded. Thus making it harder to upgrade.

This also adds a test that runs for future parser.
@hlindberg

This comment has been minimized.

Copy link
Contributor Author

hlindberg commented Jul 4, 2014

commit message change, **.pp changed to *.pp, and the PathPattern class is now used instead of Dir.glob directly (that added validation that the given absolute file does not have certain unwanted properties).

zaphod42 added a commit that referenced this pull request Jul 7, 2014
(PUP-2711) Make manifest-directory load recursively when future parser is on
@zaphod42 zaphod42 merged commit bb4608b into puppetlabs:master Jul 7, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@hlindberg hlindberg deleted the hlindberg:PUP-2711_recursive-manifest-dir branch Jul 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.