Add resource generic load, unload, status to CLI #4660
Conversation
| } | ||
| return "", "", nil, false, err | ||
| } | ||
| // Get the resource kind nad original bytes |
There was a problem hiding this comment.
| // Get the resource kind nad original bytes | |
| // Get the resource kind and original bytes |
operator/pkg/cli/generic.go
Outdated
| return dec | ||
| } | ||
|
|
||
| func getNextResource(dec *yaml2.YAMLOrJSONDecoder) (string, string, []byte, bool, error) { |
There was a problem hiding this comment.
too many returns and it is hard to know what we are getting back. Consider adding at least description.
operator/pkg/cli/generic.go
Outdated
| "os" | ||
|
|
||
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| yaml2 "k8s.io/apimachinery/pkg/util/yaml" |
operator/pkg/cli/generic.go
Outdated
| func (sc *SchedulerClient) Load(data []byte, showRequest bool, showResponse bool) error { | ||
| dec := createDecoder(data) | ||
| for { | ||
| kind, _, data, keepGoing, err := getNextResource(dec) |
There was a problem hiding this comment.
we are also not checking err before the switch
operator/pkg/cli/generic.go
Outdated
| if !keepGoing { | ||
| return err | ||
| } | ||
| switch kind { |
There was a problem hiding this comment.
should we add a warning if it is not part of these 3 resources? should we stop?
operator/pkg/cli/generic.go
Outdated
| func (sc *SchedulerClient) Unload(data []byte, showRequest bool, showResponse bool) error { | ||
| dec := createDecoder(data) | ||
| for { | ||
| kind, _, data, keepGoing, err := getNextResource(dec) |
operator/pkg/cli/generic.go
Outdated
| } | ||
| err = sc.PipelineStatus(name, showRequest, showResponse, waitCondition) | ||
| case "Experiment": | ||
| err = sc.ExperimentStatus(name, showRequest, showResponse, wait) |
There was a problem hiding this comment.
the bool is passed directly to function - experiments don't take a string waitCondition
operator/pkg/cli/generic.go
Outdated
| } | ||
| waitCondition := "" | ||
| switch kind { | ||
| case "Model": |
There was a problem hiding this comment.
consider extracting the values of the switch into enum as it is used in 3 different places already.
| var rootCmd = &cobra.Command{Use: "seldon", SilenceErrors: false, SilenceUsage: true} | ||
|
|
||
| rootCmd.DisableAutoGenTag = true | ||
|
|
||
| rootCmd.AddCommand(cmdModel, cmdServer, cmdExperiment, cmdPipeline, cmdConfig) | ||
| rootCmd.AddCommand(cmdModel, cmdServer, cmdExperiment, cmdPipeline, cmdConfig, cmdLoad, cmdUnload, cmdStatus) |
There was a problem hiding this comment.
just to keep consistency with previous usage (i.e. seldon model load and seldon pipeline load), an alternative is perhaps to use
seldon all load? I am personally not entirely sure which approach is better so happy to leave it as is.
There was a problem hiding this comment.
seems a bit verbose - maybe will leave for now
Examples:
Run kustomize to create a set of Seldon Core V2 artifacts and pipe to load:
kustomize build config | seldon loadAlso, wait for a set of resources to be ready
kustomize build config | seldon status -wA future PR can also allow for
-fto take a folder rather than a file like kubectl.