config: avoid to use default dir if root is valid #393
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #393 +/- ##
==========================================
+ Coverage 27.88% 29.13% +1.24%
==========================================
Files 40 40
Lines 4077 4074 -3
==========================================
+ Hits 1137 1187 +50
+ Misses 2801 2741 -60
- Partials 139 146 +7
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. |
cmd/containerd-nydus-grpc/main.go
Outdated
| @@ -65,6 +66,17 @@ func main() { | |||
| } | |||
| } | |||
|
|
|||
| if snapshotterConfig.Root != "" { | |||
| logConfig := &snapshotterConfig.LoggingConfig | |||
There was a problem hiding this comment.
Should these implementations be put into config.MergeConfig or defaultSnapshotterConfig.FillUpWithDefaults()?
There was a problem hiding this comment.
I think FillUpWithDefaults might not work. But putting the implementation in config.MergeConfig makes more sense.
sctb512
force-pushed
the
fix-incorrect-configuration
branch
3 times, most recently
from
5cdb7f0
to
664e4cd
Compare
Got it. Should we keep the cache dir and log dir under the root directory if the user does not specify these values. |
Yes. We should always put the log dir and cache dir under custom root dir |
config/config.go
Outdated
| cacheConfig.CacheDir = filepath.Join(to.Root, "cache") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Could we move the above code into ProcessConfigurations? I think MergeConfig is only a thin layer to merge default configurations and custom configurations. But we can tweak the merged configurations in ProcessConfigurations
|
Need rebase |
Signed-off-by: Bin Tang <tangbin.bin@bytedance.com>
Signed-off-by: Bin Tang <tangbin.bin@bytedance.com>
Signed-off-by: Bin Tang <tangbin.bin@bytedance.com>
sctb512
force-pushed
the
fix-incorrect-configuration
branch
from
664e4cd
to
fbb7a33
Compare
In the default configuration for snapshotter we fill the cacheDir and LogDir:
But we do not fill these elements after loading the configuration from the `toml' file and parsing the arguments. This results in cacheDir and LogDir not being in snapshotter's RootDir.