Remove some cli parameters #320
Conversation
Signed-off-by: Changwei Ge <gechangwei@bytedance.com>
The default logDir is already decided when filling up snapshotter's defaults config Signed-off-by: Changwei Ge <gechangwei@bytedance.com>
This is the second stage for build nydus-snapshotter's own configuration file Signed-off-by: Changwei Ge <gechangwei@bytedance.com>
Signed-off-by: Changwei Ge <gechangwei@bytedance.com>
Parpare for making full use of toml based config file. Signed-off-by: Changwei Ge <gechangwei@bytedance.com>
changweige
force-pushed
the
remove-more-cli-parameters
branch
2 times, most recently
from
761c9a6
to
7fd29b7
Compare
Codecov ReportBase: 30.50% // Head: 29.61% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #320 +/- ##
==========================================
- Coverage 30.50% 29.61% -0.89%
==========================================
Files 37 37
Lines 3800 3718 -82
==========================================
- Hits 1159 1101 -58
+ Misses 2520 2497 -23
+ Partials 121 120 -1
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. |
| @@ -127,31 +93,13 @@ func buildFlags(args *Args) []cli.Flag { | |||
| Usage: "whether to enable nydus-overlayfs", | |||
| Destination: &args.EnableNydusOverlayFS, | |||
| }, | |||
| &cli.BoolFlag{ | |||
| Name: "enable-stargz", | |||
There was a problem hiding this comment.
Does this impact compatibility?
There was a problem hiding this comment.
I suppose it doesn't. Stargz compatibility does not work for a long time.
There was a problem hiding this comment.
Should other flags still be reserved to ensure that snapshotter can be started if users use old flags?
There was a problem hiding this comment.
I am inclined not to reserve other parameters. We have to migrate to the configuration file pattern in the end.
This PR only removes the parameters that are not likely to be used.
There was a problem hiding this comment.
Ok, we need notice users that this is a breaking change in the release note.
| @@ -17,10 +17,7 @@ if [ "$#" -eq 0 ]; then | |||
| --root ${NYDUS_LIB} \ | |||
| --address ${NYDUS_RUN}/containerd-nydus-grpc.sock \ | |||
| --log-level ${LEVEL} \ | |||
| --enable-metrics=${ENABLE_METRICS} \ | |||
| --enable-nydus-overlayfs=${ENABLE_NYDUS_OVERLAY} \ | |||
| --daemon-mode ${NYDUSD_DAEMON_MODE} \ | |||
There was a problem hiding this comment.
Can we also remove --daemon-mode ? It seems using shared.
There was a problem hiding this comment.
multiple mode is the default one, so we should keep it?
There was a problem hiding this comment.
Ok, just keep --daemon-mode flag and use multiple by default.
There was a problem hiding this comment.
Let's change the shell variable default value to multiple
Signed-off-by: Changwei Ge <gechangwei@bytedance.com>
Nydus-snapshotter now has its own toml based configuration file whose entries are duplicated with CLI startup parameters.
In the first phase, we will remove those not frequently used cli parameters and suggest users to migrate the configuration to toml based configuration file.