Add doc comments around "string bool" usage #2065
Conversation
I've been poking at composefs related things here and stumbled across the very confusing fact that some values in the storage config are booleans - but must be *strings* even though TOML supports native boolean values. Further, digging in more we actually use two different parsers for boolean values; one silently accepts anything that is not the string `"true"` to mean `false`, but the other uses the Go strconv function and does return an error. I'm not trying to fix any of this yet; the clear fix is to widen what we accept into a native TOML boolean type *in addition* which would require a bit of custom deserialization. Let's just document this to start. Signed-off-by: Colin Walters <[email protected]>
cgwalters
force-pushed
the
toml-string-bool
branch
from
314c8f0 to
d4821ef
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
There’s a fine line between warning readers about unusual situations, and reaffirming known-incorrect behavior as desirable … I think this PR is closer to the latter than I’d like.
If this is worth commenting, I think it’s worth commenting what we, as of today, think about the situation — not just commenting what the code does (because that can be read in the code).
The most important thing is users. For that purpose, I think the “string bool” options should be explicitly documented as such in docs/containers-storage.conf.5.md . If we document that only "true"/"false" are valid values, that implicitly constraints the Go implementation:
- The values directly in
OverlayOptionsConfigdon’t need comments added; they can have a comment pointing out the unusual semantics in the man page, and/or a "to do: also accept native TOML booleans". - The implementation in drivers/overlay might get a comment that we accept undocumented values, and that should ideally be fixed.
| // does not use https://pkg.go.dev/strconv#ParseBool | ||
| // You can search the source code for the term "string bool" for other places to which behave similiarly. | ||
| // Ideally of course we would parse booleans in a consistent | ||
| // way - which would be most especially clear if we supported native TOML values. | ||
| func parseBooleanPullOption(pullOptions map[string]string, name string, def bool) bool { |
There was a problem hiding this comment.
The authoritative documentation of these options is in docs/containers-storage.conf.5.md, and that says "true"/"false".
I think the current behavior accepting other values is really a bug, not something we should document in detail as intentional.
I’m not sure this is correct, but my intuition is that this semantics is more of a stopgap than a carefully designed thing that should be unquestionably documented in detail. That’s also why I think this a map[string]string and not native Go struct / TOML — this was a way to hook things up throughout the call stack with the minimum effort possible.
See also the documentation of StoreOptions.PullOptions:
This API is experimental and can be changed without bumping the major version number.
s/Any/Known bug: Any/- Drop the
ParseBoolreferences, I think, depending on the other half of the discussion. - Maybe clearly say something like “To do: turn pull_options into a native well-typed Go / TOML struct”.
| @@ -20,6 +20,8 @@ type BtrfsOptionsConfig struct { | |||
| type OverlayOptionsConfig struct { | |||
| // IgnoreChownErrors is a flag for whether chown errors should be | |||
| // ignored when building an image. | |||
| // This can take any value accepted by Go's https://pkg.go.dev/strconv#ParseBool | |||
| // Search the source code for the term "string bool" to find other places which behave similarly. | |||
There was a problem hiding this comment.
For all three comments in this file… Referring to ParseBool is a correct description of the current readers. But, well, do we want to commit to accepting things like [01tTfF]? (Think possible Rust consumers of the config.)
I’d rather document just "true"/"false", and treat the behavior of drivers/overlay, accepting the other values, as an undocumented… (non?)bug when processing invalid input.
Especially when, right now, the docs/containers-storage.conf.5.md man page does not document the values at all, adding the ParseBool references here could be construed as a commitment to support the values in the config file. I’d rather not do that.
(Ceterum autem censeo, the c/storage/drivers subpackage should almost entirely become private; and the option parsing should pass these Go structures down to drivers/overlay directly instead of round-tripping through []string. But also, I’m scared of the c/storage option parsing / loading code.)
That was already done in #2066 |
That PR does not touch the authoritative-named containers-storage.conf(5) man page AFAICS. |
xref containers#2065 (comment) This does the same for the man page that I did for the default storage.conf previously; instead of trying to squash all the `pull_options` into a single option that we document "ad-hoc", make it its own table just like the other sections. - Add notes about "string bools" around relevant values. - Avoid redundantly specifying the default; the default is the first thing we show after the `=`. Synchronize the text between the man page and the default `storage.conf`. Signed-off-by: Colin Walters <[email protected]>
You're right, sorry. Hmm...probably we should try to remove some of the text from the config file in favor of referencing the man page, but for now I'll just keep them in sync.
My worry is that there are some people that accidentally set values to one of those. It doesn't seem impossible at all. But, I'm only "kind of" a developer here so if others feel we should pull off the bandaid and just e.g. error out on non-booleans I'm good with that too!
OK, I understand. Closing then in favor of just updating the man page and then we can circle back later and actually a fix to at least support native TOML booleans. |
I've been poking at composefs related things here and stumbled across the very confusing fact that some values in the storage config are booleans - but must be
strings even though TOML supports native boolean values.
Further, digging in more we actually use two different parsers for boolean values; one silently accepts anything that is not the string
"true"to meanfalse, but the other uses the Go strconv function and does return an error.I'm not trying to fix any of this yet; the clear fix is to widen what we accept into a native TOML boolean type in addition which would require a bit of custom deserialization. Let's just document this to start.