$location_sanitized variable present in code but unused

[mattkenn4545]

NOTE: the initial problem this issue aimed to resolve is not longer valid. See comments and discussion in pull request.

Affected Puppet, Ruby, OS and module versions/distributions

• Puppet: 3.8 (future parser)
• Ruby: 2.1
• Distribution: Official OSS
• Module version: v0.2.7 however code with issue is in master

How to reproduce (e.g Puppet code you use)

nginx::resource::location{ 'valid_location':
  location =>  "~ ^/login/([\w-]+)?/?(.*)?$"
} 

What are you seeing

in resource/location.pp the line $location_sanitized = regsubst($location_sanitized_tmp, '\\\\', '_', 'G') will dutifully change '\' to '_' however ~ ^/login/([\w-]+)?/?(.*)?$ is a valid nginx location and changing \ to _ breaks functionality.

What behaviour did you expect instead

regsubst() was broken in 3.8 sans future parser see https://tickets.puppetlabs.com/browse/PUP-2612 so that line had no effect. However, enabling future parser results in invalid config due the sanitization working as intended originally.

Potential fix is to only do that sanitization if the location does NOT start with '~'

Output log

Any additional information you'd like to impart

[wyardley]

Does this affect Puppet 4.x as well, or just 3.x with future parser? I am pretty sure we are not actively fixing new bugs for 3.x now that it's EOL.

[mattkenn4545]

This issue is exposed by switching to future parser for me but effects puppet 4 as well.

The code as written does not account for locations that legitimately have backslash characters in them. For example locations that use regex matching

[mattkenn4545]

I'll get a pull request ready. Here it is #1007

[wyardley]

Thanks!
Added some comments here #1007 (comment)
also edited your original issue just very slightly to wrap some of the characters that Github was interpreting as markup in backticks.

[wyardley]

Just saw your comments in #1007 (was in the middle of trying to reproduce the issue in Beaker), I don't think $location_sanitized is used at all in recent versions (I even looked back in 0.4)... You can rework it to remove those lines entirely (or make a new PR for that) if you'd like; I think that would make some sense.

There have been lots of changes, so let us know if you run into problems if you do upgrade.

[mattkenn4545]

Title and node added to description.

TL;DR $location_sanitized prior 0.3.0 was used to build the title and content of the location concat fragment. Now this variable is unused. This issue is not aimed to remove the dead code.

[rnelson0]

Thanks for identifying and removing the unused code!