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

Incorrect processing of YAML indentation #15

Open
EricVS opened this issue Nov 1, 2016 · 8 comments
Open

Incorrect processing of YAML indentation #15

EricVS opened this issue Nov 1, 2016 · 8 comments

Comments

@EricVS
Copy link

EricVS commented Nov 1, 2016

According to both YAML documentation and Ansible YAML syntax (http://docs.ansible.com/ansible/YAMLSyntax.html), the indentation that gets checked with the function files_should_be_indented is incorrect.
A warning is thrown if I have indentation at the - with a list of tags for example.

- name: Create kill link
  file:
    state: link
    src: /opt/uptime-agent/bin/upt_daemon.sh
    dest: /etc/rc.d/rc3.d/K1uptm_daemon
  tags:
    - uptimeconfig
WARN: Best practice "YAML should be correctly indented" not met:
roles/configure.uptimeagent/tasks/main.yml:11: lines starting with '- ' should have same or less indentation than previous line

The warning disappears when I remove the two spaces in front of the dash at the tag, as such.

- name: Create kill link
  file:
    state: link
    src: /opt/uptime-agent/bin/upt_daemon.sh
    dest: /etc/rc.d/rc3.d/K1uptm_daemon
  tags:
  - uptimeconfig

This doesn't follow Ansible YAML syntax as per the documentation, though it works apparently without any issues.

Can you advice on this, if it's intentional or not? Thanks in advance.

Kind regards,

Eric V.

@willthames
Copy link
Owner

Hi Eric,
The examples provided with ansible-review are examples. Of course it also provides the underlying function which is very much targeted at our particular YAML standards.

There are no Ansible indentation standards (yet?). That Ansible YAML page is so inconsistent it's a prime example as to why I created standards in the first place. Whether you choose to have two spaces before your list items or none as we do is down to your organisation - if you wish to add a pull request that makes having two spaces before a hyphen (which is valid, and could be consistent with your own standards) a configurable item, then I would treat such a request favourably.

Hope that makes sense, I absolutely understand your question and the answer is currently "that's how it's implemented" but it doesn't have to be forever (although if everyone came around to my way of thinking, then that'd work quite well ;) )

@EricVS
Copy link
Author

EricVS commented Nov 1, 2016

Hey Will,

Thanks for your swift reply and believe me, I'm all in favor of standards. It's something I try to 'enforce' at my workplace as much as possible.
There was a moment, I believe around version 1.9 of Ansible that threw an error if the indentation was not followed as per their documentation, i.e. as per my example. I ran a test playbook with version 2.2.0 today with both ways of indenting and got no error at all which leads to believe that Ansible is adhering to some standards 👍 .

As you indicated there's no real agreed standard so it's a 'anything goes' situation. Although it would be nice to have everyone adhere to your standard I'll invest some time to create a pull request adapting the code to take the indentation before a hyphen into account if so desired... configurable item as you indicated. I think it would make it less confusing for people like me if a test verifies 'all' valid configurations instead of 'gently enforcing' one.

Kind regards,

Eric V.

@willthames
Copy link
Owner

Also, as long as you and your colleagues agree, what I think is irrelevant. And if one day the Ansible community agrees on a single standard, we'll likely modify our internal standard.

If you are going to implement this, my thoughts are that it should be in config.ini - something like

[ansible-review]
indent_before_hyphen = true
indent_spaces = 4

Not sure I have a better way to do it, but welcome thoughts.

From an Ansible point of view, spaces should be irrelevant as long as the yaml parser works. And it'll work with no, two or four or more spaces before a hyphen.

@EricVS
Copy link
Author

EricVS commented Nov 1, 2016

Couldn't agree more :-)
It can either be done in the config.ini file or taking all valid options into account when running the parser.
At the end, the important thing is that code gets validated correctly and doesn't give a warning on something that's valid.

@willthames
Copy link
Owner

I'm not sure I agree with your last point - lots of different ways of writing yaml are valid. But not necessarily meeting an organisation's standards. I could run ansible-playbook --syntax-check alone if all I cared about was validity. There are four options for you:

  • Accept that my thoughts on indenting yaml lists are the One True Way, and use ansible-review's existing indentation check
  • Say if it's valid, it's fine, and not even use the check at all (after all, all checks are examples, you can just ditch them)
  • Write your own indentation check based on your organisation's needs (you could copy and paste existing check to your standards.py, fix the indentation checking for your own needs)
  • Update the indentation checks so that they're configurable

Obviously I prefer the first ;) followed by the fourth.

@EricVS
Copy link
Author

EricVS commented Nov 1, 2016

I imagine you would prefer the first one and if that would set a standard across the board for YAML indentation then I would implement at once... But I think that with the different ways of writing yaml come the differences in opinion and checking....
If a 'generic' approach taking all valid options into account is no option, then I think that the only option left is the fourth one, making it configurable so it can check according a company's choice of indentation.
Thank you indeed Will for your insight and feedback, I've learned some pretty valid points today.
I'll check in once I've had some time to put together a first version.

@bbaassssiiee
Copy link

bbaassssiiee commented Jun 16, 2017

ansible-review and yamllint have conflicting results with indentation of lists.

https://github.com/adrienverge/yamllint

As "the" standard of ansible-review is only at v0.1 I figure it would not hurt to assimilate.

Yamllint can be configured to co-exist with standard 0.1 using:
indentation: {spaces: 2, indent-sequences: false}

@willthames
Copy link
Owner

willthames commented Jun 17, 2017

I'd definitely accept a patch that used yamllint for the yaml validation. I'd still like to make it configurable though, not really sure how best to do that though - we don't really have anything configurable at the moment. Most people would probably favour a per-repo configuration (rather than the current global ansible-review configuration file) - perhaps a global config with per-repo overrides would make sense.

My current org uses the yamllint approach as standard, I can't say I prefer it, but vim does autoindent for it anyway, so saves me some keystrokes.

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

No branches or pull requests

3 participants