feature(cli): adding flag and parameter validations for commands #2600
Conversation
xoscar
requested review from
jorgeepc,
danielbdias,
schoren and
mathnogueira
and removed request for
jorgeepc and
danielbdias
| RunEnvironment installer.RunEnvironmentType | ||
| InstallationMode installer.InstallationModeType |
There was a problem hiding this comment.
With this new schema to validate our parameters, does it make sense to validate them on
https://github.com/kubeshop/tracetest/blob/main/cli/installer/types.go#L11 too?
Today they implement the Value interface on: https://github.com/spf13/pflag/blob/v1.0.5/flag.go#L187 to have an "enum-like" validation. (I forgot to document this inheritance, my bad =/)
I believe that we can simplify these types and focus on the validation here in the future.
There was a problem hiding this comment.
Yeah, I think we should focus on validations for the moment, we can update types later on
There was a problem hiding this comment.
I got your point now, the problem I see is that the validations are done one at a time, and ideally we'll want to show all possible errors at once, does it make sense?
There was a problem hiding this comment.
It makes sense! Later we can drop this support for the Value interface and focus on the validation that we do here.
cli/parameters/installer.go
Outdated
| if cmd.Flags().Lookup("run-environment").Changed && p.RunEnvironment != installer.NoneRunEnvironmentType && p.RunEnvironment != installer.DockerRunEnvironmentType && p.RunEnvironment != installer.KubernetesRunEnvironmentType { | ||
| errors = append(errors, ParamError{ | ||
| Parameter: "run-environment", | ||
| Message: "run-environment must be one of 'none', 'docker' or 'kubernetes'", | ||
| }) | ||
| } | ||
|
|
||
| if cmd.Flags().Lookup("mode").Changed && p.InstallationMode != installer.NotChosenInstallationModeType && p.InstallationMode != installer.WithDemoInstallationModeType && p.InstallationMode != installer.WithoutDemoInstallationModeType { | ||
| errors = append(errors, ParamError{ | ||
| Parameter: "mode", | ||
| Message: "mode must be one of 'not-chosen', 'with-demo' or 'just-tracetest'", | ||
| }) | ||
| } |
There was a problem hiding this comment.
Can we move this validation to a "Contains" style? Something like this:
// after the imports
var (
AllowedRunEnvironments = []installation.RunEnvironmentType{
installer.DockerRunEnvironmentType,
installer.KubernetesRunEnvironmentType,
installer.NoneRunEnvironmentType,
}
AllowedInstallationMode = []installation.InstallationModeType{
installer.WithDemoInstallationModeType,
installer.WithoutDemoInstallationModeType,
installer.NotChosenInstallationModeType,
}
)
// here on the validations
if cmd.Flags().Lookup("run-environment").Changed && !slices.Contains(AllowedRunEnvironments, p.RunEnvironment) {
// same code
}
if cmd.Flags().Lookup("mode").Changed && !slices.Contains(AllowedInstallationMode, p. InstallationMode) {
// same code
}There was a problem hiding this comment.
I didn't know about the slices lib haha thanks for pointing that out!
cli/parameters/resources.go
Outdated
| func (p *ResourceParams) Validate(cmd *cobra.Command, args []string) []ParamError { | ||
| errors := make([]ParamError, 0) | ||
|
|
||
| p.ResourceName = args[0] |
There was a problem hiding this comment.
We should validate if the resource name came in args (i.e., if len(args) > 0). Otherwise, we can have a panic here.
xoscar
deleted the
2305-cli-improvements-add-validations-for-commands-flags-and-arguments
branch
* feature(cli): adding flag and parameter validations for commands * fixing get command * PR review commentsc
This PR adds parameter based logic to validate different flags and args for each of the commands, returns all errors at once depending on the validation checks
Changes
Fixes
Checklist
https://www.loom.com/share/9efaab50749045bf9ce4557b93230806