Skip to content

Commit 61f77ed

Browse files
RSDK-9288: declutter UI (viamrobotics#4692)
1 parent b6b0a96 commit 61f77ed

10 files changed

+619
-544
lines changed

cli/app.go

+555-475
Large diffs are not rendered by default.

cli/client.go

+26-22
Original file line numberDiff line numberDiff line change
@@ -601,15 +601,16 @@ func (c *viamClient) listOAuthAppsAction(cCtx *cli.Context, orgID string) error
601601
return nil
602602
}
603603

604+
type listLocationsArgs struct {
605+
Organization string
606+
}
607+
604608
// ListLocationsAction is the corresponding Action for 'locations list'.
605-
func ListLocationsAction(c *cli.Context, args emptyArgs) error {
609+
func ListLocationsAction(c *cli.Context, args listLocationsArgs) error {
606610
client, err := newViamClient(c)
607611
if err != nil {
608612
return err
609613
}
610-
// TODO(RSDK-9288) - this is brittle and inconsistent with how most data is passed.
611-
// Move this to being a flag (but make sure existing workflows still work!)
612-
orgStr := c.Args().First()
613614
listLocations := func(orgID string) error {
614615
locs, err := client.listLocations(orgID)
615616
if err != nil {
@@ -620,6 +621,10 @@ func ListLocationsAction(c *cli.Context, args emptyArgs) error {
620621
}
621622
return nil
622623
}
624+
orgStr := args.Organization
625+
if orgStr == "" {
626+
orgStr = c.Args().First()
627+
}
623628
if orgStr == "" {
624629
orgs, err := client.listOrganizations()
625630
if err != nil {
@@ -754,14 +759,14 @@ func RobotsStatusAction(c *cli.Context, args robotsStatusArgs) error {
754759

755760
func getNumLogs(c *cli.Context, numLogs int) (int, error) {
756761
if numLogs < 0 {
757-
warningf(c.App.ErrWriter, "Provided negative %q value. Defaulting to %d", logsFlagCount, defaultNumLogs)
762+
warningf(c.App.ErrWriter, "Provided negative %q value. Defaulting to %d", generalFlagCount, defaultNumLogs)
758763
return defaultNumLogs, nil
759764
}
760765
if numLogs == 0 {
761766
return defaultNumLogs, nil
762767
}
763768
if numLogs > maxNumLogs {
764-
return 0, errors.Errorf("provided too high of a %q value. Maximum is %d", logsFlagCount, maxNumLogs)
769+
return 0, errors.Errorf("provided too high of a %q value. Maximum is %d", generalFlagCount, maxNumLogs)
765770
}
766771
return numLogs, nil
767772
}
@@ -1127,20 +1132,22 @@ func (c *viamClient) robotPartRestart(cCtx *cli.Context, args robotsPartRestartA
11271132
return nil
11281133
}
11291134

1130-
type robotsPartRunArgs struct {
1135+
type machinesPartRunArgs struct {
11311136
Organization string
11321137
Location string
11331138
Machine string
11341139
Part string
11351140
Data string
11361141
Stream time.Duration
1142+
Method string
11371143
}
11381144

1139-
// RobotsPartRunAction is the corresponding Action for 'machines part run'.
1140-
func RobotsPartRunAction(c *cli.Context, args robotsPartRunArgs) error {
1141-
// TODO(RSDK-9288) - this is brittle and inconsistent with how most data is passed.
1142-
// Move this to being a flag (but make sure existing workflows still work!)
1143-
svcMethod := c.Args().First()
1145+
// MachinesPartRunAction is the corresponding Action for 'machines part run'.
1146+
func MachinesPartRunAction(c *cli.Context, args machinesPartRunArgs) error {
1147+
svcMethod := args.Method
1148+
if svcMethod == "" {
1149+
svcMethod = c.Args().First()
1150+
}
11441151
if svcMethod == "" {
11451152
return errors.New("service method required")
11461153
}
@@ -1250,18 +1257,15 @@ func MachinesPartCopyFilesAction(c *cli.Context, args machinesPartCopyFilesArgs)
12501257
logger = logging.NewDebugLogger("cli")
12511258
}
12521259

1253-
return machinesPartCopyFilesAction(c, client, args, logger)
1260+
return client.machinesPartCopyFilesAction(c, args, logger)
12541261
}
12551262

1256-
func machinesPartCopyFilesAction(
1257-
c *cli.Context,
1258-
client *viamClient,
1263+
func (c *viamClient) machinesPartCopyFilesAction(
1264+
ctx *cli.Context,
12591265
flagArgs machinesPartCopyFilesArgs,
12601266
logger logging.Logger,
12611267
) error {
1262-
// TODO(RSDK-9288) - this is brittle and inconsistent with how most data is passed.
1263-
// Move this to being a flag (but make sure existing workflows still work!)
1264-
args := c.Args().Slice()
1268+
args := ctx.Args().Slice()
12651269
if len(args) == 0 {
12661270
return errNoFiles
12671271
}
@@ -1307,14 +1311,14 @@ func machinesPartCopyFilesAction(
13071311
return err
13081312
}
13091313

1310-
globalArgs, err := getGlobalArgs(c)
1314+
globalArgs, err := getGlobalArgs(ctx)
13111315
if err != nil {
13121316
return err
13131317
}
13141318

13151319
doCopy := func() error {
13161320
if isFrom {
1317-
return client.copyFilesFromMachine(
1321+
return c.copyFilesFromMachine(
13181322
flagArgs.Organization,
13191323
flagArgs.Location,
13201324
flagArgs.Machine,
@@ -1328,7 +1332,7 @@ func machinesPartCopyFilesAction(
13281332
)
13291333
}
13301334

1331-
return client.copyFilesToMachine(
1335+
return c.copyFilesToMachine(
13321336
flagArgs.Organization,
13331337
flagArgs.Location,
13341338
flagArgs.Machine,

cli/client_test.go

+18-18
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ func setup(
105105

106106
if dataClient != nil {
107107
// these flags are only relevant when testing a dataClient
108-
flags.String(dataFlagDestination, utils.ResolveFile(""), "")
108+
flags.String(generalFlagDestination, utils.ResolveFile(""), "")
109109
}
110110

111111
cCtx := cli.NewContext(NewApp(out, errOut), flags, nil)
@@ -788,7 +788,7 @@ func TestGetRobotPartLogs(t *testing.T) {
788788
}
789789
})
790790
t.Run("max count", func(t *testing.T) {
791-
flags := map[string]any{logsFlagCount: maxNumLogs}
791+
flags := map[string]any{generalFlagCount: maxNumLogs}
792792
cCtx, ac, out, errOut := setup(asc, nil, nil, nil, flags, "")
793793

794794
test.That(t, ac.robotsPartLogsAction(cCtx, parseStructFromCtx[robotsPartLogsArgs](cCtx)), test.ShouldBeNil)
@@ -889,29 +889,29 @@ func TestShellFileCopy(t *testing.T) {
889889
t.Run("no arguments or files", func(t *testing.T) {
890890
cCtx, viamClient, _, _ := setup(asc, nil, nil, nil, partFlags, "token")
891891
test.That(t,
892-
machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
892+
viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
893893
test.ShouldEqual, errNoFiles)
894894
})
895895

896896
t.Run("one file path is insufficient", func(t *testing.T) {
897897
args := []string{"machine:path"}
898898
cCtx, viamClient, _, _ := setup(asc, nil, nil, nil, partFlags, "token", args...)
899899
test.That(t,
900-
machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
900+
viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
901901
test.ShouldEqual, errLastArgOfFromMissing)
902902

903903
args = []string{"path"}
904904
cCtx, viamClient, _, _ = setup(asc, nil, nil, nil, partFlags, "token", args...)
905905
test.That(t,
906-
machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
906+
viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
907907
test.ShouldEqual, errLastArgOfToMissing)
908908
})
909909

910910
t.Run("from has wrong path prefixes", func(t *testing.T) {
911911
args := []string{"machine:path", "path2", "machine:path3", "destination"}
912912
cCtx, viamClient, _, _ := setup(asc, nil, nil, nil, partFlags, "token", args...)
913913
test.That(t,
914-
machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
914+
viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
915915
test.ShouldHaveSameTypeAs, copyFromPathInvalidError{})
916916
})
917917

@@ -925,7 +925,7 @@ func TestShellFileCopy(t *testing.T) {
925925
cCtx, viamClient, _, _ := setupWithRunningPart(
926926
t, asc, nil, nil, partFlags, "token", partFqdn, args...)
927927
test.That(t,
928-
machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
928+
viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
929929
test.ShouldBeNil)
930930

931931
rd, err := os.ReadFile(filepath.Join(tempDir, filepath.Base(tfs.SingleFileNested)))
@@ -944,7 +944,7 @@ func TestShellFileCopy(t *testing.T) {
944944
cCtx, viamClient, _, _ := setupWithRunningPart(
945945
t, asc, nil, nil, partFlags, "token", partFqdn, args...)
946946
test.That(t,
947-
machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
947+
viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
948948
test.ShouldBeNil)
949949

950950
rd, err := os.ReadFile(filepath.Join(tempDir, "foo"))
@@ -960,7 +960,7 @@ func TestShellFileCopy(t *testing.T) {
960960
t.Log("without recursion set")
961961
cCtx, viamClient, _, _ := setupWithRunningPart(
962962
t, asc, nil, nil, partFlags, "token", partFqdn, args...)
963-
err := machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger)
963+
err := viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger)
964964
test.That(t, errors.Is(err, errDirectoryCopyRequestNoRecursion), test.ShouldBeTrue)
965965
_, err = os.ReadFile(filepath.Join(tempDir, filepath.Base(tfs.SingleFileNested)))
966966
test.That(t, errors.Is(err, fs.ErrNotExist), test.ShouldBeTrue)
@@ -972,7 +972,7 @@ func TestShellFileCopy(t *testing.T) {
972972
cCtx, viamClient, _, _ = setupWithRunningPart(
973973
t, asc, nil, nil, partFlagsCopy, "token", partFqdn, args...)
974974
test.That(t,
975-
machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
975+
viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
976976
test.ShouldBeNil)
977977
test.That(t, shelltestutils.DirectoryContentsEqual(tfs.Root, filepath.Join(tempDir, filepath.Base(tfs.Root))), test.ShouldBeNil)
978978
})
@@ -991,7 +991,7 @@ func TestShellFileCopy(t *testing.T) {
991991
cCtx, viamClient, _, _ := setupWithRunningPart(
992992
t, asc, nil, nil, partFlagsCopy, "token", partFqdn, args...)
993993
test.That(t,
994-
machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
994+
viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
995995
test.ShouldBeNil)
996996

997997
rd, err := os.ReadFile(filepath.Join(tempDir, filepath.Base(tfs.SingleFileNested)))
@@ -1027,7 +1027,7 @@ func TestShellFileCopy(t *testing.T) {
10271027
cCtx, viamClient, _, _ := setupWithRunningPart(
10281028
t, asc, nil, nil, partFlagsCopy, "token", partFqdn, args...)
10291029
test.That(t,
1030-
machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
1030+
viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
10311031
test.ShouldBeNil)
10321032
test.That(t, shelltestutils.DirectoryContentsEqual(tfs.Root, filepath.Join(tempDir, filepath.Base(tfs.Root))), test.ShouldBeNil)
10331033

@@ -1055,7 +1055,7 @@ func TestShellFileCopy(t *testing.T) {
10551055
cCtx, viamClient, _, _ := setupWithRunningPart(
10561056
t, asc, nil, nil, partFlags, "token", partFqdn, args...)
10571057
test.That(t,
1058-
machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
1058+
viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
10591059
test.ShouldBeNil)
10601060

10611061
rd, err := os.ReadFile(filepath.Join(tempDir, filepath.Base(tfs.SingleFileNested)))
@@ -1073,7 +1073,7 @@ func TestShellFileCopy(t *testing.T) {
10731073
cCtx, viamClient, _, _ := setupWithRunningPart(
10741074
t, asc, nil, nil, partFlags, "token", partFqdn, args...)
10751075
test.That(t,
1076-
machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
1076+
viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
10771077
test.ShouldBeNil)
10781078

10791079
rd, err := os.ReadFile(randomPath)
@@ -1089,7 +1089,7 @@ func TestShellFileCopy(t *testing.T) {
10891089
t.Log("without recursion set")
10901090
cCtx, viamClient, _, _ := setupWithRunningPart(
10911091
t, asc, nil, nil, partFlags, "token", partFqdn, args...)
1092-
err := machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger)
1092+
err := viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger)
10931093
test.That(t, errors.Is(err, errDirectoryCopyRequestNoRecursion), test.ShouldBeTrue)
10941094
_, err = os.ReadFile(filepath.Join(tempDir, filepath.Base(tfs.SingleFileNested)))
10951095
test.That(t, errors.Is(err, fs.ErrNotExist), test.ShouldBeTrue)
@@ -1101,7 +1101,7 @@ func TestShellFileCopy(t *testing.T) {
11011101
cCtx, viamClient, _, _ = setupWithRunningPart(
11021102
t, asc, nil, nil, partFlagsCopy, "token", partFqdn, args...)
11031103
test.That(t,
1104-
machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
1104+
viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
11051105
test.ShouldBeNil)
11061106
test.That(t, shelltestutils.DirectoryContentsEqual(tfs.Root, filepath.Join(tempDir, filepath.Base(tfs.Root))), test.ShouldBeNil)
11071107
})
@@ -1120,7 +1120,7 @@ func TestShellFileCopy(t *testing.T) {
11201120
cCtx, viamClient, _, _ := setupWithRunningPart(
11211121
t, asc, nil, nil, partFlagsCopy, "token", partFqdn, args...)
11221122
test.That(t,
1123-
machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
1123+
viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
11241124
test.ShouldBeNil)
11251125

11261126
rd, err := os.ReadFile(filepath.Join(tempDir, filepath.Base(tfs.SingleFileNested)))
@@ -1156,7 +1156,7 @@ func TestShellFileCopy(t *testing.T) {
11561156
cCtx, viamClient, _, _ := setupWithRunningPart(
11571157
t, asc, nil, nil, partFlagsCopy, "token", partFqdn, args...)
11581158
test.That(t,
1159-
machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
1159+
viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
11601160
test.ShouldBeNil)
11611161
test.That(t, shelltestutils.DirectoryContentsEqual(tfs.Root, filepath.Join(tempDir, filepath.Base(tfs.Root))), test.ShouldBeNil)
11621162

cli/dataset.go

-1
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,6 @@ func DatasetListAction(c *cli.Context, args datasetListArgs) error {
9999
datasetIDs := args.DatasetIDs
100100
orgID := args.OrgID
101101

102-
// TODO(RSDK-9288) - see if we can make this clearer to users from the outset
103102
if orgID != "" && datasetIDs != nil {
104103
return errors.New("must specify either dataset IDs or organization ID, got both")
105104
}

cli/ml_training.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -356,15 +356,15 @@ func (c *viamClient) dataListTrainingJobs(orgID, status string) ([]*mltrainingpb
356356
}
357357

358358
// allTrainingStatusValues returns the accepted values for the trainFlagJobStatus flag.
359-
func allTrainingStatusValues() string {
359+
func allTrainingStatusValues() []string {
360360
var formattedStatuses []string
361361
for status := range mltrainingpb.TrainingStatus_value {
362362
formattedStatus := strings.ToLower(strings.TrimPrefix(status, trainingStatusPrefix))
363363
formattedStatuses = append(formattedStatuses, formattedStatus)
364364
}
365365

366366
slices.Sort(formattedStatuses)
367-
return "[" + strings.Join(formattedStatuses, ", ") + "]"
367+
return formattedStatuses
368368
}
369369

370370
func defaultTrainingStatus() string {

cli/module_build.go

+6-8
Original file line numberDiff line numberDiff line change
@@ -176,12 +176,10 @@ func ModuleBuildListAction(cCtx *cli.Context, args moduleBuildListArgs) error {
176176
}
177177

178178
func (c *viamClient) moduleBuildListAction(cCtx *cli.Context, args moduleBuildListArgs) error {
179-
var buildIDFilter *string
179+
buildIDFilter := args.ID
180180
var moduleIDFilter string
181-
// This will use the build id if present and fall back on the module manifest if not
182-
if cCtx.IsSet(moduleBuildFlagBuildID) {
183-
buildIDFilter = &args.ID
184-
} else {
181+
// Fall back on the module manifest if build id is not present.
182+
if buildIDFilter == "" {
185183
manifestPath := args.Module
186184
manifest, err := loadManifest(manifestPath)
187185
if err != nil {
@@ -198,7 +196,7 @@ func (c *viamClient) moduleBuildListAction(cCtx *cli.Context, args moduleBuildLi
198196
count := int32(args.Count)
199197
numberOfJobsToReturn = &count
200198
}
201-
jobs, err := c.listModuleBuildJobs(moduleIDFilter, numberOfJobsToReturn, buildIDFilter)
199+
jobs, err := c.listModuleBuildJobs(moduleIDFilter, numberOfJobsToReturn, &buildIDFilter)
202200
if err != nil {
203201
return err
204202
}
@@ -668,7 +666,7 @@ func resolveTargetModule(c *cli.Context, manifest *moduleManifest) (*robot.Resta
668666
modID := args.ID
669667
// todo: use MutuallyExclusiveFlags for this when urfave/cli 3.x is stable
670668
if (len(modName) > 0) && (len(modID) > 0) {
671-
return nil, fmt.Errorf("provide at most one of --%s and --%s", generalFlagName, moduleBuildFlagBuildID)
669+
return nil, fmt.Errorf("provide at most one of --%s and --%s", generalFlagName, moduleFlagID)
672670
}
673671
request := &robot.RestartModuleRequest{}
674672
//nolint:gocritic
@@ -680,7 +678,7 @@ func resolveTargetModule(c *cli.Context, manifest *moduleManifest) (*robot.Resta
680678
// TODO(APP-4019): remove localize call
681679
request.ModuleName = localizeModuleID(manifest.ModuleID)
682680
} else {
683-
return nil, fmt.Errorf("if there is no meta.json, provide one of --%s or --%s", generalFlagName, moduleBuildFlagBuildID)
681+
return nil, fmt.Errorf("if there is no meta.json, provide one of --%s or --%s", generalFlagName, moduleFlagID)
684682
}
685683
return request, nil
686684
}

cli/module_build_test.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ func TestStartBuild(t *testing.T) {
5656
StartBuildFunc: func(ctx context.Context, in *v1.StartBuildRequest, opts ...grpc.CallOption) (*v1.StartBuildResponse, error) {
5757
return &v1.StartBuildResponse{BuildId: "xyz123"}, nil
5858
},
59-
}, nil, map[string]any{moduleBuildFlagPath: manifest, moduleBuildFlagVersion: "1.2.3"}, "token")
59+
}, nil, map[string]any{moduleFlagPath: manifest, generalFlagVersion: "1.2.3"}, "token")
6060
err := ac.moduleBuildStartAction(cCtx, parseStructFromCtx[moduleBuildStartArgs](cCtx))
6161
test.That(t, err, test.ShouldBeNil)
6262
test.That(t, out.messages, test.ShouldHaveLength, 1)
@@ -79,7 +79,7 @@ func TestListBuild(t *testing.T) {
7979
},
8080
}}, nil
8181
},
82-
}, nil, map[string]any{moduleBuildFlagPath: manifest}, "token")
82+
}, nil, map[string]any{moduleFlagPath: manifest}, "token")
8383
err := ac.moduleBuildListAction(cCtx, parseStructFromCtx[moduleBuildListArgs](cCtx))
8484
test.That(t, err, test.ShouldBeNil)
8585
joinedOutput := strings.Join(out.messages, "")
@@ -195,7 +195,7 @@ func TestLocalBuild(t *testing.T) {
195195

196196
// run the build local action
197197
cCtx, _, out, errOut := setup(&inject.AppServiceClient{}, nil, &inject.BuildServiceClient{},
198-
nil, map[string]any{moduleBuildFlagPath: manifestPath, moduleBuildFlagVersion: "1.2.3"}, "token")
198+
nil, map[string]any{moduleFlagPath: manifestPath, generalFlagVersion: "1.2.3"}, "token")
199199
manifest, err := loadManifest(manifestPath)
200200
test.That(t, err, test.ShouldBeNil)
201201
err = moduleBuildLocalAction(cCtx, &manifest)

0 commit comments

Comments
 (0)