feat(pre-commit): add pre-commit

[dafyddj]

Making a start on this.

Think rolling this out is best done in stages:

• keep yamllint files and ignores in .yamllint
• roll out to all formulas
• remove standalone lint jobs
• move yamllint files and ignores from .yamllint to .pre-commit-config.yaml; and
  • implement shellcheck.exclude_paths in .pre-commit-config.yaml

Things to resolve in this PR:

• start formula by formula?
• handle use_single_job_for_linters: false
• what about docs?

[pull-assistant]

Best reviewed: commit by commit

---

Optimal code review plan

     feat(pre-commit): add pre-commit

Powered by Pull Assistant. Last update 796fb1e ... 796fb1e. Read the comment docs.

[dafyddj]

@myii ready for further testing!

[myii]

@dafyddj In progress: https://saltstack-formulas.zulipchat.com/#narrow/stream/239693-CI/topic/pre-commit.2F2020-W39_02.

Already completed the first one (apache-formula) which failed on the rstcheck. Decided to comment that out for the time being, so that we use the same linters on both sides. Then reran it and it passed (showing as fixed), so I think that's a better way to go through the whole batch. So now running across all formulas.

[myii]

@dafyddj After starting that run, I realised we could get even better results by only running the rubocop linter when it isn't configured as a separate job (allowed to fail). Also, noticed some yamllint failures being caused in .travis.yml due to the Zulip notifications section. Fixed both of those and now I'm expecting this run to be the most useful to compare between the lint jobs:

• https://saltstack-formulas.zulipchat.com/#narrow/stream/239693-CI/topic/pre-commit.2F2020-W39_03

[myii]

@dafyddj Working through the four failures:

Formula	Error	Situation
devstack	link	Fails yamllint, should ignore test/**/states/**/*.sls
eclipse	link	Fails shellcheck, should only run across certain files (i.e. git ls-files -- '*.sh' '*.bash' '*.ksh' | xargs shellcheck)
lynis	link	Fails yamllint, should ignore test/**/states/**/*.sls
postfix	link	Fails yamllint, should ignore test/**/states/**/*.sls

So just the two types of problems to resolve.

[dafyddj]

saltstack-formulas/postfix-formula#115
saltstack-formulas/lynis-formula#10
saltstack-formulas/eclipse-formula#35
saltstack-formulas/devstack-formula#48

@myii fixes for the failures - pre-commit is actually correct here, these are linting failures to be fixed

[myii]

@dafyddj Well, that's embarrassing! I mixed up the two blocks when evaluating the situation and writing the last message:

ssf-formula/ssf/defaults.yaml

Lines 218 to 222 in c259684

	ignore:
	default:
	- 'node_modules/'
	additional_ssf:
	- 'test/**/states/**/*.sls'

ssf-formula/ssf/defaults.yaml

Lines 231 to 233 in c259684

	additional_ssf:
	- '*.example'
	- 'test/**/*.sls'

So another victory for pre-commit! I suppose our current yamllint is having some issue with the Python 2 version that's being installed.

---

Thanks for pushing through all of the fixes and it's nice to see them merged so quickly. I did notice an issue with the lynis-formula fix, so I pushed another commit, as mentioned here.

---

So that leaves us with our shellcheck linter in the eclipse-formula. The rationale behind restricting the files checked (i.e. git ls-files -- '*.sh' '*.bash' '*.ksh' | xargs shellcheck) is that I came across some formulas where shell files were being used as file templates, containing Jinja in them at times. I didn't fancy having the linter messing with these templates and so restricted to the actual shell files executed by the formula itself. So how do you feel about doing the same with pre-commit?

[dafyddj]

To summarize our discussion, for now we will stick to the status quo of shellchecking '*.sh' '*.bash' '*.ksh'. In the future I would like to consider checking files without extension (i.e. pre-commit option types: [shell], excluding zsh and possibly '*.shell') so that we can check commands such as bin\install-hooks.

[myii]

To summarize our discussion, for now we will stick to the status quo of shellchecking '*.sh' '*.bash' '*.ksh'. In the future I would like to consider checking files without extension (i.e. pre-commit option types: [shell], excluding zsh and possibly '*.shell') so that we can check commands such as bin\install-hooks.

@dafyddj All fixed!

• https://saltstack-formulas.zulipchat.com/#narrow/stream/239693-CI/topic/pre-commit.2F2020-W40_01

Now to rerun everything with rstcheck enabled and then push quick fixes to those formulas (hopefully). Will be appearing at the following location soon:

• https://saltstack-formulas.zulipchat.com/#narrow/stream/239693-CI/topic/pre-commit.2F2020-W40_02

[myii]

@dafyddj The rstcheck is complete across all formulas at https://saltstack-formulas.zulipchat.com/#narrow/stream/239693-CI/topic/pre-commit.2F2020-W40_02. I count 21 formulas with failures there.

I haven't had a chance to look through in detail but there are some unexpected violations, which we may need to adjust for (probably fairly simple to fix in the files themselves, though):

• https://travis-ci.org/github/myii/deepsea-formula/builds/731606296#L252-L253
• https://travis-ci.org/github/myii/libvirt-formula/builds/731607315#L252
  • Maybe we need to just ignore the changelog for rstcheck.
  • https://github.com/saltstack-formulas/libvirt-formula/blob/master/docs/CHANGELOG.rst#321-2019-10-14 (search for ": do not prefix reader method names with get_").
  • Underlying commit message: saltstack-formulas/libvirt-formula@e7ced7b.
• https://travis-ci.org/github/myii/nut-formula/builds/731607874#L259
• https://travis-ci.org/github/myii/varnish-formula/builds/731609027#L259
  • This is the old TOFS document but we can't simply update to the new one, since we're not using the updated module there yet.

[myii]

This PR is held up by #261, which shouldn't take too long to resolve.

[dafyddj]

Do we need to add docs for this?

[myii]

Do we need to add docs for this?

Do you mean across the formulas? That's not going to be easy, since we're not currently managing the docs/README.rst file. I've had some thoughts about how to approach this but nothing satisfactory so far. My suggestion is to push this out now so at least the pre-commit linting is being done in Travis. Then we can tackle the remaining situations.

As for this PR, I've got a commit to add to it, to finalise the structure.

[dafyddj]

I didn't realise that CONTRIBUTING.rst is in another repo. That's where I meant.

[myii]

I didn't realise that CONTRIBUTING.rst is in another repo. That's where I meant.

@dafyddj Sure, that file is managed from this repo, so please go ahead and update this PR with the necessary additions.

[myii]

@dafyddj All changes pushed across formulas (finally). A couple of things remain:

1. Please rebase this PR on top of feat(formulas): capture recent changes across formulas #261, so that I can push the finalising commit (that I used locally).
2. The documentation change you've made will only end up being seen in the main contributing guidelines, not every formula. So some of the wording probably needs to be adjusted, I'll comment inline to be a bit clearer.

[myii]

We got there in the end! Thanks for providing this PR, it really helped to get this pre-commit out there.

[myii]

🎉 This PR is included in version 1.213.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀