search

rodjek / puppet-lint

Check that your Puppet manifests conform to the style guide
MIT License
819 stars 204 forks source link

2.4.x regression with puppet-lint-legacy_facts-check #902

Closed alexjfisher closed 4 years ago

alexjfisher commented 5 years ago

With 2.3.6 

location => "http://repo.proxysql.com/ProxySQL/proxysql-1.4.x/${facts['lsbdistcodename']}/",

(from https://github.com/voxpupuli/puppet-proxysql/blob/c5fc8dfbce1d78b93613278d3da5742f318dd281/manifests/params.pp#L40) fails the legacy facts check, but since 2.4.0 is no longer detected.

Looks like that line is being tokenised differently now?? <Token :VARIABLE (facts['lsbdistcodename']) @40:72> vs <Token :VARIABLE (facts) @40:73>

alexjfisher commented 5 years ago

git bisect reveals https://github.com/rodjek/puppet-lint/pull/846/commits/8da7c7c232b4a25f5158a61b3bb7d29eebbcbeea (from https://github.com/rodjek/puppet-lint/pull/846) to be the commit that broke this.

usev6 commented 5 years ago

If I'm not mistaken the new way to tokenise is wanted.

A dedicated test for tokenising ${foo[bar][baz]} inside an interpolated string was changed from expecting one token <Token :VARIABLE (foo[bar][baz]) ...> to [expecting multiple tokens <Token :VARIABLE (foo) ...> <Token :LBRACK ([) ...> <Token :NAME (bar) ...> and so forth](https://github.com/rodjek/puppet-lint/blob/8f66ab66ed/spec/puppet-lint/lexer_spec.rb#L334-L405).

That happened in 8da7c7c (the commit you found with git bisect).

@rodjek will probably comment on this, but to me it looks like the plugin puppet-lint-legacy_facts-check needs to be changed.

rodjek commented 5 years ago

Yeah, the old pre 2.4.x method of tokenising double quoted strings was broken and not consistent with how they were tokenised outside of string interpolation. It is a significant change though so I'll submit a PR to that plugin to fix it up

mmckinst commented 4 years ago

What needs to be changed in puppet-lint-legacy_facts-check to fix this?

bastelfreak commented 4 years ago

@rodjek di you have an advise for @mmckinst ?

rodjek commented 4 years ago

Sorry about the delay, I've opened mmckinst/puppet-lint-legacy_facts-check#32 to address this

rodjek commented 4 years ago

PR merged into puppet-lint-legacy_facts-check and released