RSDK-12892: CLI parity enable/disable resource + update component#5741
RSDK-12892: CLI parity enable/disable resource + update component#5741allisonschiang wants to merge 8 commits intoviamrobotics:mainfrom
Conversation
cli/client.go
Outdated
| if disable { | ||
| comp["disabled"] = true | ||
| } else { | ||
| delete(comp, "disabled") |
There was a problem hiding this comment.
if a resource is disabled, itll have disabled:true in config. if its enabled, itll have no disabled key in the config
cli/client.go
Outdated
| } | ||
|
|
||
| for k, v := range updates { | ||
| if rutils.IsEmptyJSONValue(v) { |
There was a problem hiding this comment.
this function will only replace, not delete or add. If its empty, I can assume the user either forgot to put a value or is trying to remove it so I'll provide the user guidance on how to move forward
There was a problem hiding this comment.
I think it's probably okay to just trust a user's choice to put an empty value as a way of zeroing out.
|
|
||
| // IsEmptyJSONValue returns true if a JSON-deserialized value is empty | ||
| // (null, empty string, empty array, or empty object). | ||
| func IsEmptyJSONValue(v any) bool { |
There was a problem hiding this comment.
there is surprisingly no built in way to see if a value is empty in go, so I added this. Theres another method in utils below that uses reflection, but would require making the input a reflect so wanted to make my own, super simple, json checker
There was a problem hiding this comment.
Why do we need this if the reflect version already exists? Creating a reflect object is as easy as reflect.ValueOf(foo). Is there some reason we shouldn't be doing so?
There was a problem hiding this comment.
isEmptyValue() also treats false and 0 as empty, which in the case of a json input could be valid
cli/app.go
Outdated
| []string{generalFlagPart, generalFlagName, generalFlagAttributes}, true, false), | ||
| Description: `Update fields on an existing resource. The --attributes flag accepts inline JSON | ||
| or a path to a JSON file. Each field you provide fully replaces the existing value for that | ||
| field. The "name" field cannot be changed |
There was a problem hiding this comment.
though it is annoying that the user will have to rewrite their current config (+ the changes) to update it(as it replaces the previous config value), its preferable to having to assume what the user wants to add/delete/replace
cli/client.go
Outdated
| var uniqueComponents []string | ||
| for _, name := range args.Component { | ||
| if seen[name] { | ||
| warningf(c.App.Writer, "duplicate component %q ignored\n", name) |
There was a problem hiding this comment.
this isn't necessary tbh since it doesn't lead to any thing wrong but it prompts the user to check if they mistyped a component name
| return err | ||
| } | ||
|
|
||
| req := apppb.UpdateRobotPartRequest{Id: part.Id, Name: part.Name, RobotConfig: pbConfig} |
There was a problem hiding this comment.
unfortunately theres no easy way to know if the config is correct, and app lets you save invalid configs (while providing immediately UI feedback that its wrong). there's no easy way to do validation before it gets sent to the backend, and no infrastructure to send responses to the CLI from the app telling the user its wrong right away. We could stream logs? maybe a future ticket? or lmk if there is an easy way!
There was a problem hiding this comment.
We should probably file a ticket for future action, but I think not providing that feedback here is fine for now!
cli/app.go
Outdated
| Action: createCommandWithT(robotsPartRemoveResourceAction), | ||
| }, | ||
| { | ||
| Name: "update-resource", |
There was a problem hiding this comment.
since we're already adding a resource subsection, I think it might make more sense for this to be viam resource update and not viam machines part update-resource.
cli/app.go
Outdated
| Subcommands: []*cli.Command{ | ||
| { | ||
| Name: "enable", | ||
| Usage: "enable components on a machine part", |
There was a problem hiding this comment.
The name of the command is resource enable but the description and flag all use component. Can we change it to resource and make sure that services are supported as well, both here and in disable command?
cli/client.go
Outdated
| } | ||
| data, err := os.ReadFile(s) //nolint:gosec // s is a user-provided path for a JSON file | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse as JSON and failed to read file %q: %w", s, err) |
There was a problem hiding this comment.
It looks like this only returns the error from reading the file, but it could be that it was never a file and the json is just malformed. Can we combine in the error from the json.Unmarshal step above as well?
cli/client.go
Outdated
| seen := make(map[string]bool, len(args.Component)) | ||
| var uniqueComponents []string |
There was a problem hiding this comment.
(minor, opt) It might be simpler to just have a uniqueComponents map and always only add to that.
cli/client.go
Outdated
| } | ||
| } | ||
|
|
||
| if _, ok := updates["name"]; ok { |
There was a problem hiding this comment.
I think this shouldn't be here. The resource config contains a name field and an optional attributes field, but this method is supposed to update the attributes field only. In theory, someone could have a meaningful name field inside a custom attributes map.
cli/client.go
Outdated
| if resource["name"] == args.Name { | ||
| resourceFound = true | ||
| for k, v := range updates { | ||
| resource[k] = v |
There was a problem hiding this comment.
I don't think this is right. We should be updating the attributes field specifically. Also this is inconsistent with the "will only replace, not delete or add" claim above. Since we're modifying the resource in-place, any existing fields that aren't in the attributes arg will remain as they are, meaning this is replacing in some cases but also additive.
| return err | ||
| } | ||
|
|
||
| req := apppb.UpdateRobotPartRequest{Id: part.Id, Name: part.Name, RobotConfig: pbConfig} |
There was a problem hiding this comment.
We should probably file a ticket for future action, but I think not providing that feedback here is fine for now!
cli/client.go
Outdated
| } | ||
|
|
||
| for k, v := range updates { | ||
| if rutils.IsEmptyJSONValue(v) { |
There was a problem hiding this comment.
I think it's probably okay to just trust a user's choice to put an empty value as a way of zeroing out.
|
|
||
| // IsEmptyJSONValue returns true if a JSON-deserialized value is empty | ||
| // (null, empty string, empty array, or empty object). | ||
| func IsEmptyJSONValue(v any) bool { |
There was a problem hiding this comment.
Why do we need this if the reflect version already exists? Creating a reflect object is as easy as reflect.ValueOf(foo). Is there some reason we shouldn't be doing so?
cli/client.go
Outdated
|
|
||
| config := part.RobotConfig.AsMap() | ||
| var updatedResources []string | ||
| for _, resourceType := range []string{"components", "services"} { |
There was a problem hiding this comment.
modules can also be enabled/disabled
There was a problem hiding this comment.
done! tested with
➜ rdk git:(RSDK-12892) ✗ ./bin/Darwin-arm64/viam-cli resource enable --part-id=a721f054-a5e7-4908-bb8b-1a8491d9df04 --resource=local-module-1
successfully enabled resource(s): local-module-1
➜ rdk git:(RSDK-12892) ✗ ./bin/Darwin-arm64/viam-cli resource disable --part-id=a721f054-a5e7-4908-bb8b-1a8491d9df04 --resource=local-module-1
successfully disabled resource(s): local-module-1
Testing:
Used AI to send a bunch of combinations of commands and then checked app to make sure it matched expected value
Enable/Disable Resource:
Details
# Missing --component flag $ viam resource disable --part 1f877dde-... Error: Required flag "component" not setMissing --part flag
$ viam resource disable --component my-sensor
Error: Required flag "part" not set
Component not found
$ viam resource disable --part 1f877dde-... --component nonexistent
Error: Component "nonexistent" not found in part config
Empty component name
$ viam resource disable --part 1f877dde-... --component ""
Error: --component value must not be empty
Duplicate component names
$ viam resource disable --part 1f877dde-... --component arm-1 --component arm-1
Warning: duplicate component "arm-1" ignored
successfully disabled component(s): arm-1
Invalid part ID
$ viam resource disable --part fake-part-id --component arm-1
no machine found for ""
rpc error: code = NotFound desc = no robot part found
Happy path: disable multiple
$ viam resource disable --part 1f877dde-... --component arm-1 --component camera-1
successfully disabled component(s): arm-1, camera-1
Happy path: enable
$ viam resource enable --part 1f877dde-... --component my-sensor
successfully enabled component(s): my-sensor
Update component with invalid inputs
Details
# 1. Missing --attributes $ viam machines part update-resource --part 1f877dde-... --name my-sensor Error: Required flag "attributes" not set2. Missing --name
$ viam machines part update-resource --part 1f877dde-... --attributes '{"model":"x"}'
Error: Required flag "name" not set
3. Missing --part
$ viam machines part update-resource --name my-sensor --attributes '{"model":"x"}'
Error: Required flag "part" not set
4. Invalid JSON
$ viam machines part update-resource --part 1f877dde-... --name my-sensor --attributes 'not-json'
Error: Failed to parse as JSON and failed to read file "not-json": open not-json: no such file or directory
5. Try to update name field
$ viam machines part update-resource --part 1f877dde-... --name my-sensor --attributes '{"name":"new-name"}'
Error: Cannot update "name" field; name is used to identify the resource
6. Resource not found
$ viam machines part update-resource --part 1f877dde-... --name nonexistent --attributes '{"model":"x"}'
Error: Resource "nonexistent" not found in part config
7. Invalid part ID
$ viam machines part update-resource --part fake-id --name my-sensor --attributes '{"model":"x"}'
no machine found for ""
rpc error: code = NotFound desc = no robot part found
8. Empty JSON object (no-op)
$ viam machines part update-resource --part 1f877dde-... --name my-sensor --attributes '{}'
successfully updated resource "my-sensor"
Empty JSON object
$ viam machines part update-resource --part 1f877dde-... --name my-sensor --attributes '{}'
Warning: no fields provided to update
Null
$ viam machines part update-resource --part 1f877dde-... --name arm-1 --attributes '{"model": null}'
Error: Field "model" has an empty value; add the value you want to update with or use the app to delete configurations
Empty string
$ viam machines part update-resource --part 1f877dde-... --name arm-1 --attributes '{"model": ""}'
Error: Field "model" has an empty value; add the value you want to update with or use the app to delete configurations
Empty list
$ viam machines part update-resource --part 1f877dde-... --name arm-1 --attributes '{"service_configs": []}'
Error: Field "service_configs" has an empty value; add the value you want to update with or use the app to delete configurations
Empty object
$ viam machines part update-resource --part 1f877dde-... --name arm-1 --attributes '{"attributes": {}}'
Error: Field "attributes" has an empty value; add the value you want to update with or use the app to delete configurations
Valid update (passes through)
$ viam machines part update-resource --part 1f877dde-... --name arm-1 --attributes '{"model": "fake"}'
successfully updated resource "arm-1"
update resources with valid inputs
Details
viam machines part update-resource --part a721f054-a5e7-4908-bb8b-1a8491d9df04 --name arm-1 --attributes '{"attributes": {"arm-model":"cli"}}' successfully updated resource "arm-1"viam machines part update-resource --part a721f054-a5e7-4908-bb8b-1a8491d9df04 --name generic-1 --attributes '{"service_configs": {"arm-model":"cli"}}'
successfully updated resource "generic-1"
I've left individual concerns/thoughts in comments (since I think its easier to get code context)