nit: remove double validation on folder when register flag #2173
Conversation
Signed-off-by: SamYuan1990 <[email protected]>
|
@sthaha , I am confused with the code logic in config pkg, as there many times validation been invoked. I hope we can make it clean as a Validation function in public or a validation function as hook. To follow single response, or KISS. |
| // Validate config before run | ||
| if err := cfg.Validate(); err != nil { | ||
| logger.Error("Error validate command line flags", "error", err.Error()) | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Lets not make it the responsibility of the caller to validate the config.
| if err := cfg.Validate(); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
| hostSysFS := app.Flag(HostSysFSFlag, "Host sysfs path").Default("/sys").ExistingDir() | ||
| hostProcFS := app.Flag(HostProcFSFlag, "Host procfs path").Default("/proc").ExistingDir() |
There was a problem hiding this comment.
Lets not make it String since we must ensure the that the directory exists
but wait, if For instance, on Mac OS, |
I agree that if |
I suppose we double validate
/sysor/procfolder if we follow the default settings, or if we set up another folder other than/sysor/procwe still validate those folder during flag reg process and we have another validation function to check if the config is validated. So in this PR, just remove validation in flag reg process.