Increase yamllint coverage

[ssbarnea]

Makes yamllinting execution to cover files that previously were not
linted in order to assure consistent formatting.

This also enables people to run yamllint without extra parameters
and avoid getting errors.

Signed-off-by: Sorin Sbarnea [email protected]

PR Type

• Bugfix Pull Request

[webknjaz]

Don't indent sequences in YAML files mainly maintained by me. Other than that, it's fine.

[ssbarnea]

@webknjaz Please don't take this personally by the whole point of the change is to avoid file exceptions in the linter. That is not a personal preference on indentation, that's the linter rule.

[webknjaz]

that's the linter rule.

I don't accept it because linter is wrong. And if you want to apply wrong rules to other files it's fine as long as it doesn't break files I care about.

[decentral1se]

This also enables people to run yamllint without extra parameters and avoid getting errors.

Worth it for this, I'd say.

No idea how you're going to persuade "not in my back garden" issue above though ...

[webknjaz]

how you're going to persuade

That's very easily addressed with having pre-commit tool integrated and having a separate linter

[webknjaz]

This also enables people to run yamllint without extra parameters

While this is nice we shouldn't advertise "you can build/manage your envs manually without tox" approach. This may create "now support my extra weird config" type requests on the issue tracker which is kinda distracting.

[MarkusTeufelberger]

I don't accept it because linter is wrong.

Please cite the part of the specification that is violated by writing a list in a mapping with indentation before the -. Otherwise it is just a matter of personal taste.

[decentral1se]

Yes, please spare us personal subjectivities of "wrong" ...

[webknjaz]

Please cite the part of the specification

Can you cite the part where it says that indented thing is more readable then?

[MarkusTeufelberger]

I never made a claim about readability, but you made a claim that yamllint is wrong.

Here's the spec on indenting sequences:

https://yaml.org/spec/1.2/spec.html#id2759963 in Example 2.3, 2.9 and 2.10 indents sequences with 2 spaces.

Also from there (emphasis mine):
"YAML’s block collections use indentation for scope and begin each entry on its own line." As the scope of a list in a mapping is the key of the mapping, not the mapping itself, the list should therefor be indented.

Edit: Also the "full examples" (2.27 and 2.28) indent lists this way (treating the - part as part of the indented string, not as the indentation itself)

[webknjaz]

Those examples aren't etalons of readability. Especially not that one next to your snippet example 2.4.

Your snippet was too generic.

There's a section about paragraphs https://yaml.org/spec/1.2/spec.html#-%20block%20sequence%20entry// saying: A block sequence is simply a series of nodes, each denoted by a leading “-” indicator. with no mention of the indentation required.

Plus we all know that parsers do not require it.
If it was true, we'd apply indentation on the document level as well following instructions about that (because a document is also a scope):

---

  - item1:

      - with:

          - 
            some: subitem2

  - item3:

      - with:

          - 
            some: subitem4

---

  - item5:

      - with:

          - 
            some: subitem6

  - item7:

      - with:

          - 
            some: subitem8

---

this example doesn't use too much nesting, but for more complex things it turns YAML from format for humans into yet another format for robots.

[MarkusTeufelberger]

Plus we all know that parsers do not require it.

The linter requires it. Every other YAML file in this repository conforms to it. It is valid and well-formed YAML. You claim that the linter is wrong. Prove it.

[decentral1se]

Please fix the CI errors and let's get this merged. I think making an exception for specific maintainers in our configuration is not reasonable and I see a majority of community memebers arguing for this change to be accepted.

[decentral1se]

Merging can be performed automatically once the requested changes are addressed.

Please unblock this @webknjaz.

[webknjaz]

Majority of community members will not end up reading it. I'm not convinced.

[MarkusTeufelberger]

So... what would it take to convince you otherwise?

[webknjaz]

@MarkusTeufelberger I've already explained my position. Let's listen to each other instead of just pushing for own vision.
I don't care about files I mostly don't look at (actually I care, but less, so, as a compromise, I don't even try telling you what to do there).
But forcing me to format things you never even contributed (and most likely never will) is selfish.

Readability matters. But what does this actually mean?
I think that things should be readable to ones who are actually going to read them. It's just so happened that in this case, it's me.
I maintain tons of similar configs (.travis.yml, in particular!) in a lot of other repos. I often even copy-paste chunks between them when I find new/better approaches. It is PITA to have it in different formats all over the Internet.
And even if not for easy and less error-prone code merging, maintainability is also an important factor here. It's probably connected with the power of habit, however, by having familiar things looking familiar you reduce the time for understanding of what's going on along with the number of errors connecting to it.

[MarkusTeufelberger]

Great, so you just need me to take over maintaining the .travis-ci file until travis finally dies.

It takes ~500 lines to run only 28 jobs anyways and contains more anchors than Popeye's village, so I'll get started there right away.

[decentral1se]

Let's listen to each other instead of just pushing for own vision.

As I understand this: your position within this thread is "I maintain this file more than other people and I want it to be formatted the way I prefer". Every other community member on this thread has argued for "the project has a general configuration and it is more consistent to apply it across the entire project". So this is about a "project vision" and not our "own vision". This is not the first time that you have argued obstinately against other community members opinions and now you are in actual fact blocking that point of view from progressing. I find this both worrying and demoralising.

[decentral1se]

Please see ansible/community#427 (comment).

[MarkusTeufelberger]

Ok, after looking into it: Using more than one variable env-var with Travis is a mess. There would be other options out there too (e.g. Circle-CI, which has nicer integration with Github checks too or Azure devops), but according to the latest meeting notes, molecule might switch to a different CI system anyways (ansible-core-ci) after 2.8 drops so it's probably not worth investing any more time in this.

[webknjaz]

@decentral1se

As I understand this: your position within this thread is "I maintain this file more than other people and I want it to be formatted the way I prefer".

Again: it's just an end-effect of underlying reasoning everyone seems to have chosen to ignore. I've got actual supporting arguments there rather than just "this other guy also thinks so".

You seem to be advocating to democracy (which evidently doesn't work well; seealso: trump/brexit) where the majority is considered to know better than everyone else. While I argue that quality is more important than quantity.

I find this both worrying and demoralising.

I'm sorry to hear that. I understand that discussions may become heated sometimes but it's never my intention to make anyone feel bad.
OTOH I fully understand your feelings. I myself feel like I've suddenly got into the middle of a quite unwelcoming place. Proving points to folks who are not interested in hearing any reason is exhausting.

@MarkusTeufelberger

It takes ~500 lines to run only 28 jobs anyways and contains more anchors than Popeye's village

Before judging first take time to actually understand the reasoning behind the solution. I don't even know what else to say here. You can unwrap that into 1500 lines if that seems better maintainable to you...

Yes, more explicit job configs are easier to read (this is where you're 100% correct) but unfortunately hard to maintain. At one point I even was going to have a CI config generator but never got to implement that...

Using more than one variable env-var with Travis is a mess

Yes and no. There's advantages and drawbacks in all CIs. I've tried the majority of them. Travis' upside is that it has probably the best integration with GitHub while disadvantages include poor flexibility of the config file in particular. There's more. It's easy to judge what's wrong and it's hard to do what's right. Writing that something is bad is discouraging. Not having improvement suggestions while doing so is also making such criticism more or less useless.

I did the best I could do with this which is having an explicit list of jobs with a few explicit variables for readability.

[gundalow]

• yamllint by definition is opinionated, that's the purpose of linters
• Coding standard is what's enforced by CI. (Though this is an update to that standard)
• It was previously confusing to have different coding standards for different parts of the code base
• I'd like the Molecule Community to move away from all these formatting discussions:
  • It's at best busy work
  • At worst we are actively annoying contributors (this has already happened)
  • I believe we all have better things to do with our time.
• Therefore, as interim BDFL I approve this update.

I will work on adding a governance section to the Contributing Guide to make the above clearer.

Travis-CI

Just to confirm the comment made by @MarkusTeufelberger https://github.com/MarkusTeufelberger

• We are currently using (and paying) for travis-ci.com
• Molecule, along with other Ansible related projects will be looking at moving to Zuul

I know the current Travis setup isn't the best, though I want us to avoid the switching cost of moving from Travis to X to Zuul. If the capacity currently allocated (10 concurrent jobs) in Travis-CI.com isn't enough I can fix that by throwing more money at it.

#1919 (comment)

[gundalow]

Note for reviews, use the whitespace disabled flag ?w=1 https://github.com/ansible/molecule/pull/1919/files?w=1

[gundalow]

I just triggered a fresh CI run:

./.pyup.yml
  4:1       warning  missing document start "---"  (document-start)
  9:6       warning  truthy value should be true or false  (truthy)
  13:2      warning  missing starting space in comment  (comments)
  23:9      warning  truthy value should be true or false  (truthy)
  30:1      error    wrong indentation: expected 2 but found 0  (indentation)
  33:10     warning  truthy value should be true or false  (truthy)
  38:10     warning  truthy value should be true or false  (truthy)
  49:1      error    wrong indentation: expected 2 but found 0  (indentation)
  59:2      warning  missing starting space in comment  (comments)
  63:12     warning  truthy value should be true or false  (truthy)
./.readthedocs.yml
  12:3      error    wrong indentation: expected 4 but found 2  (indentation)
  15:5      error    wrong indentation: expected 6 but found 4  (indentation)

[gundalow]

CI is failing as other regressions have been merged since this PR was created.

[webknjaz]

fix the indentation

[ssbarnea]

Tested in my own fork to assure that it does not break DCO service.

[ssbarnea]

Tested in my own fork to assure that it does not break RTD service.

[ssbarnea]

PYUP is the only tool that seems to choke on "hardened" yaml files, so we skip it.

[webknjaz]

@ssbarnea you can add a separate lint once you finish pre-commit one

[ssbarnea]

This the the major goal of this change: so we avoid adding YAML files to other locations that do not have standardized formatting.
Also this makes much eaiser to run run yamllint outside tox or from other tools like IDEs, hooks,...

Sorted, please recheck it.