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

regsubst error in resource/location.pp with future parser #322

Closed
huasome opened this issue May 29, 2014 · 5 comments
Closed

regsubst error in resource/location.pp with future parser #322

huasome opened this issue May 29, 2014 · 5 comments

Comments

@huasome
Copy link

huasome commented May 29, 2014

The lines

  $location_sanitized_tmp = regsubst($location, '\/', '_', 'G')
  $location_sanitized = regsubst($location_sanitized_tmp, '\\', '_', 'G')

in location.pp generate an regsubst error when using the future parser. It appears to be caused by https://tickets.puppetlabs.com/browse/PUP-2612

@baurmatt
Copy link
Contributor

This can be fixed as followed:

-  $location_sanitized = regsubst($location_sanitized_tmp, '\\', '_', 'G')
+  $location_sanitized = regsubst($location_sanitized_tmp, '\\\\', '_', 'G')

But i actually don't know how to implemented it in a way that the module isn't broken for user of an older Puppet version.

@jfryman
Copy link
Contributor

jfryman commented Jul 11, 2014

That's the thing. Until future parser is mainline, I'm reluctant to fix this behavior that will break the rest of the general population.

agstubbs pushed a commit to wahanda/puppet-nginx that referenced this issue Jul 29, 2014
also sanitize locations with a ~ in them
@khaefeli
Copy link

khaefeli commented Sep 4, 2014

what's about an official puppet parser future branch?
if the parser becomes stable, merge it..

advantages:

  • already ready for the newest puppet versions on release date
  • supporting the development of puppet, when using and testing beta features

@3flex
Copy link
Contributor

3flex commented Sep 4, 2014

If there are changes required for future parser that don't affect other users they can be merged already. But there's enough work to be done in the master branch without having to maintain a separate branch for incompatibilities for those using the future parser so this is unlikely to happen. You can raise a separate issue for this if you wish.

Coming back to the original issue, Puppet's docs have been updated and say that backslash escape behavior has only changed for single-quoted strings with future parser. It should thus be possible to change these to equivalent double quoted strings. If someone wishes to raise a PR to change accordingly please go ahead.

https://docs.puppetlabs.com/puppet/latest/reference/lang_datatypes.html#single-quoted-strings
https://docs.puppetlabs.com/puppet/latest/reference/lang_datatypes.html#double-quoted-strings

@jfryman
Copy link
Contributor

jfryman commented Sep 4, 2014

@khaefeli I agree with @3flex here. The future parser is awesome, and lots of cool stuff happening in there. However, this module is geared toward compatibility with Puppet stable, especially folks running Puppet Enterprise. To that end, I am 👎 in merging any items that will create a logic split between parsers.

So, if future parser merges, then we'll support features there. If something in future parser does not cause issue with current parser, then it's eligible for merge. Otherwise, any only future-parser features will not be merged.

I know this might not be the desired outcome, but supporting unstable/unsupported is not an objective of this module.

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

No branches or pull requests

5 participants