Skip to content

Commit

Permalink
RSDK-9288: declutter UI (#4692)
Browse files Browse the repository at this point in the history
  • Loading branch information
@purplenicole730
purplenicole730 authored Jan 16, 2025
1 parent b6b0a96 commit 61f77ed
Show file tree
Hide file tree
Showing 10 changed files with 619 additions and 544 deletions.
1,030 changes: 555 additions & 475 deletions cli/app.go
View file

Large diffs are not rendered by default.

48 changes: 26 additions & 22 deletions cli/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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,
Expand All @@ -1328,7 +1332,7 @@ func machinesPartCopyFilesAction(
)
}

return client.copyFilesToMachine(
return c.copyFilesToMachine(
flagArgs.Organization,
flagArgs.Location,
flagArgs.Machine,
Expand Down
36 changes: 18 additions & 18 deletions cli/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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{})
})

Expand All @@ -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)))
Expand All @@ -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"))
Expand All @@ -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)
Expand All @@ -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)
})
Expand All @@ -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)))
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)))
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
})
Expand All @@ -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)))
Expand Down Expand Up @@ -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)

Expand Down
1 change: 0 additions & 1 deletion cli/dataset.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
4 changes: 2 additions & 2 deletions cli/ml_training.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
14 changes: 6 additions & 8 deletions cli/module_build.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down Expand Up @@ -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
Expand All @@ -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
}
Expand Down
6 changes: 3 additions & 3 deletions cli/module_build_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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, "")
Expand Down Expand Up @@ -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)
Expand Down
Loading

0 comments on commit 61f77ed

Please sign in to comment.