-
Notifications
You must be signed in to change notification settings - Fork 127
[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.
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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() | ||
| } | ||
|
|
||
| return orgID, nil | ||
| } | ||
|
|
||
| func (c *viamClient) triggerCloudReloadBuild( | ||
|
|
@@ -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) | ||
| } | ||
|
|
@@ -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{ | ||
|
|
@@ -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. | ||
|
|
@@ -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 { | ||
|
|
@@ -940,11 +970,6 @@ func (c *viamClient) moduleCloudReload( | |
| return nil, err | ||
| } | ||
|
|
||
| id := ctx.String(generalFlagID) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| } | ||
|
|
@@ -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) | ||
|
|
@@ -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: ".", | ||
|
|
||
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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_idto the robot collection or actually populate it in the response for GetLocation?