Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: merge travis matrix, add salt-lint & rubocop to lint job #175

Conversation

myii
Copy link
Member

@myii myii commented Oct 8, 2019


Essentially, this PR is quite simple:

  1. Convert the lint stage into a job that is run concurrently with the rest of the matrix.
  2. Add salt-lint to the lint job.
  3. Add rubocop to the lint job.

The details are easier to explain commit-by-commit.


refactor(travis): merge lint stage into the test stage

  • Nothing new here, just a pure refactor to merge the lint stage into the test stage matrix.
  • Even when we first introduced the lint stage, there were ideas to run it first, to get the quickest feedback to PR authors and maintainers.
  • With the merge into the matrix and making it the first job, we get the best of both worlds.
  • https://travis-ci.com/saltstack-formulas/template-formula/builds/130839797.

ci(travis): run salt-lint during the Lint job


ci(travis): run rubocop during the Lint job

  • Useful to lint our Ruby files as well (viz. InSpec).

fix(rubocop): add fixes using rubocop --safe-auto-correct

  • First set of fixes automated by rubocop.

fix(rubocop): fix remaining errors manually

  • Remaining fixes made manually.

@myii myii force-pushed the ci/merge-matrix-and-add-salt-lint-and-rubocop branch 2 times, most recently from 2ae6519 to 7d77cd2 Compare October 8, 2019 02:38
Copy link
Contributor

@baby-gnu baby-gnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, it was really painful to wait so long to see a lint problem.

Thanks.

.travis.yml Outdated Show resolved Hide resolved
myii added a commit to myii/template-formula that referenced this pull request Oct 8, 2019
@myii myii force-pushed the ci/merge-matrix-and-add-salt-lint-and-rubocop branch from 7d77cd2 to 88fbddb Compare October 8, 2019 15:25
@myii myii requested a review from dafyddj October 8, 2019 15:27
@myii myii force-pushed the ci/merge-matrix-and-add-salt-lint-and-rubocop branch from 88fbddb to 9359bad Compare October 8, 2019 15:32
myii added a commit to myii/template-formula that referenced this pull request Oct 8, 2019
.travis.yml Outdated Show resolved Hide resolved
myii added a commit to myii/template-formula that referenced this pull request Oct 8, 2019
@myii myii force-pushed the ci/merge-matrix-and-add-salt-lint-and-rubocop branch from 8055b60 to 36a8e31 Compare October 8, 2019 17:03
@myii myii force-pushed the ci/merge-matrix-and-add-salt-lint-and-rubocop branch from 36a8e31 to 5f773d1 Compare October 8, 2019 19:15
@myii
Copy link
Member Author

myii commented Oct 8, 2019

So an update: I've run this against every formula using the ssf-formula. They've all tripped up for one reason or another. Every... single... one! I was expecting that but to make this process a bit more palatable, I'm going to split each linter into a separate job, so it's easier to find what each formula is failing on. Then it will be easier to make a plan about how to tackle these lint issues.


@n-rodriguez On a separate note, any feedback about the Ruby issue I asked above?

@myii
Copy link
Member Author

myii commented Oct 8, 2019

Yes, that's better. Most are failing both but some are at least passing salt-lint, e.g.:

Seems like most of the lint fixes are going to be for rubocop.

@myii myii merged commit 9436192 into saltstack-formulas:develop Oct 8, 2019
@myii myii deleted the ci/merge-matrix-and-add-salt-lint-and-rubocop branch October 8, 2019 21:51
@myii
Copy link
Member Author

myii commented Oct 8, 2019

OK, using allow_failures will help get around this problem:

So there will be two types of .travis.yml that will be propagated:

  1. The ideal: all of the linters in one job, like this PR.
  2. Second job: Have a separate job for rubocop, which will use allow_failures.

I can do that above using ssf-formula, probably using the TOFS feature.

We should insist that salt-lint passes for every run, so I'll do that with each PR that's created from this process.

@daks @baby-gnu @dafyddj @aboe76 Thanks for taking out time to review this.

saltstack-formulas-travis pushed a commit that referenced this pull request Oct 8, 2019
## [3.3.2](v3.3.1...v3.3.2) (2019-10-08)

### Bug Fixes

* **rubocop:** add fixes using `rubocop --safe-auto-correct` ([484ce24](484ce24))
* **rubocop:** fix remaining errors manually ([9566b6f](9566b6f))

### Code Refactoring

* **travis:** merge `lint` stage into the `test` stage ([d3b93f8](d3b93f8))

### Continuous Integration

* **kitchen:** install required packages to bootstrapped `opensuse` [skip ci] ([1cfed60](1cfed60))
* **kitchen:** use bootstrapped `opensuse` images until `2019.2.2` [skip ci] ([0467bdf](0467bdf))
* **travis:** quote `${INSTANCE}` when running `kitchen verify` ([00d56a4](00d56a4)), closes [/github.com//pull/175#discussion_r332525964](https://github.com//github.com/saltstack-formulas/template-formula/pull/175/issues/discussion_r332525964)
* **travis:** run `rubocop` during the `Lint` job ([8d8c766](8d8c766))
* **travis:** run `salt-lint` during the `Lint` job ([2df4646](2df4646)), closes [/freenode.logbot.info/saltstack-formulas/20191004#c2723464](https://github.com//freenode.logbot.info/saltstack-formulas/20191004/issues/c2723464) [/freenode.logbot.info/saltstack-formulas/20191004#c2724272](https://github.com//freenode.logbot.info/saltstack-formulas/20191004/issues/c2724272)
* **travis:** use `env` and `name` for improved display in Travis ([5f773d1](5f773d1)), closes [/github.com//pull/175#discussion_r332613933](https://github.com//github.com/saltstack-formulas/template-formula/pull/175/issues/discussion_r332613933)

### Documentation

* **bug_report:** add section requesting commit hash / release tag ([faccb6a](faccb6a))
* **bug_report:** group into sections for better logical ordering ([e9b6c2f](e9b6c2f))
* **contributing:** add recent `semantic-release` formula ([c2924b0](c2924b0))
* **contributing:** add recent `semantic-release` formula ([8d2318c](8d2318c))
* **contributing:** add recent `semantic-release` formula [skip ci] ([85118de](85118de))
* **issues:** provide `Bug report` & `Feature request` templates ([f90f1f6](f90f1f6))
* **issues:** use `Meta` instead of `Optional` as suggested ([65cadb4](65cadb4)), closes [/github.com//pull/174#issuecomment-538999459](https://github.com//github.com/saltstack-formulas/template-formula/pull/174/issues/issuecomment-538999459)
* **issues:** use larger headings (from level 4 to level 3) ([53e7b75](53e7b75))
* **pillar.example:** fix TOFS comment to explain the default path [skip ci] ([fde5063](fde5063)), closes [/github.com/saltstack-formulas/libvirt-formula/pull/60#issuecomment-537965254](https://github.com//github.com/saltstack-formulas/libvirt-formula/pull/60/issues/issuecomment-537965254) [/github.com/saltstack-formulas/libvirt-formula/pull/60#issuecomment-537988138](https://github.com//github.com/saltstack-formulas/libvirt-formula/pull/60/issues/issuecomment-537988138)
* **pillar.example:** improve TOFS comment to explain the default path [skip ci] ([27d2fe4](27d2fe4)), closes [/github.com/saltstack-formulas/nginx-formula/blob/17291a0ae2c2554707b79d897bb6ddec716e8426/pillar.example#L340-L341](https://github.com//github.com/saltstack-formulas/nginx-formula/blob/17291a0ae2c2554707b79d897bb6ddec716e8426/pillar.example/issues/L340-L341)
@saltstack-formulas-travis

🎉 This PR is included in version 3.3.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@n-rodriguez
Copy link
Member

@myii

@n-rodriguez On a separate note, any feedback about the Ruby issue I asked above?

Sorry for the late answer : usually I conform to https://github.com/rubocop-hq/ruby-style-guide#method-invocation-parens and https://github.com/rubocop-hq/ruby-style-guide#method-invocation-parens-internal-dsl and generally speaking to the whole guide itself.

Between :

describe file('/etc/postgresql/9.1/main/pg_hba.conf') do
  its('content') { should match(%r{local\s.*?all\s.*?all\s.*?md5}) }
  its('content') { should match(%r{host\s.*?all\s.*?all\s.*?127.0.0.1\/32\s.*?md5}) }
  its('content') { should match(%r{host\s.*?all\s.*?all\s.*?::1\/128\s.*?md5}) }
end

VS

describe file('/etc/postgresql/9.1/main/pg_hba.conf') do
  its('content') { should match %r{local\s.*?all\s.*?all\s.*?md5} }
  its('content') { should match %r{host\s.*?all\s.*?all\s.*?127.0.0.1\/32\s.*?md5} }
  its('content') { should match %r{host\s.*?all\s.*?all\s.*?::1\/128\s.*?md5} }
end

I prefer the second version.

@myii
Copy link
Member Author

myii commented Oct 9, 2019

@n-rodriguez Thanks for the confirmation, that's what I went with in the end.

myii pushed a commit to myii/ssf-formula that referenced this pull request Oct 10, 2019
# [1.18.0](v1.17.1...v1.18.0) (2019-10-10)

### Bug Fixes

* **bin/kitchen:** fix `rubocop` errors ([](58881a7))
* **gemfile:** fix `rubocop` errors ([](e0ec88a))
* **salt-lint:** fix errors ([](5890b8a))

### Code Refactoring

* **defaults:** use node anchors for common `line_length` values ([](ac9b7a5))

### Continuous Integration

* merge travis matrix, add `salt-lint` & `rubocop` to `lint` job ([](2dac9b0))

### Features

* **rubocop:** add per-formula overrides ([](212edf0))
* **rubocop:** include for this repo ([](f4fc3c1))
* **salt-lint:** add per-formula overrides (via. TOFS) ([](9ec9b1e))
* **salt-lint:** include for this repo ([](1d9636e))
* **travis:** update for new structure of merging the `lint` stage ([](dbee3f7))
* **travis:** use `env` and `name` for improved display in Travis ([](8d86eb4)), closes [/github.com/saltstack-formulas/template-formula/pull/175#discussion_r332613933](https://github.com//github.com/saltstack-formulas/template-formula/pull/175/issues/discussion_r332613933)
* **travis:** use conditional to provide one or two lint jobs ([](5c2f134))
* **ufw:** add specific `pip3` customisations to `.travis.yml` ([](c3acbd1))
@myii
Copy link
Member Author

myii commented Oct 12, 2019

@dafyddj @n-rodriguez Interesting note about the parentheses. In the case with should match (i.e. regex), it requires them otherwise it triggers:

Which probably explains why the InSpec documentation had them in that example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants