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

why decimal vs octal for file permissions #842

Closed
dustymabe opened this issue Aug 8, 2019 · 8 comments
Closed

why decimal vs octal for file permissions #842

dustymabe opened this issue Aug 8, 2019 · 8 comments

Comments

@dustymabe
Copy link
Member

From the docs:

mode (integer): the file's permission mode. Note that the mode must be properly specified as a decimal value (i.e. 0644 -> 420). If not specified, the permission mode for files defaults to 0644 or the existing file's permissions if overwrite is false, source is unspecified, and a file already exists at the path.

I know users aren't technically supposed to write ignition configs, but if they ever have to look at them why not use a notation that's familiar? Similarly if you leave out the permissions the warning you get looks like:

warning at $.storage.files.2.mode, line 19 col 14: permissions unset, defaulting to 0644

Which means the error message actually shows us the perms in octal notation.

@ajeddeloh
Copy link
Contributor

That error message should be updated. Maybe to something like "defaulting to 420 (0644)"?

Several reasons:

  • parsing integers is easier
  • you shouldn't be writing them by hand
  • writing tooling that generates them is easier if it's integers
  • you can do bitwise math on them if they're integers

Basically they're easier to deal with in go if they're integers and json doesn't support octal notation for numbers.

@ajeddeloh
Copy link
Contributor

Going to close this unless there's more to discuss. Feel free to reopen.

@lucab
Copy link
Contributor

lucab commented Feb 11, 2020

Circling back to this:

but if they ever have to look at them why not use a notation that's familiar?

This cannot be technically done, as it breaks the following invariants:

  • Ignition config must be valid JSON document
  • JSON specs (section 8 of ECMA-404) specifies that numerical values must be in decimal base and without leading zeroes

@jamescassell
Copy link

Ansible recently "solved" this by defining them as strings. It's certainly easier when reading the output of jq, for example.

@dustymabe
Copy link
Member Author

I think we should do something similar. While maybe the point about "users should generate ignition configs and not hand edit them" is valid, I find that my life is much harder as someone who is trying to support users (myself being a user as well) who are passing me Ignition configs (because that's what the machine sees) and asking for help. My brain just doesn't easily convert from decimal to octal.

In short I just don't think it makes practical sense to not use an expected notation for this in the long term. In the short term, yes, we'll probably keep it because it's another "migration" that needs to happen.

@jlebon
Copy link
Member

jlebon commented Mar 12, 2020

Hmm, something like support either integer or string? If integer, keep parsing as decimal as usual, but if string, base it off of C convention? (100 vs 0100 vs 0x100)? Obviously though parsing a string opens a new way for Ignition to fail at runtime. And it makes things more confusing to have two ways to specify modes. So... not sure.

@dustymabe
Copy link
Member Author

I would say just create a new version of the spec that requires a string, old ignition configs should still work by upconverting 3.0.0 to the next version.

@arithx
Copy link
Contributor

arithx commented Mar 12, 2020

Hmm, something like support either integer or string?

I'd generally rather not; if we did go down this route I think the only viable way is to create an additional field (similar to what we did with start & startMiB in spec2x).

I would say just create a new version of the spec that requires a string

You'd either need to implement a new field other than mode which allowed for octal strings or you'd be talking about spec 4.0.0. Old configs need to keep working if you just change the config version forwards (within the same major version), not just that they can keep translating forward.

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

6 participants