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

Use yamllint config to prevent empty-values & use new yaml-files setting #164

Closed
myii opened this issue Sep 6, 2019 · 10 comments · Fixed by #165
Closed

Use yamllint config to prevent empty-values & use new yaml-files setting #164

myii opened this issue Sep 6, 2019 · 10 comments · Fixed by #165
Labels

Comments

@myii
Copy link
Member

myii commented Sep 6, 2019

This has come up before and there was a reminder of it here:

Having empty values in pillar.example (or any of the YAML files) is never a good idea, leading to confusion and rendering errors. Values should be set explicitly, with it being easy to set null if that's what's really required.

With yamllint now active in our repos, this can easily be enforced from its configuration file:

 rules:
+  empty-values:
+    forbid-in-block-mappings: true
+    forbid-in-flow-mappings: true
+
   line-length:
     # Increase from default of `80`
     # Based on https://github.com/PyCQA/flake8-bugbear#opinionated-warnings (`B950`)
     max: 88

Propagating this is straightforward using the ssf-formula.

@myii
Copy link
Member Author

myii commented Sep 9, 2019

So I've run some tests across all of the relevant formulas using myii/ssf-formula#27. As you can see from that page, 8 out of the nearly 40 formulas have issues. The rest pass the yamllint with this rule included. Clicking through to the Travis run via. the red crosses shows that about 4 of those would be relatively easy to solve (just a handful of values to fix). So in my opinion, this rule is worth enforcing now, with the fixes made to these 8 formulas as well. Any comments about this before I go ahead with this?

@baby-gnu
Copy link
Contributor

baby-gnu commented Sep 9, 2019

Hello.

So in my opinion, this rule is worth enforcing now, with the fixes made to these 8 formulas as well.

I agree. I'm really happy to see how the formulas improve.

Regards.

@myii
Copy link
Member Author

myii commented Sep 9, 2019

Thanks, @baby-gnu. I'll just wait to see if there any other responses before taking the next step (if applicable).

@daks
Copy link
Member

daks commented Sep 9, 2019

OK for me

@myii
Copy link
Member Author

myii commented Sep 9, 2019

@daks Excellent, that's good enough for me. I'll get something rolled out soon, the only thing that will delay me a little is getting the fixes included as well.

myii added a commit to myii/ssf-formula that referenced this issue Sep 9, 2019
@n-rodriguez
Copy link
Member

n-rodriguez commented Sep 9, 2019

@myii it could the occasion to improve documentation about accepted values (bool, int, string, array, hast, null)

@myii
Copy link
Member Author

myii commented Sep 9, 2019

@n-rodriguez Do you mean from a YAML angle or across the SaltStack Formulas organisation? At the current time, my opinion is that as long as it's valid YAML, that's fine. There are cases where null is required, so that's good as well but it should be set explicitly rather than implicitly. I've seen that empty values are being used as an empty string and that's not the case, so the idea here is to prevent that misunderstanding going forward. If you can suggest any specific documentation, that would be great, though.

Generally, there's more good news. The latest release of yamllint has introduced the yaml-files setting, which means that we no longer have to list all of the files to be checked by yamllint. This configuration option will work across all of our repos:

yaml-files:
  - '*.yaml'
  - '*.yml'
  - '.yamllint'
  - '*.example'
  - 'test/**/*.sls'

So I'm planning to include this at the same time.

@n-rodriguez
Copy link
Member

The latest release of yamllint has introduced the yaml-files setting, which means that we no longer have to list all of the files to be checked by yamllint.

great!

myii added a commit to myii/ssf-formula that referenced this issue Sep 10, 2019
myii added a commit to myii/ssf-formula that referenced this issue Sep 10, 2019
@myii myii changed the title Use yamllint config to prevent empty-values Use yamllint config to prevent empty-values & use new yaml-files setting Sep 10, 2019
@myii
Copy link
Member Author

myii commented Sep 10, 2019

The PRs have all been created. A few have been marked as WIP where the extra work needs to be done but most are ready to go.

myii added a commit to myii/template-formula that referenced this issue Sep 10, 2019
@myii myii closed this as completed in #165 Sep 12, 2019
saltstack-formulas-travis pushed a commit that referenced this issue Sep 23, 2019
## [3.3.1](v3.3.0...v3.3.1) (2019-09-23)

### Bug Fixes

* **subcomponent:** clean referencing wrong sls ([394808e](394808e))

### Continuous Integration

* use `dist: bionic` & apply `opensuse-leap-15` SCP error workaround ([330b0cb](330b0cb))
* **kitchen:** change `log_level` to `debug` instead of `info` ([1b929ff](1b929ff))
* **platform:** add `arch-base-latest` ([042e8e2](042e8e2))
* **yamllint:** add rule `empty-values` & use new `yaml-files` setting ([70ed7e2](70ed7e2)), closes [#164](#164)

### Documentation

* **contributing:** add recent `semantic-release` formulas ([7f36ae9](7f36ae9))
@saltstack-formulas-travis

🎉 This issue has been resolved in version 3.3.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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 a pull request may close this issue.

5 participants