Page MenuHomePhabricator

Make Puppet repository pass lenient and strict lint checks
Closed, ResolvedPublic

Description

For all changes in the Puppet repository, the lint checks operations-puppet-puppetlint-lenient and puppetlint-strict are run.

Those fail all the time, thus a) they are being ignored for the most part and b) they require developers and reviewers who are interested in them to go to the details page and search for the names of the files that a change touched to see if (new) warnings were added.

The repository should be made compliant with the lint checks or, where those are too severe or, when importing external modules, comparability with upstream is more important than linting, they should be disabled for those cases either in the repository (preferred) or in the Jenkins job definition.

Related Objects

Event Timeline

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

The repository should be made compliant with the lint checks ..

Yes. that. It's just an ungoing and very long task. We have been making tons of lint changes over the last couple years to get closer to that goal. The purpose is to be able to finally pass those checks and make them vote in the glorious future. We are just not there yet. I have stats though how much better we got when it comes to ratio between code lines and warnings. So don't give up on this, don't remove them, it just takes time.

gerritbot added a subscriber: gerritbot.

Change 188206 had a related patch set uploaded (by Hashar):
fix another 28 puppet linter warnings

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

Patch-For-Review

I filled a task to propose a bump of puppet-lint from 0.3.2 (as provided by debian packages) to 1.1.0 (provided by latest ruby gem): T88430.

Change 188206 merged by Alexandros Kosiaris:
fix another 28 puppet linter warnings

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

Faidon has backported puppet-lint 1.1.0 from Ubuntu Vivid in our apt for both Precise and Trusty. I am upgrading puppet-lint on the CI slaves.

Change 188375 had a related patch set uploaded (by Hashar):
Move puppet-lint options to .puppet-lint.rc

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

Patch-For-Review

Change 188805 had a related patch set uploaded (by Hashar):
puppet-lint: ignore some var in single quoted strings

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

Patch-For-Review

https://gerrit.wikimedia.org/r/188375 brings it a bunch of ignore we had in the rakefile and expose them in .puppet-lint.rc. That let us discards a lot of warnings and errors.

https://gerrit.wikimedia.org/r/188805 fix the five remaining false positive errors and make the job operations-puppet-puppetlint-lenient to pass for the first time ever (i.e. no error reported).

Thank you for waiting on a proper .deb package :) I've approved those patches but will wait until Antoine is back @ work before merging.

@Andrew All CI slaves are now using puppet-lint 1.1.0 :-] So unless my patches have a weird side effect on prod they are good to land :-]

Andrew triaged this task as Medium priority.

Change 188375 merged by Andrew Bogott:
Move puppet-lint options to .puppet-lint.rc

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

Change 188805 merged by Andrew Bogott:
puppet-lint: ignore some var in single quoted strings

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

With the operations/puppet.git patches merged above, puppet-lint v1.1.0 no more reports any error. Thus, the Jenkins job operations-puppet-puppetlint-lenient is passing!

It is up to SRE team to now have the Job result to vote -1 on error. Can be done by removing the following lines from integration/config.git zuul/layout.yaml

- name: operations-puppet-puppetlint-lenient
  voting: false

Then +2 and deploy the Zuul configuration change.

I have pointed the ops list to this comment.

Nicely done, merci!

If Jenkins would vote -1, would that reject merges? I. e., if in an emergency SRE needs to merge a change even if it doesn't lint, would they be able to do that?

Nicely done, merci!

If Jenkins would vote -1, would that reject merges? I. e., if in an emergency SRE needs to merge a change even if it doesn't lint, would they be able to do that?

Yes they would be able. Just have to drop jenkins-bot from the list of reviewers, vote Verified +2 and hit "submit patch". Additionally on jenkins-bot never merge patches on ops/puppet , it is always a human action.

This comment was removed by Matanya.

Change 189589 had a related patch set uploaded (by Matanya):
zuul: Make jenkins vote on patches not passing lint check

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

Patch-For-Review

Change 189589 had a related patch set uploaded (by Krinkle):
zuul: Make operations-puppet-puppetlint-lenient voting

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

Patch-For-Review

Change 189589 merged by jenkins-bot:
zuul: Make operations-puppet-puppetlint-lenient voting

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

The Jenkins job (puppet-lint lenient) now complains whenever a puppet-lint error is detected. Huge thanks to everyone that made this possible.

Still have to fix up or ignore the warnings though so leaving this task open.

Change 197759 had a related patch set uploaded (by Tim Landscheidt):
WIP: Mute warnings about case statements without default matches

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

Change 198116 had a related patch set uploaded (by Tim Landscheidt):
Ignore warnings about URLs without modules for private repository

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

Change 198170 had a related patch set uploaded (by Tim Landscheidt):
Disable class_inherits_from_params_class puppet-lint checks

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

we fixed all remaining tab errors and enabled checks for it

https://gerrit.wikimedia.org/r/#/c/198814/

Change 197759 merged by Dzahn:
Ignore some warnings about case statements without default matches

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

Change 198170 merged by Dzahn:
Disable class_inherits_from_params_class puppet-lint checks

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

Change 201884 had a related patch set uploaded (by Tim Landscheidt):
apache: Mute warnings about right-to-left relationships

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

Change 201884 merged by Dzahn:
apache: Mute warnings about right-to-left relationships

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

current status: puppet lenient is voting, strict isn't yet. Is it worth to chase strict warnings too?

I think as well that strict lint checks should be enforced, and as https://integration.wikimedia.org/ci/job/operations-puppet-puppetlint-strict/24810/console shows the only major issue left is "puppet:// URL without modules/ found (puppet_url_without_modules)" (cf. https://gerrit.wikimedia.org/r/#/c/198116/ for a change (to be rebased) to fix that; another approach would be to ignore the warning globally).

Change 226459 had a related patch set uploaded (by Tim Landscheidt):
cassandra: Fix strict puppet-lint warnings

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

Change 226463 had a related patch set uploaded (by Tim Landscheidt):
statsdlb: Fix strict puppet-lint check

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

Change 226459 merged by Yuvipanda:
cassandra: Fix strict puppet-lint warnings

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

Change 198116 merged by Dzahn:
Ignore warnings about URLs without modules for private repository

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

From a recent puppet lint strict run, seems the left over is migrating legacy manifests to puppet modules.

Change 228682 had a related patch set uploaded (by Tim Landscheidt):
Ignore warnings about URLs without modules for volatile directory

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

Change 228712 had a related patch set uploaded (by Tim Landscheidt):
haproxy: Move check_haproxy to module itself

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

Change 233073 had a related patch set uploaded (by Tim Landscheidt):
cassandra: Mute strict puppet-lint warnings

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

Change 233073 merged by Alexandros Kosiaris:
cassandra: Mute strict puppet-lint warnings

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

Change 228712 merged by Yuvipanda:
haproxy: Move check_haproxy to module itself

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

Change 228682 merged by Dzahn:
Ignore warnings about URLs without modules for volatile directory

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

Change 226463 merged by Dzahn:
statsdlb: Fix strict puppet-lint check

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

I would like us to just disable puppet_url_without_modules-check for now, and then make the strict test voting. That will lock in all the lint fixes that we've made heretofore.

If that happens we can create a new task about turning that test back on and fixing violations... if we care :)

Change 243177 had a related patch set uploaded (by Andrew Bogott):
puppet-lint: Turn on --no-puppet_url_without_modules-check

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

I would like us to just disable puppet_url_without_modules-check for now, and then make the strict test voting. That will lock in all the lint fixes that we've made heretofore.

If that happens we can create a new task about turning that test back on and fixing violations... if we care :)

Sounds good to me

Change 243185 had a related patch set uploaded (by Hashar):
operations/puppet now has strict puppet-lint

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

Change 243177 merged by Andrew Bogott:
puppet-lint: Turn on --no-puppet_url_without_modules-check

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

Change 243185 merged by jenkins-bot:
operations/puppet now has strict puppet-lint

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

CI is now voting on puppet-lint error/warnings. Seems all the repository has been cleaned up :-}

Change 243803 had a related patch set uploaded (by Dzahn):
lint: re-enable 'variable not enclosed' check

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

Change 243803 merged by Dzahn:
lint: re-enable 'variable not enclosed' check

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

it seems like this is working now...but the blocked by tasks are not closed?

chasemp claimed this task.

well played @scfc

(Prior to c41b180660e96270e9d90cd66634caa90e0e7029, --no-puppet_url_without_modules-check was not enabled, so the blocking tasks made sense then. I have removed them as blockers; they are still valid in their own right.)