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

Suggested improvements/alternative to file mode implementation #29

Closed
myii opened this issue Oct 6, 2019 · 5 comments · Fixed by #31
Closed

Suggested improvements/alternative to file mode implementation #29

myii opened this issue Oct 6, 2019 · 5 comments · Fixed by #31
Assignees

Comments

@myii
Copy link
Contributor

myii commented Oct 6, 2019

The underlying issue here is that numbers beginning with zero are interpreted as octal in YAML.

https://docs.saltstack.com/en/latest/ref/states/all/salt.states.file.html

Warning

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.

A useful look is how yamllint handles these.

https://yamllint.readthedocs.io/en/stable/rules.html#module-yamllint.rules.octal_values

  • Ultimately, this is about any numbers beginning with 0, not just file/dir modes.

I've done test runs on 45 formulas under the SaltStack Formulas organisation. Many of them fall afoul of both 207 and 208. The problem is that most of these are false positives. The very common usage of 3-digit, unquoted modes (e.g. 755 and 644) is completely legitimate. My suggestion is that these shouldn't be flagged as errors/warnings. There's a simple workaround for this for the time being:

  • Apply something similar to: myii@177e3de
  • Add 208 to the ignored list

A more comprehensive solution could be achieved under a new lint ID, which checks for any number with a leading zero (not just file/dir modes), as shown on the yamllint docs. Then users could add both 207 and 208 to the ignore list and use the comprehensive octal bug linter instead.

@roaldnefs
Copy link
Member

Thanks for the suggestion @myii!

I prefer the comprehensive solution that could be achieved under a new lint ID, which checks for any number with a leading zero (not just file/dir modes), as shown on the yamllint docs.

Please let me know if you would like to take a stab at a PR, otherwise I will try to make an extra rule for it tonight.

@myii
Copy link
Contributor Author

myii commented Oct 6, 2019

@roaldnefs Thanks for the quick response. I've got my hands full with SaltStack Formulas at the current time (including rolling out salt-lint!) so I won't be able to do much at the moment. Thanks for offering to take it on.

@jbouter
Copy link
Member

jbouter commented Oct 7, 2019

@myii We have indeed decided that #207 and #208 can be added to an exclude list if so desired, and @roaldnefs has added an extra check (#210) that simply detects unquoted leading-zero numbers and flags them. Thanks for your suggestions and contribution! :-)

@myii
Copy link
Contributor Author

myii commented Oct 7, 2019

Fantastic!

@tacerus
Copy link

tacerus commented Jan 5, 2024

Hi,

I wonder, with 210 being in place, is there still a valid reason for 207 and 208? As @myii states (and patched in myii@177e3de), there are valid uses for three digit file modes, ones without a leading zero - for example 640, 755, ...?

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

Successfully merging a pull request may close this issue.

4 participants