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

style(inspec): match current practices for file and control names #212

Merged

Conversation

baby-gnu
Copy link
Contributor

@baby-gnu baby-gnu commented Oct 8, 2020

PR progress checklist (to be filled in by reviewers)

  • Changes to documentation are appropriate (or tick if not required)
  • Changes to tests are appropriate (or tick if not required)
  • Reviews completed

What type of PR is this?

Primary type

  • [build] Changes related to the build system
  • [chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
  • [ci] Changes to the continuous integration configuration
  • [feat] A new feature
  • [fix] A bug fix
  • [perf] A code change that improves performance
  • [refactor] A code change that neither fixes a bug nor adds a feature
  • [revert] A change used to revert a previous commit
  • [style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

  • [docs] Documentation changes
  • [test] Adding missing or correcting existing tests

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

Describe the changes you're proposing

I don't find a reference best practices in the documentation but:

  • the _spec in the control file name is inherited from RSpec and few examples use it

  • the control name should be an identifier, I matched the tpldot name

  • the title is a human readable description of the control

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Now the output looks like this:

  ✔  TEMPLATE.config.file: verify the template-formula.conf lines
     ✔  File /etc/template-formula.conf is expected to be file
     ✔  File /etc/template-formula.conf is expected to be owned by "root"
     […]	 	 
  ✔  TEMPLATE.subcomponent.config.file: verify the subcomponent configuration file
     ✔  File /etc/TEMPLATE-subcomponent-formula.conf is expected to be file
     ✔  File /etc/TEMPLATE-subcomponent-formula.conf is expected to be owned by "root"
     […]

  ✔  TEMPLATE._mapdata: `map.jinja` should match the reference file
     ✔  File /tmp/salt_mapdata_dump.yaml is expected to exist
     ✔  File /tmp/salt_mapdata_dump.yaml content is expected to eq "# yamllint disable rule:indentation rule:line-length\n# CentOS Linux-8\n---\nadded_in_defaults: defa...ATE-subcomponent-config-file-file-managed:\n    - subcomponent-example.tmpl.jinja\nwinner: pillar\n"
  ✔  TEMPLATE.package.install: required package should be installed
     ✔  System Package bash is expected to be installed
  ✔  TEMPLATE.service.running: the service should be running and enabled
     ✔  Service systemd-journald is expected to be installed
     ✔  Service systemd-journald is expected to be enabled
     ✔  Service systemd-journald is expected to be running

Documentation checklist

  • Updated the README (e.g. Available states).
  • Updated pillar.example.

Testing checklist

  • Included in Kitchen (i.e. under state_top).
  • Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
  • Updated the relevant test pillar.

Additional context

We spoke abouth this in Salt Formula Working Group 2020-09-08

@baby-gnu baby-gnu requested a review from a team as a code owner October 8, 2020 14:29
@baby-gnu baby-gnu force-pushed the refactor/inspec-best-practices branch from 13c1d13 to 1001fe4 Compare October 8, 2020 14:33
@baby-gnu baby-gnu changed the title refactor(inspec): match current practices for file and control names style(inspec): match current practices for file and control names Oct 8, 2020
Copy link
Member

@daks daks left a comment

Choose a reason for hiding this comment

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

At work, we also use Inspec like it was/is (except for the filename which has no real sense in fact) before this PR. Do other changes comes from Inspec own documentation?

@baby-gnu
Copy link
Contributor Author

baby-gnu commented Oct 8, 2020

Do other changes comes from Inspec own documentation?

I don't really found guidelines to write control files, I mostly took the comments from the WG meeting and the examples.

@daks
Copy link
Member

daks commented Oct 8, 2020

Do other changes comes from Inspec own documentation?

I don't really found guidelines to write control files, I mostly took the comments from the WG meeting and the examples.

Looks fine, but I'm not sure there is any 'standard', first file opened https://github.com/inspec/inspec/blob/master/examples/kitchen-chef/test/integration/default/web_spec.rb :) with a title and control which forms a complete sentence.

@myii
Copy link
Member

myii commented Oct 8, 2020

Thanks for going to this effort, @baby-gnu. Nice to see working group items getting tackled. I'm pretty happy with the changes, unless we can find some official guidelines for how to make these adjustments.

By the way, this also reminded me that we have this user story in Taiga, to propose adding some kitchen-salt examples to that repo (via. the kitchen-inspec README):

@baby-gnu
Copy link
Contributor Author

baby-gnu commented Oct 8, 2020

Looks fine, but I'm not sure there is any 'standard', first file opened https://github.com/inspec/inspec/blob/master/examples/kitchen-chef/test/integration/default/web_spec.rb :) with a title and control which forms a complete sentence.

You're right @daks and then I found things like this and the counter example in the same directory 🤪

@dafyddj
Copy link
Contributor

dafyddj commented Oct 8, 2020

As the one who brought this up in the WG, here is what my comments were based on: Chef InSpec Profile Style Guide

@baby-gnu baby-gnu force-pushed the refactor/inspec-best-practices branch from 1001fe4 to adb80d0 Compare February 3, 2021 09:38
@baby-gnu baby-gnu requested a review from a team as a code owner February 3, 2021 09:38
@myii
Copy link
Member

myii commented Feb 4, 2021

Thanks, @baby-gnu, I'm good with this. Just a minor suggestion offered.

Any further concerns or suggestions, @daks, @dafyddj?

@baby-gnu baby-gnu force-pushed the refactor/inspec-best-practices branch from adb80d0 to 56c4b4a Compare February 4, 2021 15:00
Copy link
Contributor

@dafyddj dafyddj left a comment

Choose a reason for hiding this comment

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

Perhaps I'm being pedantic but just making some suggestions for tidiness/consistency.

test/integration/default/controls/config.rb Outdated Show resolved Hide resolved
test/integration/default/controls/packages.rb Outdated Show resolved Hide resolved
test/integration/default/controls/services.rb Outdated Show resolved Hide resolved
test/integration/default/controls/subcomponent_config.rb Outdated Show resolved Hide resolved
@baby-gnu baby-gnu force-pushed the refactor/inspec-best-practices branch from 56c4b4a to 728640c Compare February 10, 2021 15:02
@myii
Copy link
Member

myii commented Feb 10, 2021

Thanks @baby-gnu.

@dafyddj If you're happy with this, feel free to merge it.

@myii myii requested a review from dafyddj February 10, 2021 19:06
I don't find a reference best practices in the documentation but as
stated during a SaltStack Formula Workgroup meeting:

- the `_spec` in the control file name is inherited from RSpec and few
  examples use it

- the `control` name should be an identifier, I matched the `tpldot`
  name

- the `title` is a human readable description of the `control`

- remove `impact` which is not appropriate for an integration test
@baby-gnu baby-gnu force-pushed the refactor/inspec-best-practices branch from 728640c to aa8a58b Compare February 10, 2021 22:02
@dafyddj dafyddj merged commit 4777a08 into saltstack-formulas:master Feb 11, 2021
@baby-gnu baby-gnu deleted the refactor/inspec-best-practices branch February 11, 2021 06:10
@saltstack-formulas-travis

🎉 This PR is included in version 4.3.7 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

myii added a commit to myii/ssf-formula that referenced this pull request Feb 11, 2021
myii added a commit to myii/ssf-formula that referenced this pull request Feb 11, 2021
* saltstack-formulas/template-formula#212

Use `:r! grep -riIn _mapdata_spec` to locate all of the lines to be
adjusted.
Then fix with `%s:_mapdata_spec.rb:_mapdata.rb:` (check changes
carefully afterwards).

Use `:r! find ssf/files/ -name _mapdata_spec.rb` to locate all of the
templates to rename and adjust.
myii added a commit to myii/ssf-formula that referenced this pull request Feb 11, 2021
* saltstack-formulas/template-formula#212

Use `:r! grep -riIn _mapdata_spec` to locate all of the lines to be
adjusted.
Then fix with `%s:_mapdata_spec.rb:_mapdata.rb:` (check changes
carefully afterwards).

Use `:r! find ssf/files/ -name _mapdata_spec.rb` to locate all of the
templates to rename and adjust.
@myii
Copy link
Member

myii commented Feb 11, 2021

Changes have been propagated to all formulas using myii/ssf-formula#292.

@myii
Copy link
Member

myii commented Feb 11, 2021

Changes have been propagated to all formulas using myii/ssf-formula#292.

Note, this means that _mapdata_spec.rb is renamed to _mapdata.rb in all formulas but the rest of the InSpec files will still have the _spec in them. I did contemplate adding a little script to the PR, so that these would be automatically fixed as well but I felt that was a little too much to push through in one go. Any thoughts on a cleaner way to do this or shall we just fix them in each formula manually, in due course?

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.

5 participants