-
Notifications
You must be signed in to change notification settings - Fork 56
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
Add example config TOML #871
Conversation
d5366b7
to
bdd0549
Compare
Created #872 to try and clean up TOML variable usage, as it's pretty unclear what is and is not actually used. |
d22fd87
to
9e50419
Compare
Signed-off-by: David Son <[email protected]>
9e50419
to
9cdb1d8
Compare
After making some changes I realized that Perhaps another case to prioritize #851 a bit more? |
LGTM. I do think we need a Similar to https://github.com/containerd/containerd/blob/main/docs/man/containerd-config.toml.5.md. |
Created #884 to properly document this change I noted this already but for greater transparency — this implementation has one bug I managed to do a workaround for. I also quite frankly don't like how I implemented the test (copying the file from x location) but as the alternative is maintaining two identical copies of the same logic I suppose this is the best way to do this. |
Issue #, if available:
#758
Description of changes:
Added an example TOML in config/config.toml documenting most of the available variables that can be changed in the TOML file. I omitted some variables that are known to be unused (particularly the
Config
struct in in service/plugin/plugin.go), but there are probably still some variables in this example that have no effect. If any of them are known to do nothing, they can be changed in a later PR.I additionally left some comments attempting to clarify what certain variables do. For example,
MaxWaitMsec=0
under[http]
will actually set the variable to the default value stored in the codebase for MaxWaitMsec, which in this case would be 300,000. This makes sense in this context (as waiting 0ms would never allow it to happen), but for something likemin_layer_size=0
under[snapshotter]
, where a user might want no minimum layer size, it could cause some confusion. I did my best to outline this in the comments, but perhaps this confusion could be cleared in the future (e.g. by defaulting numeric values to -1 instead of 0).Testing performed:
Created a new integration test in
run_test.go
calledTestRunWithDefaultConfig
, which creates a snapshotter base shell and reboots containerd with the example TOML. If it cannot parse the example TOML, it fails.I also verified the TOML structure by getting the config and marshalling it with gotoml (right before line 178 in cmd/soci-snapshotter-grps/main.go here), then printing to stdout.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.