The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

This issue tracker is now in read-only archive mode and automatic ticket export has been disabled. Redmine users will need to create a new JIRA account to file tickets using https://tickets.puppetlabs.com. See the following page for information on filing tickets with JIRA:

Bug #10146

Puppet interpolates variables differently in 2.7.x

Added by Philip Gardner over 4 years ago. Updated over 3 years ago.

Status:ClosedStart date:10/18/2011
Priority:UrgentDue date:
Assignee:-% Done:

0%

Category:language
Target version:2.7.20
Affected Puppet version:2.7.5 Branch:https://github.com/puppetlabs/puppet/pull/757
Keywords:

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com


Description

I’m not sure when this changed, but I recently upgraded from 2.6.7 to 2.7.5.

Given: file { “/usr/local/$lsbdistid-$architecture”: ensure => directory }

In 2.6.7 the following would generate a directory name called “/usr/local/CentOS-x86_64”:

However, in 2.7.5, this now creates a directory called “/usr/local/-x86_64”

If you escape the variables, the resource is created correctly, however this wasn’t clear until I jumped on IRC.


Related issues

Related to Puppet - Bug #5268: hyphen in class name messes with qualified variables Re-opened 11/11/2010
Related to Puppet - Feature #17260: Optional deprecation warning for hyphens in names Closed
Duplicated by Puppet - Bug #5447: Fix duplicate definition error Duplicate 12/02/2010
Duplicated by Puppet - Bug #12888: Puppet variable name interpretation bug Duplicate 02/29/2012

History

#1 Updated by Brice Figureau over 4 years ago

2.7 has a completely rewritten string interpolation. ‘–’ is valid in a variable name. It shouldn’t be valid when used at the end of a variable name though, in which case this would have been parsed correctly.

#2 Updated by Henrik Lindberg over 4 years ago

Brice Figureau wrote:

2.7 has a completely rewritten string interpolation. ‘–’ is valid in a variable name. It shouldn’t be valid when used at the end of a variable name though, in which case this would have been parsed correctly.

Not allowing variable to end with ‘–’ is good as it also resolves ambiguities regarding minus as in $b-$c, $b—$a.

#3 Updated by James Turnbull over 4 years ago

  • Status changed from Unreviewed to Needs Decision
  • Assignee set to Nigel Kersten

#4 Updated by Bostjan Skufca over 4 years ago

This “feature” renders my whole catalog useless, and I am quite sure I am not alone. This is effectively preventing me from upgrading to 2.7.x release.

How far are we from this design decision?

b.

#5 Updated by James Turnbull over 4 years ago

Bostjan – there is a simple work-around – have you tried that?

#6 Updated by Nigel Kersten over 4 years ago

  • Category changed from documentation to language
  • Status changed from Needs Decision to Accepted
  • Assignee deleted (Nigel Kersten)
  • Priority changed from Normal to High

To reiterate, you should always take the safer approach and escape the variable names when interpolating as per the original report, even before this change was made.

Variables should not be allowed to end in “–” however.

#7 Updated by Bostjan Skufca over 4 years ago

James Turnbull wrote:

Bostjan – there is a simple work-around – have you tried that?

Hello James,

do you mean using braces like ${variable-name1}–${variable-name2}?

b.

#8 Updated by Nigel Kersten over 4 years ago

yes.

#9 Updated by Bostjan Skufca over 4 years ago

Yes, I am aware of that “workaround”.

Anyway, there are 2 things to consider here: 1. It renders my (I don’t mean to be egocentric here, applies to anyone in the same situation) whole module catalog useless if I upgrade to puppet 2.7.x. 2. It is much less readable: “${variable1}–${variable2}” vs “$variable1-$variable2”. This also aligns with how programming languages/shells parse mentioned statements (end result is the same).

In order to speed up upgrade rate (which is probably your desire, because results in less support for old versions) I believe you should fix this one ASAP.

#10 Updated by James Turnbull over 4 years ago

Sorry maybe I am missing something but how does it render your whole module catalog “useless”?

#11 Updated by James Turnbull over 4 years ago

  • Status changed from Accepted to Needs Decision
  • Assignee set to Nigel Kersten

Okay looking at this some more and having a play I think we should revert this change. I propose that:

$lsbdistid-$architecture

Should work just the same as:

${lsbdistid}-${architecture}

Nigel?

#12 Updated by Henrik Lindberg over 4 years ago

James Turnbull wrote:

Okay looking at this some more and having a play I think we should revert this change.

I assume you mean not to revert that ‘–’ is allowed in a variable name, and instead make it illegal to have a variable that ends with ‘–’. Is that correct?

I propose that: […]

Should work just the same as:

[…]

What about "$a-b", Is that equivalent to: "${a}-b" or "${a-b}" ?

#13 Updated by R.I. Pienaar over 4 years ago

My vote is to make ‘–’ illegal in variable names. Allowing people to create variables they cannot use in a template without resorting to lookupvar() is not a good feature.

Given the really large number of bugs this feature has caused since the decision was made to officially allow it I think at some point we should concede its a bad, undeliverable, idea

#14 Updated by James Turnbull over 4 years ago

Henrik Lindberg wrote:

James Turnbull wrote:

Okay looking at this some more and having a play I think we should revert this change.

I assume you mean not to revert that ‘–’ is allowed in a variable name, and instead make it illegal to have a variable that ends with ‘–’. Is that correct?

Actually no I lean somewhere towards banning – in variables names or reverting this whole change and starting again with a deprecation warning. We should never have changed the interpolation without warning.

#15 Updated by Nigel Kersten over 4 years ago

R.I. Pienaar wrote:

My vote is to make ‘–’ illegal in variable names. Allowing people to create variables they cannot use in a template without resorting to lookupvar() is not a good feature.

Given the really large number of bugs this feature has caused since the decision was made to officially allow it I think at some point we should concede its a bad, undeliverable, idea

I’m not sure what the most practical deprecation plan is here though.

Do we start throwing deprecation warnings in 2.7.x and remove it in Telly?

I don’t really want this to live on longer than that if possible.

#16 Updated by Anonymous over 4 years ago

Nigel Kersten wrote:

R.I. Pienaar wrote:

My vote is to make ‘–’ illegal in variable names. Allowing people to create variables they cannot use in a template without resorting to lookupvar() is not a good feature.

Given the really large number of bugs this feature has caused since the decision was made to officially allow it I think at some point we should concede its a bad, undeliverable, idea

I’m not sure what the most practical deprecation plan is here though. Do we start throwing deprecation warnings in 2.7.x and remove it in Telly? I don’t really want this to live on longer than that if possible.

Given that this is much worse than the usual “works, but not awesomely” feature that has a longer deprecation cycle, I think this would be appropriate. This is more or less broken right now for people trying to use the product, so getting it gone faster seems solid.

#17 Updated by R.I. Pienaar over 4 years ago

yes, kill as soon as we can really.

We should also reconsider – in class names for similar reasons ${foo-bar::baz}, gross.

#18 Updated by Henrik Lindberg over 4 years ago

R.I. Pienaar wrote:

yes, kill as soon as we can really.

+1 to that, IMO allowing ‘–’ in a name was a mistake. Fix this on 2.7.x asap by making it produce an error to stop this from spreading.

We should also reconsider – in class names for similar reasons ${foo-bar::baz}, gross.

Agree, It must to apply to all named things that may be part of a variable name.

#19 Updated by Brice Figureau over 4 years ago

+1 for killing this in 2.7.x. I don’t think people had really time to use this feature… Do we have the original feature ticket? There was certainly a reason this was added in the first place…

#20 Updated by R.I. Pienaar over 4 years ago

Brice Figureau wrote:

+1 for killing this in 2.7.x. I don’t think people had really time to use this feature… Do we have the original feature ticket? There was certainly a reason this was added in the first place…

The history is that the docs used to say this is not valid but in some cases it did work. I wanted the code to be changed to disable this in #2974 but the choice was made instead to make the –’s work reliably everywhere.

Since it’s never worked well, changing behavior etc so finally I am glad to see it killed completely (hopefully)

#21 Updated by Nigel Kersten over 4 years ago

Henrik Lindberg wrote:

Agree, It must to apply to all named things that may be part of a variable name.

Noting that we do allow it in certnames, but yes, all named things that may be part of a variable name is a good line to draw.

I don’t believe it’s reasonable to throw an error in a 2.7.x release, but a strongly worded deprecation warning seems reasonable.

#22 Updated by Peter Meier over 4 years ago

R.I. Pienaar wrote:

yes, kill as soon as we can really.

+1

We should also reconsider – in class names for similar reasons ${foo-bar::baz}, gross.

If we want to do this, this should be treated as seperate issue. – in class names have been working for a long time and is widely used. So this would definitely require a proper deprecation path.

#23 Updated by Henrik Lindberg over 4 years ago

Nigel Kersten wrote:

I don’t believe it’s reasonable to throw an error in a 2.7.x release, but a strongly worded deprecation warning seems reasonable.

As this needs to be fixed in the lexer (make ‘–’ illegal in variable name token), the result of parsing $a = $b-a + 10 produces a syntax error. See no way to both have the cake and eat it too, a ‘–’ is either included in the variable token, or it is not.

#24 Updated by Henrik Lindberg over 4 years ago

Peter Meier wrote:

We should also reconsider – in class names for similar reasons ${foo-bar::baz}, gross.

If we want to do this, this should be treated as seperate issue. – in class names have been working for a long time and is widely used. So this would definitely require a proper deprecation path.

Since this is the case (‘–’ in classnames are allowed), the proposal to just make an ending ‘–’ illegal in a variable name (and class name) may be a better option. If ‘–’ is allowed in classnames, there is no way to refer to variables in such classes.

Fixing it this way (no ‘–’ at end) may be the lesser of all evils.

#25 Updated by R.I. Pienaar over 4 years ago

‘–’ in class names worked for ever but was documented not to work, I’d prefer to see us go back to a time where they just werent allowed.

If you allow – in class names you are still stuck with $foo-bar::baz and this is going to bite us at some point, I really think we need to make an effort to design away surprises and any vars with – in them is bad

#26 Updated by Nigel Kersten over 4 years ago

Given the bollocks behavior in templates, we are going to get rid of hyphens in class names and variables, and any other object that may be used to compose a variable name.

The decision left to be made is how we go about doing this, not whether we’re doing it.

#27 Updated by Peter Meier over 4 years ago

I’m totally fine to also not allow it anymore in class names. I just wanted to point out that – in classnames is probably way more spread than in variables, as it used to work. Actually I use it all over in my modules and I know a couple of other places, where they will have to change a lot of their exisiting module setup. It’s quite an easy step to move modules/classes from for example site-foo to site_foo, still it’s probably way more the case than the variables. But that’s my feeling. Not allowing it anymore in the next major release is fine.

#28 Updated by Brice Figureau over 4 years ago

Peter Meier wrote:

I’m totally fine to also not allow it anymore in class names. I just wanted to point out that – in classnames is probably way more spread than in variables, as it used to work. Actually I use it all over in my modules and I know a couple of other places, where they will have to change a lot of their exisiting module setup. It’s quite an easy step to move modules/classes from for example site-foo to site_foo, still it’s probably way more the case than the variables. But that’s my feeling. Not allowing it anymore in the next major release is fine.

I concur with Peter. I also do have some modules with ‘–’. It’s not a big deal if I have to rename them, but I bet it’s much more frequent than variable names. I’m in favor of an instant removal of ‘–’ in variable in 2.7 and a deprecation notice for class names.

#29 Updated by Brice Figureau over 4 years ago

Henrik Lindberg wrote:

Peter Meier wrote:

We should also reconsider – in class names for similar reasons ${foo-bar::baz}, gross.

If we want to do this, this should be treated as seperate issue. – in class names have been working for a long time and is widely used. So this would definitely require a proper deprecation path.

Since this is the case (‘–’ in classnames are allowed), the proposal to just make an ending ‘–’ illegal in a variable name (and class name) may be a better option. If ‘–’ is allowed in classnames, there is no way to refer to variables in such classes.

Fixing it this way (no ‘–’ at end) may be the lesser of all evils.

Yes, that was my idea (and 1st comment on this ticket) except it doesn’t really work.

People that are complaining are complaining because the interpolation of “$a-randomstring” doesn’t evaluate to “${a}-randomstring” but “${a-randomstring}”. If you forbid trailing ‘–’ but allow it anywhere else, then “$a-randomstring” will still be seen as “${a-randomstring}”, because a-randomstring becomes a valid variable name.

#30 Updated by Henrik Lindberg over 4 years ago

I also agree to instant removal of ‘–’ in variable names and deprecation of ‘–’ in names that are addressable by variable name segments.

Will revert the support for ‘–’ in variable names in Geppetto asap. and move the refactoring support for variables with hyphens to classes/definitions with hyphens :)

#31 Updated by Paul Tinsley about 4 years ago

I think you kill it with fire, the confusion it causes is bad enough, the fact that it changed in a point release and changed the behavior of interpolation is even worse. People expecting to do a bugfix upgrade of puppet end up with hours of debugging and fixing modules to work around this.

And now this is 3 months in from discovery and people are likely using this “feature” for the first time and now you’ll end up with the opposite problem with killing it. The longer it goes the more people it will affect in the opposite way.

#32 Updated by Anonymous about 4 years ago

  • Status changed from Needs Decision to Accepted

Daniel Pittman and I think that we should backtrack and disallow dashes in variables.

#33 Updated by Nigel Kersten about 4 years ago

  • Assignee deleted (Nigel Kersten)

We’re committing to making this change now in 2.7.x are we?

#34 Updated by Bostjan Skufca almost 4 years ago

How far are we from resolving this and commiting it to 2.7.x?

#35 Updated by James Turnbull almost 4 years ago

  • Priority changed from High to Urgent
  • Target version changed from 2.7.x to 3.x

I just spoke to Engineering and this is now on the backlog for Telly to be fixed.

#36 Updated by Anonymous almost 4 years ago

  • Target version changed from 3.x to 2.7.x
  • Branch set to https://github.com/puppetlabs/puppet/pull/757

https://github.com/puppetlabs/puppet/pull/757 fixes this, for 2.7.x. It enforces the rule that the - is not part of a variable name.

This seems reasonable – our documentation is quite clear that it is not valid, and resolves the problem.

#37 Updated by Moses Mendoza almost 4 years ago

Daniel – https://github.com/puppetlabs/puppet/commit/b26699a2b89f3724b4322a3723d2c700fb7e4dd3, referencing this ticket, was released with 2.7.15rc1. Is the code sufficient to consider this issue closed?

#38 Updated by Anonymous almost 4 years ago

  • Status changed from Accepted to Merged - Pending Release
  • Target version changed from 2.7.x to 2.7.15

Moses Mendoza wrote:

Daniel – https://github.com/puppetlabs/puppet/commit/b26699a2b89f3724b4322a3723d2c700fb7e4dd3, referencing this ticket, was released with 2.7.15rc1. Is the code sufficient to consider this issue closed?

Yes, that bit of admin must have been missed. Thanks for catching the slip.

#39 Updated by Moses Mendoza over 3 years ago

  • Status changed from Merged - Pending Release to Closed

Released in 2.7.15rc1.

#40 Updated by Anonymous over 3 years ago

  • Target version changed from 2.7.15 to 2.7.20

Fixed in 2.7.20

According to the announcement:

Special notes about 2.7.20

Puppet 2.7.20 also addresses concerns regarding a change introduced in Puppet 2.7.16, “b26699a (#10146) - is not legal in variable names.”, which disallowed the use of dashes in variable names. Puppet 2.7.20 introduces a configuration option, allow_variables_with_dashes, (http://docs.puppetlabs.com/references/2.7.latest/configuration.html#allowvariableswithdashes) which can be set to true to restore earlier behavior, although this is strongly discouraged. The option is set to false by default to maintain current behavior.

(#10146) - in variable names should be deprecated!

In commit b26699a2 I fixed an accidentally introduced change to the lexer, allowing - to be part of a variable name. That had lasted for a while and was surprisingly popular. It was also hugely ambiguous around - as minus, and led to all sorts of failures – unexpected interpolations to nothing – because of that.

A much better strategy would have been to deprecate the feature, issue proper warnings, and include an option to allow users to toggle the behaviour.

Initially defaulting that to “permit”, and eventually toggling over to “deny”, would have made the whole experience much smoother – even if this was totally documented as not working, and was a clear bug that it changed.

So, thanks to prompting from Benjamin Irizarry, we do just that: introduce the configuration option, default it to “deny” to match current behaviour, but allow users the choice to change this back.

Please be aware that allowing variables with - might break all sorts of things around, for example, Forge modules though. Most people write code to the documented standard, and their code might well totally fail to work with this changed away from the default!

Also available in: Atom PDF