RSDK-9288: declutter UI

cli/client.go

@@ -601,15 +601,16 @@ func (c *viamClient) listOAuthAppsAction(cCtx *cli.Context, orgID string) error
 	return nil
 }
 
+type listLocationsArgs struct {
+	Organization string
+}
+
 // ListLocationsAction is the corresponding Action for 'locations list'.
-func ListLocationsAction(c *cli.Context, args emptyArgs) error {
+func ListLocationsAction(c *cli.Context, args listLocationsArgs) error {
 	client, err := newViamClient(c)
 	if err != nil {
 		return err
 	}
-	// TODO(RSDK-9288) - this is brittle and inconsistent with how most data is passed.
-	// Move this to being a flag (but make sure existing workflows still work!)
-	orgStr := c.Args().First()
 	listLocations := func(orgID string) error {
 		locs, err := client.listLocations(orgID)
 		if err != nil {
@@ -620,6 +621,10 @@ func ListLocationsAction(c *cli.Context, args emptyArgs) error {
 		}
 		return nil
 	}
+	orgStr := args.Organization
+	if orgStr == "" {
+		orgStr = c.Args().First()
+	}
 	if orgStr == "" {
 		orgs, err := client.listOrganizations()
 		if err != nil {
@@ -754,14 +759,14 @@ func RobotsStatusAction(c *cli.Context, args robotsStatusArgs) error {
 
 func getNumLogs(c *cli.Context, numLogs int) (int, error) {
 	if numLogs < 0 {
-		warningf(c.App.ErrWriter, "Provided negative %q value. Defaulting to %d", logsFlagCount, defaultNumLogs)
+		warningf(c.App.ErrWriter, "Provided negative %q value. Defaulting to %d", generalFlagCount, defaultNumLogs)
 		return defaultNumLogs, nil
 	}
 	if numLogs == 0 {
 		return defaultNumLogs, nil
 	}
 	if numLogs > maxNumLogs {
-		return 0, errors.Errorf("provided too high of a %q value. Maximum is %d", logsFlagCount, maxNumLogs)
+		return 0, errors.Errorf("provided too high of a %q value. Maximum is %d", generalFlagCount, maxNumLogs)
 	}
 	return numLogs, nil
 }
@@ -1127,20 +1132,22 @@ func (c *viamClient) robotPartRestart(cCtx *cli.Context, args robotsPartRestartA
 	return nil
 }
 
-type robotsPartRunArgs struct {
+type machinesPartRunArgs struct {
 	Organization string
 	Location     string
 	Machine      string
 	Part         string
 	Data         string
 	Stream       time.Duration
+	Method       string
 }
 
-// RobotsPartRunAction is the corresponding Action for 'machines part run'.
-func RobotsPartRunAction(c *cli.Context, args robotsPartRunArgs) error {
-	// TODO(RSDK-9288) - this is brittle and inconsistent with how most data is passed.
-	// Move this to being a flag (but make sure existing workflows still work!)
-	svcMethod := c.Args().First()
+// MachinesPartRunAction is the corresponding Action for 'machines part run'.
+func MachinesPartRunAction(c *cli.Context, args machinesPartRunArgs) error {
+	svcMethod := args.Method
+	if svcMethod == "" {
+		svcMethod = c.Args().First()
+	}
 	if svcMethod == "" {
 		return errors.New("service method required")
 	}
@@ -1250,18 +1257,15 @@ func MachinesPartCopyFilesAction(c *cli.Context, args machinesPartCopyFilesArgs)
 		logger = logging.NewDebugLogger("cli")
 	}
 
-	return machinesPartCopyFilesAction(c, client, args, logger)
+	return client.machinesPartCopyFilesAction(c, args, logger)
 }
 
-func machinesPartCopyFilesAction(
-	c *cli.Context,
-	client *viamClient,
+func (c *viamClient) machinesPartCopyFilesAction(
+	ctx *cli.Context,
 	flagArgs machinesPartCopyFilesArgs,
 	logger logging.Logger,
 ) error {
-	// TODO(RSDK-9288) - this is brittle and inconsistent with how most data is passed.
-	// Move this to being a flag (but make sure existing workflows still work!)
-	args := c.Args().Slice()
+	args := ctx.Args().Slice()
 	if len(args) == 0 {
 		return errNoFiles
 	}
@@ -1307,14 +1311,14 @@ func machinesPartCopyFilesAction(
 		return err
 	}
 
-	globalArgs, err := getGlobalArgs(c)
+	globalArgs, err := getGlobalArgs(ctx)
 	if err != nil {
 		return err
 	}
 
 	doCopy := func() error {
 		if isFrom {
-			return client.copyFilesFromMachine(
+			return c.copyFilesFromMachine(
 				flagArgs.Organization,
 				flagArgs.Location,
 				flagArgs.Machine,
@@ -1328,7 +1332,7 @@ func machinesPartCopyFilesAction(
 			)
 		}
 
-		return client.copyFilesToMachine(
+		return c.copyFilesToMachine(
 			flagArgs.Organization,
 			flagArgs.Location,
 			flagArgs.Machine,

cli/client_test.go

@@ -105,7 +105,7 @@ func setup(
 
 	if dataClient != nil {
 		// these flags are only relevant when testing a dataClient
-		flags.String(dataFlagDestination, utils.ResolveFile(""), "")
+		flags.String(generalFlagDestination, utils.ResolveFile(""), "")
 	}
 
 	cCtx := cli.NewContext(NewApp(out, errOut), flags, nil)
@@ -788,7 +788,7 @@ func TestGetRobotPartLogs(t *testing.T) {
 		}
 	})
 	t.Run("max count", func(t *testing.T) {
-		flags := map[string]any{logsFlagCount: maxNumLogs}
+		flags := map[string]any{generalFlagCount: maxNumLogs}
 		cCtx, ac, out, errOut := setup(asc, nil, nil, nil, flags, "")
 
 		test.That(t, ac.robotsPartLogsAction(cCtx, parseStructFromCtx[robotsPartLogsArgs](cCtx)), test.ShouldBeNil)
@@ -889,29 +889,29 @@ func TestShellFileCopy(t *testing.T) {
 	t.Run("no arguments or files", func(t *testing.T) {
 		cCtx, viamClient, _, _ := setup(asc, nil, nil, nil, partFlags, "token")
 		test.That(t,
-			machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
+			viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
 			test.ShouldEqual, errNoFiles)
 	})
 
 	t.Run("one file path is insufficient", func(t *testing.T) {
 		args := []string{"machine:path"}
 		cCtx, viamClient, _, _ := setup(asc, nil, nil, nil, partFlags, "token", args...)
 		test.That(t,
-			machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
+			viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
 			test.ShouldEqual, errLastArgOfFromMissing)
 
 		args = []string{"path"}
 		cCtx, viamClient, _, _ = setup(asc, nil, nil, nil, partFlags, "token", args...)
 		test.That(t,
-			machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
+			viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
 			test.ShouldEqual, errLastArgOfToMissing)
 	})
 
 	t.Run("from has wrong path prefixes", func(t *testing.T) {
 		args := []string{"machine:path", "path2", "machine:path3", "destination"}
 		cCtx, viamClient, _, _ := setup(asc, nil, nil, nil, partFlags, "token", args...)
 		test.That(t,
-			machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
+			viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
 			test.ShouldHaveSameTypeAs, copyFromPathInvalidError{})
 	})
 
@@ -925,7 +925,7 @@ func TestShellFileCopy(t *testing.T) {
 			cCtx, viamClient, _, _ := setupWithRunningPart(
 				t, asc, nil, nil, partFlags, "token", partFqdn, args...)
 			test.That(t,
-				machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
+				viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
 				test.ShouldBeNil)
 
 			rd, err := os.ReadFile(filepath.Join(tempDir, filepath.Base(tfs.SingleFileNested)))
@@ -944,7 +944,7 @@ func TestShellFileCopy(t *testing.T) {
 			cCtx, viamClient, _, _ := setupWithRunningPart(
 				t, asc, nil, nil, partFlags, "token", partFqdn, args...)
 			test.That(t,
-				machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
+				viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
 				test.ShouldBeNil)
 
 			rd, err := os.ReadFile(filepath.Join(tempDir, "foo"))
@@ -960,7 +960,7 @@ func TestShellFileCopy(t *testing.T) {
 			t.Log("without recursion set")
 			cCtx, viamClient, _, _ := setupWithRunningPart(
 				t, asc, nil, nil, partFlags, "token", partFqdn, args...)
-			err := machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger)
+			err := viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger)
 			test.That(t, errors.Is(err, errDirectoryCopyRequestNoRecursion), test.ShouldBeTrue)
 			_, err = os.ReadFile(filepath.Join(tempDir, filepath.Base(tfs.SingleFileNested)))
 			test.That(t, errors.Is(err, fs.ErrNotExist), test.ShouldBeTrue)
@@ -972,7 +972,7 @@ func TestShellFileCopy(t *testing.T) {
 			cCtx, viamClient, _, _ = setupWithRunningPart(
 				t, asc, nil, nil, partFlagsCopy, "token", partFqdn, args...)
 			test.That(t,
-				machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
+				viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
 				test.ShouldBeNil)
 			test.That(t, shelltestutils.DirectoryContentsEqual(tfs.Root, filepath.Join(tempDir, filepath.Base(tfs.Root))), test.ShouldBeNil)
 		})
@@ -991,7 +991,7 @@ func TestShellFileCopy(t *testing.T) {
 			cCtx, viamClient, _, _ := setupWithRunningPart(
 				t, asc, nil, nil, partFlagsCopy, "token", partFqdn, args...)
 			test.That(t,
-				machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
+				viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
 				test.ShouldBeNil)
 
 			rd, err := os.ReadFile(filepath.Join(tempDir, filepath.Base(tfs.SingleFileNested)))
@@ -1027,7 +1027,7 @@ func TestShellFileCopy(t *testing.T) {
 					cCtx, viamClient, _, _ := setupWithRunningPart(
 						t, asc, nil, nil, partFlagsCopy, "token", partFqdn, args...)
 					test.That(t,
-						machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
+						viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
 						test.ShouldBeNil)
 					test.That(t, shelltestutils.DirectoryContentsEqual(tfs.Root, filepath.Join(tempDir, filepath.Base(tfs.Root))), test.ShouldBeNil)
 
@@ -1055,7 +1055,7 @@ func TestShellFileCopy(t *testing.T) {
 			cCtx, viamClient, _, _ := setupWithRunningPart(
 				t, asc, nil, nil, partFlags, "token", partFqdn, args...)
 			test.That(t,
-				machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
+				viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
 				test.ShouldBeNil)
 
 			rd, err := os.ReadFile(filepath.Join(tempDir, filepath.Base(tfs.SingleFileNested)))
@@ -1073,7 +1073,7 @@ func TestShellFileCopy(t *testing.T) {
 			cCtx, viamClient, _, _ := setupWithRunningPart(
 				t, asc, nil, nil, partFlags, "token", partFqdn, args...)
 			test.That(t,
-				machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
+				viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
 				test.ShouldBeNil)
 
 			rd, err := os.ReadFile(randomPath)
@@ -1089,7 +1089,7 @@ func TestShellFileCopy(t *testing.T) {
 			t.Log("without recursion set")
 			cCtx, viamClient, _, _ := setupWithRunningPart(
 				t, asc, nil, nil, partFlags, "token", partFqdn, args...)
-			err := machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger)
+			err := viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger)
 			test.That(t, errors.Is(err, errDirectoryCopyRequestNoRecursion), test.ShouldBeTrue)
 			_, err = os.ReadFile(filepath.Join(tempDir, filepath.Base(tfs.SingleFileNested)))
 			test.That(t, errors.Is(err, fs.ErrNotExist), test.ShouldBeTrue)
@@ -1101,7 +1101,7 @@ func TestShellFileCopy(t *testing.T) {
 			cCtx, viamClient, _, _ = setupWithRunningPart(
 				t, asc, nil, nil, partFlagsCopy, "token", partFqdn, args...)
 			test.That(t,
-				machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
+				viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
 				test.ShouldBeNil)
 			test.That(t, shelltestutils.DirectoryContentsEqual(tfs.Root, filepath.Join(tempDir, filepath.Base(tfs.Root))), test.ShouldBeNil)
 		})
@@ -1120,7 +1120,7 @@ func TestShellFileCopy(t *testing.T) {
 			cCtx, viamClient, _, _ := setupWithRunningPart(
 				t, asc, nil, nil, partFlagsCopy, "token", partFqdn, args...)
 			test.That(t,
-				machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
+				viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
 				test.ShouldBeNil)
 
 			rd, err := os.ReadFile(filepath.Join(tempDir, filepath.Base(tfs.SingleFileNested)))
@@ -1156,7 +1156,7 @@ func TestShellFileCopy(t *testing.T) {
 					cCtx, viamClient, _, _ := setupWithRunningPart(
 						t, asc, nil, nil, partFlagsCopy, "token", partFqdn, args...)
 					test.That(t,
-						machinesPartCopyFilesAction(cCtx, viamClient, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
+						viamClient.machinesPartCopyFilesAction(cCtx, parseStructFromCtx[machinesPartCopyFilesArgs](cCtx), logger),
 						test.ShouldBeNil)
 					test.That(t, shelltestutils.DirectoryContentsEqual(tfs.Root, filepath.Join(tempDir, filepath.Base(tfs.Root))), test.ShouldBeNil)
 

cli/dataset.go

@@ -99,7 +99,6 @@ func DatasetListAction(c *cli.Context, args datasetListArgs) error {
 	datasetIDs := args.DatasetIDs
 	orgID := args.OrgID
 
-	// TODO(RSDK-9288) - see if we can make this clearer to users from the outset
 	if orgID != "" && datasetIDs != nil {
 		return errors.New("must specify either dataset IDs or organization ID, got both")
 	}

cli/ml_training.go

@@ -356,15 +356,15 @@ func (c *viamClient) dataListTrainingJobs(orgID, status string) ([]*mltrainingpb
 }
 
 // allTrainingStatusValues returns the accepted values for the trainFlagJobStatus flag.
-func allTrainingStatusValues() string {
+func allTrainingStatusValues() []string {
 	var formattedStatuses []string
 	for status := range mltrainingpb.TrainingStatus_value {
 		formattedStatus := strings.ToLower(strings.TrimPrefix(status, trainingStatusPrefix))
 		formattedStatuses = append(formattedStatuses, formattedStatus)
 	}
 
 	slices.Sort(formattedStatuses)
-	return "[" + strings.Join(formattedStatuses, ", ") + "]"
+	return formattedStatuses
 }
 
 func defaultTrainingStatus() string {

cli/module_build.go

@@ -176,12 +176,10 @@ func ModuleBuildListAction(cCtx *cli.Context, args moduleBuildListArgs) error {
 }
 
 func (c *viamClient) moduleBuildListAction(cCtx *cli.Context, args moduleBuildListArgs) error {
-	var buildIDFilter *string
+	buildIDFilter := args.ID
 	var moduleIDFilter string
-	// This will use the build id if present and fall back on the module manifest if not
-	if cCtx.IsSet(moduleBuildFlagBuildID) {
-		buildIDFilter = &args.ID
-	} else {
+	// Fall back on the module manifest if build id is not present.
+	if buildIDFilter == "" {
 		manifestPath := args.Module
 		manifest, err := loadManifest(manifestPath)
 		if err != nil {
@@ -198,7 +196,7 @@ func (c *viamClient) moduleBuildListAction(cCtx *cli.Context, args moduleBuildLi
 		count := int32(args.Count)
 		numberOfJobsToReturn = &count
 	}
-	jobs, err := c.listModuleBuildJobs(moduleIDFilter, numberOfJobsToReturn, buildIDFilter)
+	jobs, err := c.listModuleBuildJobs(moduleIDFilter, numberOfJobsToReturn, &buildIDFilter)
 	if err != nil {
 		return err
 	}
@@ -668,7 +666,7 @@ func resolveTargetModule(c *cli.Context, manifest *moduleManifest) (*robot.Resta
 	modID := args.ID
 	// todo: use MutuallyExclusiveFlags for this when urfave/cli 3.x is stable
 	if (len(modName) > 0) && (len(modID) > 0) {
-		return nil, fmt.Errorf("provide at most one of --%s and --%s", generalFlagName, moduleBuildFlagBuildID)
+		return nil, fmt.Errorf("provide at most one of --%s and --%s", generalFlagName, moduleFlagID)
 	}
 	request := &robot.RestartModuleRequest{}
 	//nolint:gocritic
@@ -680,7 +678,7 @@ func resolveTargetModule(c *cli.Context, manifest *moduleManifest) (*robot.Resta
 		// TODO(APP-4019): remove localize call
 		request.ModuleName = localizeModuleID(manifest.ModuleID)
 	} else {
-		return nil, fmt.Errorf("if there is no meta.json, provide one of --%s or --%s", generalFlagName, moduleBuildFlagBuildID)
+		return nil, fmt.Errorf("if there is no meta.json, provide one of --%s or --%s", generalFlagName, moduleFlagID)
 	}
 	return request, nil
 }

cli/module_build_test.go

@@ -56,7 +56,7 @@ func TestStartBuild(t *testing.T) {
 		StartBuildFunc: func(ctx context.Context, in *v1.StartBuildRequest, opts ...grpc.CallOption) (*v1.StartBuildResponse, error) {
 			return &v1.StartBuildResponse{BuildId: "xyz123"}, nil
 		},
-	}, nil, map[string]any{moduleBuildFlagPath: manifest, moduleBuildFlagVersion: "1.2.3"}, "token")
+	}, nil, map[string]any{moduleFlagPath: manifest, generalFlagVersion: "1.2.3"}, "token")
 	err := ac.moduleBuildStartAction(cCtx, parseStructFromCtx[moduleBuildStartArgs](cCtx))
 	test.That(t, err, test.ShouldBeNil)
 	test.That(t, out.messages, test.ShouldHaveLength, 1)
@@ -79,7 +79,7 @@ func TestListBuild(t *testing.T) {
 				},
 			}}, nil
 		},
-	}, nil, map[string]any{moduleBuildFlagPath: manifest}, "token")
+	}, nil, map[string]any{moduleFlagPath: manifest}, "token")
 	err := ac.moduleBuildListAction(cCtx, parseStructFromCtx[moduleBuildListArgs](cCtx))
 	test.That(t, err, test.ShouldBeNil)
 	joinedOutput := strings.Join(out.messages, "")
@@ -195,7 +195,7 @@ func TestLocalBuild(t *testing.T) {
 
 	// run the build local action
 	cCtx, _, out, errOut := setup(&inject.AppServiceClient{}, nil, &inject.BuildServiceClient{},
-		nil, map[string]any{moduleBuildFlagPath: manifestPath, moduleBuildFlagVersion: "1.2.3"}, "token")
+		nil, map[string]any{moduleFlagPath: manifestPath, generalFlagVersion: "1.2.3"}, "token")
 	manifest, err := loadManifest(manifestPath)
 	test.That(t, err, test.ShouldBeNil)
 	err = moduleBuildLocalAction(cCtx, &manifest)