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

Feature Request: warnings should link to explanation #70

Closed
bmwiedemann opened this issue Oct 22, 2019 · 8 comments · Fixed by #91
Closed

Feature Request: warnings should link to explanation #70

bmwiedemann opened this issue Oct 22, 2019 · 8 comments · Fixed by #91
Labels

Comments

@bmwiedemann
Copy link

I was trying salt-lint on a repo of ours and it reported

[207] File modes should always be encapsulated in quotation marks
srv/salt/service/etherpad.sls:25
    - mode: 0755

[210] Numbers that start with `0` should always be encapsulated in quotation marks
srv/salt/service/etherpad.sls:25
    - mode: 0755

yet nowhere (not even in salt-lint git commit history) could I find any reasoning on why would it be a good idea to have a mode as string instead of a number. The "Why" is always an important bit to communicate.

On this particular one, https://docs.saltstack.com/en/latest/ref/states/all/salt.states.file.html has examples with 644, 0644 and '0644' styles, suggesting that there is no strong consensus.

@roaldnefs roaldnefs added the Type: Enhancement New feature or request label Oct 22, 2019
@roaldnefs
Copy link
Member

Thanks @bmwiedemann for the feature request! This would be a great addition to the linter, definitely something to think about for a future release.

Numbers (unquoted) with a leading zero are interpreted as octal values in YAML, for more information see: #29.

@bmwiedemann
Copy link
Author

bmwiedemann commented Oct 22, 2019

Numbers (unquoted) with a leading zero are interpreted as octal values in YAML

This is fine, because file modes are octal numbers since 40+ years.

e.g. look for 0400 in http://man7.org/linux/man-pages/man2/open.2.html

@jbouter
Copy link
Member

jbouter commented Oct 22, 2019

Regarding octal values

Only if they are interpreted as such by the module setting the rights, and not wrongly interpreted by SaltStack. From the SaltStack documentation:

When using a mode that includes a leading zero you must wrap the value in single quotes. If the value is not wrapped in quotes it will be read by YAML as an integer and evaluated as an octal.

Regarding rules and their documentation

hadolint have their rules listed in a table in their readme, linking to corresponding wiki pages with an explanation and and example. This might be a good idea to implement ourselves.

@roaldnefs
Copy link
Member

It would be nice if SaltStack would accept unquoted leading zero mode's, but this causes some issues on the YAML rendering, see: saltstack/salt#661.

@bmwiedemann
Copy link
Author

IMHO that warning on the SaltStack doc is bogus. In 3 other places on the same place they do use a mode with a leading 0

The quoting only makes sense if there are places where people write 0123 and expect it to be interpreted as decimal (so not mode).

@jbouter
Copy link
Member

jbouter commented Oct 22, 2019

We're currently working on disabling rules through configuration (though you already can at the time of running salt-lint). Feel free to disable the check if you think it's bogus.

I'd like for @myii or other SaltStack developers to chime in before we remove checks that we have based on SaltStack's documentation.

@myii
Copy link
Contributor

myii commented Oct 22, 2019

IMHO that warning on the SaltStack doc is bogus...

@bmwiedemann Unfortunately, it's not bogus. The issue is with YAML itself, not Salt specifically. A very basic example:

>>> import yaml
>>> yaml.load('key: 0644')
{'key': 420}
>>> yaml.safe_load('key: 0755')
{'key': 493}

Also have a look at the yamllint documentation:

Recent versions of Salt have incorporated a fix for this, but 2017.7 (which is still in official support) still suffers from this bug. Based on the following YAML:

template:
  octals:
    test1: 0010
    test2: 0400
    test3: 0700
    test4: 0600
    test5: 0755
    test6: 0644

This is the result: https://travis-ci.org/myii/template-formula/jobs/601413963#L763-L774.

       [ERROR   ] octals1
       8
       [ERROR   ] octals2
       256
       [ERROR   ] octals3
       448
       [ERROR   ] octals4
       384
       [ERROR   ] octals5
       493
       [ERROR   ] octals6
       420

... In 3 other places on the same place they do use a mode with a leading 0

The documentation is a collaborative effort. Those entries were either made before the problem was identified or by someone who was unaware of it. Would you care to submit a PR to fix those? That would be helpful. The file to be modified is:

@roaldnefs
Copy link
Member

An overview of all the rules will be made available in #82, in the table each rule links to https://github.com/warpnet/salt-lint/wiki for the explanations.

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.

4 participants