Better regular expressions for mountpoint paths. #8
Conversation
|
Waiting for CLA signature by @silversupreme @silversupreme - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppetlabs.com/ Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppetlabs.com/community/trivial_patch_exemption.html |
|
I'm pretty sure this is a trivial change under your guidelines - it's a one-line change that updates two regular expressions. No real creative endeavour went into its creation. 😄 |
|
Hello there! Any updates on merging this? It's a simple change but it's pretty useful, and I'd like to not have to point my Puppetfile at my fork. 😺 |
|
Someone in #puppet encountered this same issue earlier today. Could we get an update on this pull request? |
|
If it helps, I just signed the CLA online. I had hoped that this would be small enough to not need it but there you are. 😺 |
| @@ -37,7 +37,7 @@ | |||
| validate do |value| | |||
| raise Puppet::Error, "name is not allowed to contain whitespace" if value =~ /\s/ | |||
| raise Puppet::Error, "name is not allowed to have trailing slashes" if value =~ %r{/$} | |||
| raise Puppet::Error, "name must be an absolute path" if value =~ %r{^[^/]} or value =~ %r{/../} | |||
| raise Puppet::Error, "name must be an absolute path" if ((value =~ /^[^\/]/) == 0) or ((value =~ %r{/\.\./}) == 0) | |||
There was a problem hiding this comment.
These should have been left without == 0. The first regular expression already restricts itself to the beginning of the string with ^ and the second regular expression should be allowed to match anywhere in the string. You don't want to let /mnt/location/../otherlocation through, for example.
There was a problem hiding this comment.
How does this look, as an alternative form?
(value !~ /^\// or value =~ /\/\.{2}\//)
There was a problem hiding this comment.
The fewer unnecessary changes the better. I'd go with:
raise Puppet::Error, "name must be an absolute path" if value =~ %r{^[^/]} or value =~ %r{/\.\./}
This should not choke on small directory names, like "pg". Thanks to @elyscape for pointing out a better way of doing this.
|
@elyscape I've rewritten this and rebased to make the history cleaner. Your version works with all the tests I do. Thank you for your recommendation here. 😺 |
|
Pinging some maintainers (@mhaskel, @hunner, @jeffmccune): any updates on merging this? |
Better regular expressions for mountpoint paths.
|
Thanks for the contribution @silversupreme, and sorry for the delay! We've removed the CLA bot from modules, and yes, this would've been considered 'trivial' anyways. |
This changeset modifies the
mountpointregular expressions which determine absolute paths, to not error out in a couple of cases that I could find. I was trying to use two-character directory names - to wit,/var/lib/pg/data- and this regex was choking on them.I've tried to make a general solution that works for all the forms of expressing a path.