RSDK-9174: add app wrappers #4561
Conversation
viambot
added
the
safe to test
viambot
added
safe to test
There was a problem hiding this comment.
Echoing @benjirewis, thank you so much for putting in all this work! I'll admit I skimmed the portions that were fairly boilerplate-y, but this generally looks really good! Just a couple mostly small changes requested.
app/data_client.go
Outdated
| @@ -177,7 +177,7 @@ type DatabaseConnReturn struct { | |||
| HasDatabaseUser bool | |||
| } | |||
|
|
|||
| // NewDataClient constructs a new DataClient using the connection passed in by the viamClient. | |||
| // NewDataClient constructs a new DataClient using the connection passed in by the Viam client. | |||
There was a problem hiding this comment.
(minor/nit) we should probably go with ViamClient here since that's the name of the type.
app/app_proto_conversions.go
Outdated
| // `role`` must be "owner" or "operator" | ||
| role string | ||
| // `resourceType` must be "organization", "location", or "robot" |
There was a problem hiding this comment.
If it's possible that a user constructs an APIKeyAuthorization directly (which I believe it is, given that it's a public struct), we probably shouldn't give them the option to pass the wrong value here. I realize we've already got a lot of types, but I'd consider adding AppAuthRole and AppResourceType enums that constrain what a user is allowed to say to valid inputs only.
app/app_proto_conversions.go
Outdated
| resourceID string | ||
| } | ||
|
|
||
| func createAuthorization(orgID, identityID, identityType, role, resourceType, resourceID string) (*pb.Authorization, error) { |
There was a problem hiding this comment.
I'm a bit split on this one but I wonder if we should ask for an APIKeyAuthorization here instead of asking for role, resourceType, and resourceID as separate fields. It's not necessarily for creating an API key so we'd probably want to rename that struct, but it might simplify the user experience.
If we opt not to do that (and I'll defer to your opinion on this one), I think we should be sure to switch the types we ask for here to prevent invalid inputs (see above comment).
There was a problem hiding this comment.
Hmm... That's a good point. Every time I think I settled on a decision, I change it.
I think sticking with APIKeyAuthorization is the move because I can't tell why these specific three would be grouped other than because that's what you specifically need for an API key. But yes, I will change the types.
app/registry_proto_conversions.go
Outdated
| } | ||
| } | ||
|
|
||
| // Model has the API and model of a model. |
There was a problem hiding this comment.
(minor) this comment is a little confusing/recursive. What is the model of a model?
There was a problem hiding this comment.
Probably want messaging like this https://github.com/viamrobotics/api/blob/main/proto/viam/app/v1/app.proto#L1237-L1240.
app/app_client.go
Outdated
| return resp.Available, nil | ||
| } | ||
|
|
||
| // UpdateOrganizationOptions contains optional parameters for UpdateOrganization. |
There was a problem hiding this comment.
This is a bit of an annoying/heavy lift (sorry!), but we have a number of option structs that just have this explanatory comment ({T}Options contains optional parameters for {T}). I think most of the time what these parameters do is pretty self explanatory so more isn't necessary, but in at least a couple cases (firstRun from UpdateModuleOptions comes to mind) the intended purpose/use seems non-obvious. Can we go through and add brief side comments in cases where it's unclear?
There was a problem hiding this comment.
Agree! I'm surprised the linter isn't yelling at you, actually, as it will usually require an "export comment" on every exported field of an exported struct.
There was a problem hiding this comment.
Yeah, the linter requires you to have a comment for each struct, which is why that comment is there on all of them. But yes, will definitely look through one more time to see confusing types.
| errorsOnly = true | ||
| ) | ||
|
|
||
| var ( |
There was a problem hiding this comment.
(q) just out of curiosity, why are these vars and not consts?
There was a problem hiding this comment.
Some of them have to be pointers, and in golang, you can't take an address to a const.
app/app_client.go
Outdated
| type UpdateOrganizationOptions struct { | ||
| Name *string | ||
| Namespace *string | ||
| Region *string | ||
| CID *string | ||
| } |
There was a problem hiding this comment.
I see here you are putting structs right above where they are being used rather than at the top. in this example it looks like they are doing that too - so I will change mine to mimic this
There was a problem hiding this comment.
coming back to this actually, I did not make these changes in my PR yet. I def can but I am curious which way you think we should do it? I think that there are a lot of shadow types and structs in these files and I don't mind having them all at the top that way they are easy to access in one place, but if you prefer having them above the function I am happy to make that change :)
| return organizations, nil | ||
| } | ||
|
|
||
| // GetOrganization gets an organization. |
There was a problem hiding this comment.
might be overkill to add this but maybe "GetOrganization gets an organization gets an organization based on a given orgID"?
There was a problem hiding this comment.
I think this should be fine. While the python SDK does expand on this specific function, there are also functions like get_location() that the Python SDK uses #Get a location
app/app_client.go
Outdated
| } | ||
|
|
||
| // UpdateOrganization updates an organization. | ||
| func (c *AppClient) UpdateOrganization(ctx context.Context, orgID string, opts UpdateOrganizationOptions) (*Organization, error) { |
There was a problem hiding this comment.
iirc I believe we want to change the opts parameter to a pointer, opts * UpdateOrganizationOptions, because we want to explicitly allow nil values since these are optional fields.
app/app_client.go
Outdated
| ParentLocationID *string | ||
| } | ||
|
|
||
| // CreateLocation creates a location. |
There was a problem hiding this comment.
it is a little unclear what the parameters here are being used for, maybe add something like:
// CreateLocation creates a new location with the specified name under the given orgID.
// Optionally, the location can be placed under a new parent location specified in the options.
app/app_client.go
Outdated
| } | ||
|
|
||
| // CreateLocation creates a location. | ||
| func (c *AppClient) CreateLocation(ctx context.Context, orgID, name string, opts CreateLocationOptions) (*Location, error) { |
There was a problem hiding this comment.
same thing here about passing opts as pointer (and the ones below)
app/app_client.go
Outdated
| return resp.Available, nil | ||
| } | ||
|
|
||
| // UpdateOrganizationOptions contains optional parameters for UpdateOrganization. |
There was a problem hiding this comment.
Agree! I'm surprised the linter isn't yelling at you, actually, as it will usually require an "export comment" on every exported field of an exported struct.
app/app_client.go
Outdated
| } | ||
|
|
||
| // UpdateOrganization updates an organization. | ||
| func (c *AppClient) UpdateOrganization(ctx context.Context, orgID string, opts UpdateOrganizationOptions) (*Organization, error) { |
app/app_client.go
Outdated
| stream pb.AppService_TailRobotPartLogsClient | ||
| } | ||
|
|
||
| // Next gets the next robot part log entry. |
There was a problem hiding this comment.
| // Next gets the next robot part log entry. | |
| // Next gets the next slice of robot part log entries. |
app/registry_proto_conversions.go
Outdated
| } | ||
| } | ||
|
|
||
| // Model has the API and model of a model. |
There was a problem hiding this comment.
Probably want messaging like this https://github.com/viamrobotics/api/blob/main/proto/viam/app/v1/app.proto#L1237-L1240.
app/app_proto_conversions.go
Outdated
| } | ||
|
|
||
| // SharedSecretState specifies if the secret is enabled, disabled, or unspecified. | ||
| type SharedSecretState int32 |
There was a problem hiding this comment.
[nit] This and the enums in this PR can just be int, I think (int32 or int64 depending on your system.) Using int32 implies some need for a smaller int that probably is not necessary.
app/app_proto_conversions.go
Outdated
| Auth *LocationAuth | ||
| Organizations []*LocationOrganization | ||
| CreatedOn *timestamppb.Timestamp | ||
| RobotCount int32 |
There was a problem hiding this comment.
I would also prefer int > int32 inside structs/as parameters.
There was a problem hiding this comment.
Switching these int types are a bigger struggle than I thought, but hopefully I figured it out.
There was a problem hiding this comment.
overall I think this looks great! The one thing that stood out to me was that the optional parameters should probably be passed as pointers. Another thing to consider is adding in checks to ensure we are not passing nil values to proto requests. Unless the .proto file defines the variable as an optional *string but I think for the most part it doesn't (I could be wrong)
app/app_client.go
Outdated
| type UpdateOrganizationOptions struct { | ||
| Name *string | ||
| Namespace *string | ||
| Region *string | ||
| CID *string | ||
| } |
There was a problem hiding this comment.
coming back to this actually, I did not make these changes in my PR yet. I def can but I am curious which way you think we should do it? I think that there are a lot of shadow types and structs in these files and I don't mind having them all at the top that way they are easy to access in one place, but if you prefer having them above the function I am happy to make that change :)
| resp, err := c.client.UpdateOrganization(ctx, &pb.UpdateOrganizationRequest{ | ||
| OrganizationId: orgID, | ||
| Name: opts.Name, | ||
| PublicNamespace: opts.Namespace, | ||
| Region: opts.Region, | ||
| Cid: opts.CID, | ||
| }) |
There was a problem hiding this comment.
I added a check to all of the optional parameters that said
if options.SomeParam != nil {
metadata.SomeParam = *options.SomeParam
}
I don't know if this is necessary but I was nervous about a request taking in a nil value. I was hoping that by adding in the check it would just omit this field and give it a zero-value instead. again, not sure if this was needed but sharing jic
There was a problem hiding this comment.
Passing in nil to pointers is a common practice in Go. If SomeParam is nil, metadata.SomeParam = *options.SomeParam would be passing in a pointer to nil, which I think actually causes problems.
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
Adding all the app wrappers.
Notes:
app_proto_conversions.gointoapp_client.goif preferred.registry_proto_conversions.go. I kept this here since ML Training will be using this too, so I'll probably use this file and rename it as the ML Training client. Anything ML Training does not use when we introduce the ML Training wrappers will probably go back intoapp_proto_conversions.go.TailRobotPartLogs, we can give the stream object itself to the user so that the user does what they want with it, whether they want to make it a channel or not. This seems to be common practice in Go.