Fix override for log-to-stdout cmdline option #391
Conversation
There was a problem hiding this comment.
It seems that we forgot to add toml:"log_to_stdout" to LoggingConfig.LogToStdout and its tests.
You can find the changes here: https://github.com/containerd/nydus-snapshotter/pull/380/commits
Others, looks great to me.
|
I re-added the log_to_stdout option where it was removed by the mentioned commits. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #391 +/- ##
==========================================
- Coverage 28.08% 27.88% -0.20%
==========================================
Files 40 40
Lines 4084 4077 -7
==========================================
- Hits 1147 1137 -10
- Misses 2798 2801 +3
Partials 139 139
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
There is a legacy issue. We should also restore the configuration item |
It is already restored in this PR? |
| @@ -109,7 +109,7 @@ type DaemonConfig struct { | |||
| } | |||
|
|
|||
| type LoggingConfig struct { | |||
| LogToStdout bool | |||
| LogToStdout bool `toml:"log_to_stdout"` | |||
There was a problem hiding this comment.
@sctb512 The log_to_stdout config is already restored
There was a problem hiding this comment.
Yes, it does. But the log_to_stdout entry in misc/snapshotter/config.toml is lost.
There was a problem hiding this comment.
Yes. another PR should be raised to fix the example config toml
Updates github.com/urfave/cli/v2 to v.2.24.4 which allows the usage of the
Countproperty forBoolFlag. This enables the detection of the cmdline flag being set or not and allows for correctly overriding the option from the toml config.The corresponding tests have been added as well.
Signed-off-by: Philipp Kolberg philipp.kolberg@t-online.de