Page MenuHomePhabricator

Resource attributes are quoted inconsistently
Open, LowestPublic

Description

The Puppet coding style requires inter alia:

  • Must single quote all resource names and their attribute (unless they contain a variable, of course).

This grossly clashes with the status quo:

[tim@passepartout ~/src/operations/puppet]$ git grep "ensure *=> *\([a-z]\+\)" | wc -l
1378
[tim@passepartout ~/src/operations/puppet]$ git grep "ensure *=> *\('[a-z]\+'\)" | wc -l
465
[tim@passepartout ~/src/operations/puppet]$

Before I submit a patch to change 1378 lines :-) (or rather multiple patches), I'd like to make sure that there is consensus that it should indeed be written:

ensure => 'present',

Also, there are 48 occurences of ensure => installed/ensure => 'installed'. installed is equivalent to present, and I like to replace installed with present (or rather 'present') for consistency as well.

Details

SubjectRepoBranchLines +/-
operations/puppetproduction+126 -126
operations/puppetproduction+29 -24
operations/puppetproduction+23 -23
operations/puppetproduction+22 -22
operations/puppetproduction+5 -5
operations/puppetproduction+13 -12
operations/puppetproduction+36 -35
operations/puppetproduction+8 -8
operations/puppetproduction+9 -11
operations/puppetproduction+16 -15
operations/puppetproduction+15 -15
operations/puppetproduction+2 -2
operations/puppetproduction+7 -5
operations/puppetproduction+3 -3
operations/puppetproduction+4 -4
operations/puppetproduction+9 -9
operations/puppetproduction+5 -5
operations/puppetproduction+45 -45
operations/puppetproduction+14 -14
operations/puppetproduction+1 -1
operations/puppetproduction+28 -28
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

IIRC puppet-lint does not complain about it which could explain why we are not forcing it. My take is feel free to submit patches (note the plural) and change the "installed" to "present". I 'll be happy to review

Change 195599 had a related patch set uploaded (by Matanya):
librenms: Resource attributes quoting

https://gerrit.wikimedia.org/r/195599

Change 195601 had a related patch set uploaded (by Matanya):
clamav: Resource attributes quoting

https://gerrit.wikimedia.org/r/195601

Change 195607 had a related patch set uploaded (by Matanya):
swift_new: lint and resource quoting

https://gerrit.wikimedia.org/r/195607

Change 195613 had a related patch set uploaded (by Matanya):
puppetception: Resource attributes quoting

https://gerrit.wikimedia.org/r/195613

Change 195616 had a related patch set uploaded (by Matanya):
limn: minor lint and Resource attributes quoting

https://gerrit.wikimedia.org/r/195616

Change 195627 had a related patch set uploaded (by Matanya):
dynamicproxy: resource attributes quotin

https://gerrit.wikimedia.org/r/195627

It would be nice to have this checked in gerrit (voting) after the changes are done.

Change 195644 had a related patch set uploaded (by Matanya):
diamond: resource attributes quoting

https://gerrit.wikimedia.org/r/195644

Change 195650 had a related patch set uploaded (by Matanya):
bastionhost: resource attributes quoting

https://gerrit.wikimedia.org/r/195650

Change 195652 had a related patch set uploaded (by Matanya):
haproxy: resource attributes quoting

https://gerrit.wikimedia.org/r/195652

Change 195658 had a related patch set uploaded (by Matanya):
reprepro: resource attributes quoting

https://gerrit.wikimedia.org/r/195658

Change 195660 had a related patch set uploaded (by Matanya):
puppet_compiler: resource attributes quoting and minor lints

https://gerrit.wikimedia.org/r/195660

Change 195661 had a related patch set uploaded (by Matanya):
backup: resource attributes quoting

https://gerrit.wikimedia.org/r/195661

Change 195665 had a related patch set uploaded (by Matanya):
locales: resource attributes quoting

https://gerrit.wikimedia.org/r/195665

Change 195665 had a related patch set uploaded (by Krinkle):
locales: resource attributes quoting

https://gerrit.wikimedia.org/r/195665

Change 195680 had a related patch set uploaded (by Matanya):
scap: lint and resource attributes quoting

https://gerrit.wikimedia.org/r/195680

Change 195743 had a related patch set uploaded (by Matanya):
extdist: resource attributes quoting

https://gerrit.wikimedia.org/r/195743

Change 195764 had a related patch set uploaded (by Matanya):
labs_vmbuilder: resource attributes quoting

https://gerrit.wikimedia.org/r/195764

Change 195769 had a related patch set uploaded (by Matanya):
zuul: lint resource attributes quoting

https://gerrit.wikimedia.org/r/195769

Change 195770 had a related patch set uploaded (by Matanya):
motd: resource attributes quoting

https://gerrit.wikimedia.org/r/195770

Change 195857 had a related patch set uploaded (by Matanya):
memcached: resource attributes quoting

https://gerrit.wikimedia.org/r/195857

Change 195858 had a related patch set uploaded (by Matanya):
ferm: resource attributes quoting

https://gerrit.wikimedia.org/r/195858

Change 195859 had a related patch set uploaded (by Matanya):
etherpad: resource attributes quoting

https://gerrit.wikimedia.org/r/195859

Change 195860 had a related patch set uploaded (by Matanya):
ocg: resource attributes quoting

https://gerrit.wikimedia.org/r/195860

Change 195862 had a related patch set uploaded (by Matanya):
shinken: resource attributes quoting

https://gerrit.wikimedia.org/r/195862

Change 195863 had a related patch set uploaded (by Matanya):
racktables: resource attributes quoting

https://gerrit.wikimedia.org/r/195863

Change 195864 had a related patch set uploaded (by Matanya):
wikimania_scholarships: resource attributes quoting and minor lint

https://gerrit.wikimedia.org/r/195864

Change 195866 had a related patch set uploaded (by Matanya):
wikistats: resource attributes quoting

https://gerrit.wikimedia.org/r/195866

Change 195874 had a related patch set uploaded (by Matanya):
logstash: resource attributes quoting

https://gerrit.wikimedia.org/r/195874

I have just noticed that we have this in our puppet guidelines, but my personal "standard" is as follows:

  • never quote what is going to be transformed in ruby barewords, like the arguments of ensure
  • never quote native types, so integers or booleans
  • single-quote everything else unless you interpolate variables
  • double-quote interpolated strings

I do prefer octals to be unquoted, but I conformed to what puppetlabs mandated over time.

So my proposal is like follows

file { 'foo':
  ensure  => directory,
  path      => "${base_dir}/foo",
  mode    =>  '0755',
  owner    => 'root',
  recurse  => true
}

And I think we should change our puppet coding standards accordingly

Up to now, I mostly follow this as well, though I do align arrows :P

  • I am fine with ensure => 'present' or ensure => present although my tendency is ensure => present but that's a habit. Technically 'present' is more correct and I lean towards it these days but I am not really strong on that (or I would have changed the entire tree already)
  • I am trying to be careful always of weird cases with booleans

Booleans sometimes in our code are not booleans but strings. And there are bad checks around in our code that could result in a change of behaviour if the type is changed. Examples like:

$a = 'false'
$b = 'true'
if $a { # Yes it will evaluate to true }
if $a == 'false' { # gues what will happen if $a gets changed to false or true }
if $b == 'false' { # etc etc}

need to be carefully considered before being changed. This happens mostly in class parameters. And they do have the tendency in our code to trickle down to definition parameters that trickle down to templates.

  • By the way we used to have some code where file mode attributes are 5 digits. Like
mode => '00755'

which @Matanya thankfully fixed.

I tend to like unquoted parameters to ensure =>, or to generalized what joe said about not quoting ruby barewords.

For boolean, I remember at least one occurrence of a 'true' string being replaced by a boolean but the puppet class eating it just fine. As Alexandros said, should be done carefuly on a case by case basis. Unfortunately, it does not seem puppet class supports type hinting :(

So can we agree on using

ensure => present

and

ensure => directory

?

As for the "more correct" nature of using quoted values... whatever is legitimate in a language is by its definition also correct, right?

In T91908#1112282, @Joe wrote:

So can we agree on using

ensure => present

and

ensure => directory

?

+1

As for the "more correct" nature of using quoted values... whatever is legitimate in a language is by its definition also correct, right?

Lets not go down this road.

So can we declare this consensus ? If so, i'll add this to the style guide, and fix accordingly.

yuvipanda subscribed.

So can we declare this consensus ? If so, i'll add this to the style guide, and fix accordingly.

Joe on IRC said OK, mark +2ed already, I am OK as well. Seems like we got consensus of anyone that cared to have an opinion.

Change 195874 abandoned by Matanya:
logstash: resource attributes quoting

Reason:
consensus is in the other direction

https://gerrit.wikimedia.org/r/195874

Change 195863 abandoned by Matanya:
racktables: resource attributes quoting

Reason:
consensus is in the other direction.

https://gerrit.wikimedia.org/r/195863

Change 195862 abandoned by Matanya:
shinken: resource attributes quoting

Reason:
consensus was in the other diretion

https://gerrit.wikimedia.org/r/195862

Change 195860 abandoned by Matanya:
ocg: resource attributes quoting

Reason:
consensus was in the other direction.

https://gerrit.wikimedia.org/r/195860

Change 195857 abandoned by Matanya:
memcached: resource attributes quoting

Reason:
consensus is in the other direction.

https://gerrit.wikimedia.org/r/195857

Change 195770 abandoned by Matanya:
motd: resource attributes quoting

Reason:
consensus is in the other direction.

https://gerrit.wikimedia.org/r/195770

Change 195665 abandoned by Matanya:
locales: resource attributes quoting

Reason:
consensus is in the other direction.

https://gerrit.wikimedia.org/r/195665

Change 195650 abandoned by Matanya:
bastionhost: resource attributes quoting

Reason:
consensus is in the other direction.

https://gerrit.wikimedia.org/r/195650

Change 195644 abandoned by Matanya:
diamond: resource attributes quoting

Reason:
consensus is in the other direction

https://gerrit.wikimedia.org/r/195644

Change 195601 abandoned by Matanya:
clamav: Resource attributes quoting

Reason:
consensus was in the other direction.

https://gerrit.wikimedia.org/r/195601

Change 195599 abandoned by Matanya:
librenms: Resource attributes quoting

Reason:
consensus was in the other direction.

https://gerrit.wikimedia.org/r/195599

Change 195864 merged by Dzahn:
wikimania_scholarships: resource attributes quoting and minor lint

https://gerrit.wikimedia.org/r/195864

@Matanya, as you wrote most of the style guide, do you want to update it accordingly?

Yes, i'll do it during the weekend.

Change 195764 merged by Andrew Bogott:
labs_vmbuilder: resource attributes quoting

https://gerrit.wikimedia.org/r/195764

Change 195607 merged by Filippo Giunchedi:
swift_new: lint and resource quoting

https://gerrit.wikimedia.org/r/195607

Change 195661 merged by Andrew Bogott:
backup: resource attributes quoting

https://gerrit.wikimedia.org/r/195661

is there a way we could make the style enforced by tools? e.g. having this failing puppet-lint would make things so much easier

Filed upstream; if someone is interested in implementing, a developer's tutorial is here.

Change 195627 merged by Andrew Bogott:
dynamicproxy: resource attributes quote

https://gerrit.wikimedia.org/r/195627

Change 195858 merged by Andrew Bogott:
ferm: resource attributes quoting

https://gerrit.wikimedia.org/r/195858

Change 195660 merged by Andrew Bogott:
puppet_compiler: resource attributes quoting and minor lints

https://gerrit.wikimedia.org/r/195660

Filed upstream; if someone is interested in implementing, a developer's tutorial is here.

Upstream rejected that; filed T95377 here.

Change 195616 merged by Dzahn:
limn: minor lint and Resource attributes quoting

https://gerrit.wikimedia.org/r/195616

fgiunchedi lowered the priority of this task from Low to Lowest.Jul 21 2015, 10:37 AM

@jbond and all. I wonder what you would think about this now in 2022.

Are the barewords (ensure => link,) good and the single quotes bad as it says in T95377 or is it the other way around and it _SHOULD_ be single quotes as it says on this ticket?

Or... does the fact that we haven't done it since 2015 just show that it doesn't matter to us much?

The only thing that changed since 2015 is the numbers are now higher :)

  ~/repos/puppet$ git grep "ensure *=> *\([a-z]\+\)" | wc -l
3027
 ~/repos/puppet$ git grep "ensure *=> *\('[a-z]\+'\)" | wc -l
1107

@Dzahn this is what i wrote on T95377#8435033

@jbond and all. I wonder what you would think about this now in 2022.

Are the barewords (ensure => link,) good and the single quotes bad as it says on this ticket or is it the other way around and it _SHOULD_ be single quotes as it says on T91908?

In general i think strings should be quoted with '' and interpolated strings should be quoted with double quotes. however id say the ensure parameter is the exception as it allows syntax highlighters to detect and highlight them, thus providing a quick way to pick up simple errors like typos.

Or... does the fact that we haven't done it since 2015 just show that it doesn't matter to us much?

Using a path for the ensure parameter was deprecated in 4.3 so we could create a plug-in for that or send a patch to https://github.com/voxpupuli/puppet-lint-file_ensure-check but im not sure its worth it and is checked by puppet-lint

note this is also inline with the upstream style guide:

If a string is a value from an enumerable set of options, such as present and absent, it SHOULD NOT be enclosed in quotes at all.

https://www.puppet.com/docs/puppet/8/style_guide.html#quoting

It also seems that that was the consensus reached in this ticket, from the discussion starting T91908#1109218

If a string is a value from an enumerable set of options, such as present and absent, it SHOULD NOT be enclosed in quotes at all.

Thank you, John! Uploading a couple changes to reduce the number of quoted values like that.

It seemed a bit much to link every single change to this ticket, but then also,, I wanted to somehow link them.

So here it goes as a single link to a topic branch:

https://gerrit.wikimedia.org/r/q/topic:T91908

wikistats: https://gerrit.wikimedia.org/r/c/operations/puppet/+/934640
microsites: https://gerrit.wikimedia.org/r/c/operations/puppet/+/934637
vrts: https://gerrit.wikimedia.org/r/c/operations/puppet/+/934641
releases: https://gerrit.wikimedia.org/r/c/operations/puppet/+/934642
gerrit: https://gerrit.wikimedia.org/r/c/operations/puppet/+/934638
phabricator: https://gerrit.wikimedia.org/r/c/operations/puppet/+/934639

for the sre-serviceops-collab services

edit: these are all merged. fixed about 80 cases of wrong quoting