Skip to content

[APP-14739] get org from part for reload #5605

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gmulz wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[APP-14739] get org from part for reload #5605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1161e06
get org from part for reload
gmulz Dec 18, 2025
a0220a5
cleanup
gmulz Dec 18, 2025
3d4e074
cleanup
gmulz Dec 18, 2025
9537936
pass org id to downlod module action
gmulz Dec 18, 2025
c571c51
lint
gmulz Dec 18, 2025
c79bfc6
add tests'
gmulz Jan 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 50 additions & 18 deletions cli/module_build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,17 +784,35 @@ func (c *viamClient) ensureModuleRegisteredInCloud(
return nil
}

func (c *viamClient) inferOrgIDFromManifest(manifest ModuleManifest) (string, error) {
moduleID, err := parseModuleID(manifest.ModuleID)
func (c *viamClient) getOrgIDForPart(part *apppb.RobotPart) (string, error) {
robot, err := c.client.GetRobot(c.c.Context, &apppb.GetRobotRequest{
Id: part.GetRobot(),
})
if err != nil {
return "", err
}
org, err := getOrgByModuleIDPrefix(c, moduleID.prefix)

location, err := c.client.GetLocation(c.c.Context, &apppb.GetLocationRequest{
LocationId: robot.Robot.GetLocation(),
})
if err != nil {
return "", err
}

return org.GetId(), nil
// use the primary org id for the machine as the reload
// module org
var orgID string
for _, org := range location.Location.Organizations {
if org.Primary {
orgID = org.GetOrganizationId()
break
}
}
if orgID == "" {
orgID = location.Location.Organizations[0].GetOrganizationId()
}
Comment on lines +803 to +813
Copy link
Member

@michaellee1019 michaellee1019 Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you don't want to champion for adding primary_org_id to the API? This feels a bit fragile to put on the client side. Its hard to predict the future but if we ever make changes to locations we'd break some range of versions of the CLI.

Copy link
Member Author

@gmulz gmulz Jan 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree this way to fetch locations sucks, but was the most workable thing to start with.

are you saying add primary_org_id to the robot collection or actually populate it in the response for GetLocation?

Copy link
Member

@michaellee1019 michaellee1019 Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed this comment. The primary org is already in the DB, its just missing from the API response. Would be a very small change but a lot of coordination. I'm fine with not doing to get this merged today. We can do it as a followup. Im sure it will be useful for other use cases later to have this on the API.


return orgID, nil
}

func (c *viamClient) triggerCloudReloadBuild(
Expand All @@ -814,16 +832,10 @@ func (c *viamClient) triggerCloudReloadBuild(
return "", err
}

orgID, err := c.inferOrgIDFromManifest(manifest)
if err != nil {
return "", err
}

part, err := c.getRobotPart(partID)
if err != nil {
return "", err
}

if part.Part == nil {
return "", fmt.Errorf("part with id=%s not found", partID)
}
Expand All @@ -832,6 +844,11 @@ func (c *viamClient) triggerCloudReloadBuild(
return "", errors.New("unable to determine platform for part")
}

orgID, err := c.getOrgIDForPart(part.Part)
if err != nil {
return "", err
}

// App expects `BuildInfo` as the first request
platform := part.Part.UserSuppliedInfo.Fields["platform"].GetStringValue()
req := &buildpb.StartReloadBuildRequest{
Expand Down Expand Up @@ -901,10 +918,11 @@ func getNextReloadBuildUploadRequest(file *os.File) (*buildpb.StartReloadBuildRe

// moduleCloudBuildInfo contains information needed to download a cloud build artifact.
type moduleCloudBuildInfo struct {
ID string
ModuleID string
Version string
Platform string
ArchivePath string // Path to the temporary archive that should be deleted after download
OrgID string
}

// moduleCloudReload triggers a cloud build and returns info needed to download the artifact.
Expand All @@ -921,6 +939,18 @@ func (c *viamClient) moduleCloudReload(
return nil, err
}

part, err := c.getRobotPart(partID)
if err != nil {
return nil, err
}
if part.Part == nil {
return nil, fmt.Errorf("part with id=%s not found", partID)
}
orgID, err := c.getOrgIDForPart(part.Part)
if err != nil {
return nil, err
}

// ensure that the module has been registered in the cloud
moduleID, err := parseModuleID(manifest.ModuleID)
if err != nil {
Expand All @@ -940,11 +970,6 @@ func (c *viamClient) moduleCloudReload(
return nil, err
}

id := ctx.String(generalFlagID)
Copy link
Member Author

@gmulz gmulz Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for reload this didn't seem to be initialized ever, so it was always defaulting to "" and so would always get set to the module id. figured we'd just make it explicitly ModuleID in the moduleCloudBuildInfo

Copy link
Member

@stuqdog stuqdog Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic got leftover after a refactor I think, thanks for catching!

if id == "" {
id = manifest.ModuleID
}

if err := pm.Start("archive"); err != nil {
return nil, err
}
Expand Down Expand Up @@ -1011,13 +1036,19 @@ func (c *viamClient) moduleCloudReload(

// Return build info so the caller can download the artifact with a spinner
return &moduleCloudBuildInfo{
ID: id,
ModuleID: manifest.ModuleID,
OrgID: orgID,
Version: getReloadVersion(reloadVersionPrefix, partID),
Platform: platform,
ArchivePath: archivePath,
}, nil
}

// IsReloadVersion checks if the version is a reload version.
func IsReloadVersion(version string) bool {
return strings.HasPrefix(version, reloadVersionPrefix)
}

// ReloadModuleLocalAction builds a module locally, configures it on a robot, and starts or restarts it.
func ReloadModuleLocalAction(c *cli.Context, args reloadModuleArgs) error {
return reloadModuleAction(c, args, false)
Expand Down Expand Up @@ -1174,7 +1205,8 @@ func reloadModuleActionInner(
return err
}
downloadArgs := downloadModuleFlags{
ID: buildInfo.ID,
ModuleID: buildInfo.ModuleID,
OrgID: buildInfo.OrgID,
Version: buildInfo.Version,
Platform: buildInfo.Platform,
Destination: ".",
Expand Down
190 changes: 190 additions & 0 deletions cli/module_build_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"time"

v1 "go.viam.com/api/app/build/v1"
apppb "go.viam.com/api/app/v1"
"go.viam.com/test"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
Expand Down Expand Up @@ -480,3 +481,192 @@ func TestRetryableCopyToPart(t *testing.T) {
test.That(t, errMsg, test.ShouldContainSubstring, "run the RDK as root")
})
}

func TestIsReloadVersion(t *testing.T) {
tests := []struct {
name string
version string
expected bool
}{
{
name: "reload version with part ID",
version: "reload-abc123",
expected: true,
},
{
name: "reload version simple",
version: "reload",
expected: true,
},
{
name: "reload-source version",
version: "reload-source-abc123",
expected: true,
},
{
name: "normal semver version",
version: "1.2.3",
expected: false,
},
{
name: "latest version",
version: "latest",
expected: false,
},
{
name: "empty version",
version: "",
expected: false,
},
{
name: "version containing reload but not prefix",
version: "v1.0.0-reload",
expected: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := IsReloadVersion(tt.version)
test.That(t, result, test.ShouldEqual, tt.expected)
})
}
}

func TestGetOrgIDForPart(t *testing.T) {
t.Run("returns primary org ID", func(t *testing.T) {
expectedOrgID := "primary-org-123"
secondaryOrgID := "secondary-org-456"
robotID := "robot-abc"
locationID := "location-xyz"

mockClient := &inject.AppServiceClient{
GetRobotFunc: func(ctx context.Context, req *apppb.GetRobotRequest,
opts ...grpc.CallOption,
) (*apppb.GetRobotResponse, error) {
return &apppb.GetRobotResponse{
Robot: &apppb.Robot{
Id: robotID,
Location: locationID,
},
}, nil
},
GetLocationFunc: func(ctx context.Context, req *apppb.GetLocationRequest,
opts ...grpc.CallOption,
) (*apppb.GetLocationResponse, error) {
test.That(t, req.LocationId, test.ShouldEqual, locationID)
return &apppb.GetLocationResponse{
Location: &apppb.Location{
Id: locationID,
Organizations: []*apppb.LocationOrganization{
{OrganizationId: secondaryOrgID, Primary: false},
{OrganizationId: expectedOrgID, Primary: true},
},
},
}, nil
},
}

_, vc, _, _ := setup(mockClient, nil, &inject.BuildServiceClient{}, map[string]any{}, "token")

part := &apppb.RobotPart{
Robot: robotID,
}
orgID, err := vc.getOrgIDForPart(part)
test.That(t, err, test.ShouldBeNil)
test.That(t, orgID, test.ShouldEqual, expectedOrgID)
})

t.Run("falls back to first org when no primary", func(t *testing.T) {
firstOrgID := "first-org-123"
secondOrgID := "second-org-456"
robotID := "robot-abc"
locationID := "location-xyz"

mockClient := &inject.AppServiceClient{
GetRobotFunc: func(ctx context.Context, req *apppb.GetRobotRequest,
opts ...grpc.CallOption,
) (*apppb.GetRobotResponse, error) {
return &apppb.GetRobotResponse{
Robot: &apppb.Robot{
Id: robotID,
Location: locationID,
},
}, nil
},
GetLocationFunc: func(ctx context.Context, req *apppb.GetLocationRequest,
opts ...grpc.CallOption,
) (*apppb.GetLocationResponse, error) {
return &apppb.GetLocationResponse{
Location: &apppb.Location{
Id: locationID,
Organizations: []*apppb.LocationOrganization{
{OrganizationId: firstOrgID, Primary: false},
{OrganizationId: secondOrgID, Primary: false},
},
},
}, nil
},
}

_, vc, _, _ := setup(mockClient, nil, &inject.BuildServiceClient{}, map[string]any{}, "token")

part := &apppb.RobotPart{
Robot: robotID,
}
orgID, err := vc.getOrgIDForPart(part)
test.That(t, err, test.ShouldBeNil)
test.That(t, orgID, test.ShouldEqual, firstOrgID)
})

t.Run("returns error when GetRobot fails", func(t *testing.T) {
mockClient := &inject.AppServiceClient{
GetRobotFunc: func(ctx context.Context, req *apppb.GetRobotRequest,
opts ...grpc.CallOption,
) (*apppb.GetRobotResponse, error) {
return nil, errors.New("robot not found")
},
}

_, vc, _, _ := setup(mockClient, nil, &inject.BuildServiceClient{}, map[string]any{}, "token")

part := &apppb.RobotPart{
Robot: "robot-abc",
}
_, err := vc.getOrgIDForPart(part)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, "robot not found")
})

t.Run("returns error when GetLocation fails", func(t *testing.T) {
robotID := "robot-abc"
locationID := "location-xyz"

mockClient := &inject.AppServiceClient{
GetRobotFunc: func(ctx context.Context, req *apppb.GetRobotRequest,
opts ...grpc.CallOption,
) (*apppb.GetRobotResponse, error) {
return &apppb.GetRobotResponse{
Robot: &apppb.Robot{
Id: robotID,
Location: locationID,
},
}, nil
},
GetLocationFunc: func(ctx context.Context, req *apppb.GetLocationRequest,
opts ...grpc.CallOption,
) (*apppb.GetLocationResponse, error) {
return nil, errors.New("location not found")
},
}

_, vc, _, _ := setup(mockClient, nil, &inject.BuildServiceClient{}, map[string]any{}, "token")

part := &apppb.RobotPart{
Robot: robotID,
}
_, err := vc.getOrgIDForPart(part)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, "location not found")
})
}
Loading