Skip to content

storage.conf: Various cleanups#2066

Merged
rhatdan merged 1 commit intocontainers:mainfrom
cgwalters:toml-cleanup
Aug 19, 2024
Merged

storage.conf: Various cleanups#2066
rhatdan merged 1 commit intocontainers:mainfrom
cgwalters:toml-cleanup

Conversation

@cgwalters
Copy link
Contributor

The default storage.conf we ship is inconsistent in a few ways; there's a lot of fixes rolled up into this single commit.

First: we were using a toml "inline table" for the pull options, and then documenting each key in that table in one blob above. It simply looks much nicer to use a non-inline table - then we can move the docs next to each individual value. This is also more consistent with other sections of the config.

I also thinned out a bit the doc comments; I think instead of trying to have a longer explanation of zstd:chunked in the comments here we should refer to the man page, which is a better place to have details (and that we should fill out more).

Per another PR, I also stumbled across the fact that we have a lot of "string bool" values and cannot be native TOML booleans. Document that clearly next to each type.

We already have default values in the code for all of these, so comment them all out to be consistent with other values. (We're then getting closer to having the config file be entirely comments, but that's a distinct project)

Finally, update the recent man pages I added to match these changes.

The default storage.conf we ship is inconsistent in a few ways;
there's a lot of fixes rolled up into this single commit.

First: we were using a toml "inline table" for the pull options,
and then documenting each key in that table in one blob above.
It simply looks much nicer to use a non-inline table - then
we can move the docs next to each individual value. This is also
more consistent with other sections of the config.

I also thinned out a bit the doc comments; I think instead of trying
to have a longer explanation of zstd:chunked in the comments here
we should refer to the man page, which is a better place to have
details (and that we should fill out more).

Per another PR, I also stumbled across the fact that we have a
lot of "string bool" values and cannot be native TOML booleans. Document
that clearly next to each type.

We already have default values in the *code* for all
of these, so comment them all out to be consistent with other
values. (We're then getting closer to having the config file
be entirely comments, but that's a distinct project)

Finally, update the recent man pages I added to match
these changes.

Signed-off-by: Colin Walters <walters@verbum.org>
@rhatdan
Copy link
Member

rhatdan commented Aug 19, 2024

Can we support both native and string at the same time?

LGTM

@giuseppe @nalind @mtrmac @flouthoc @saschagrunert PTAL

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 19, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, flouthoc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@flouthoc
Copy link
Collaborator

I agree that maybe both native and string format must be supported to make things easier.

@cgwalters
Copy link
Contributor Author

Can we support both native and string at the same time?

Yes, I believe so, but we should look at that after this.

@rhatdan
Copy link
Member

rhatdan commented Aug 19, 2024

Agreed.

@rhatdan rhatdan merged commit f4fccb7 into containers:main Aug 19, 2024
lsm5 added a commit to lsm5/common that referenced this pull request Sep 3, 2024
pull_options syntax has chagned recently.
Ref: containers/storage#2066

Signed-off-by: Lokesh Mandvekar <lsm5@fedoraproject.org>
lsm5 added a commit to lsm5/common that referenced this pull request Sep 5, 2024
storage.conf has been updated upstream so the config files update script
needs to account for that.

Ref: containers/storage#2066

Signed-off-by: Lokesh Mandvekar <lsm5@fedoraproject.org>
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 this pull request may close these issues.

3 participants