implement initial discover service #4665
Conversation
viambot
added
the
safe to test
| @@ -431,3 +431,5 @@ require ( | |||
| github.com/ziutek/mymysql v1.5.4 // indirect | |||
| golang.org/x/exp v0.0.0-20240904232852-e7e105dedf7e | |||
| ) | |||
|
|
|||
| replace go.viam.com/api => github.com/johnn193/api v0.0.0-20241231164642-99f059defc82 | |||
There was a problem hiding this comment.
will revert once api is merged
| return discovery.NewRPCServiceServer(injectSvc).(pb.DiscoveryServiceServer), injectDiscovery, injectDiscovery2, nil | ||
| } | ||
|
|
||
| func TestDiscoveryDo(t *testing.T) { |
There was a problem hiding this comment.
since this is literally copy/paste from the generic service I should probably remove this test, but I'm also fine with leaving it in case we are worried this will somehow break
There was a problem hiding this comment.
It is good to test all APIs of a service to ensure that there is adequate coverage. I can't imagine it breaking - but say someone with no context joins viam in the future and removes the DoCommands accidentally - good to have all these tests fail :) . It does not have to be separate from your other test, you can add an injected DoCommand to the injected service in your test.
Good test to have with DoComamnd are: 1. test that it returns errors if no DoCommand is implemented with resource.Named, and 2. responds with the expected responses when a DoCommand is implemented.
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
| // this was taken from proto_conversion_test.go | ||
| // | ||
| //nolint:thelper | ||
| func validateComponent(t *testing.T, actual, expected resource.Config) { |
There was a problem hiding this comment.
this is the same function as in proto_conversion_test.go
unsure if theres a good spot to put this function to use it in both spots, or if its fine to leave here
| // Service describes the functions that are available to the service. | ||
| type Service interface { | ||
| resource.Resource | ||
| DiscoverResources(ctx context.Context, extra map[string]any) ([]*resource.Config, error) |
There was a problem hiding this comment.
Does this need to return an array of pointers to resource.Configs? Or would it be okay to return []resource.Config, since you can already return nil here?
There was a problem hiding this comment.
I'm not familiar with this project but in terms of API design:
At a cursory glance, the vast majority of resource.Configs are passed and returned from functions as structs rather than pointers to structs.
In my opinion, as long as DiscoverResources isn't performance sensitive (aka we can afford to copy all the resource.Configs), we should return []resource.Config rather than []*resource.Config to be consistent with the other uses of resource.Config and b/c it makes it clear that the caller is getting a copy of the data which they are allowed to modify if they choose.
There was a problem hiding this comment.
The API is meant to aid users in configuring their machines - it can return a large array - say you're configuring a thousand cameras at once - but I don't see it needing to be that performant - and I think arrays in go are pointers under the hood anyway?
There was a problem hiding this comment.
Slices are indeed pointers in go. However each element of the []resource.Config is going to have a new copy of resource.Config, where as []*resource.Config can return pointers to the same objects rather than needing to make copies. As a result, if the slice has a lot of elements and the resource.Config is very big I would expect []*resource.Config to consume less memory than []resource.Config.
That said, returning pointers to objects that other parts of the system are using (and don't expect to change) is a common source of bugs, so I think we should prefer the safer and slightly more expensive method signature until we know the performance matters. I agree that this code is unlikely to be very performance-sensitive.
| } | ||
|
|
||
| // Service describes the functions that are available to the service. | ||
| type Service interface { |
There was a problem hiding this comment.
hmm, surprised this is called Service not discovery since the components follow. different pattern. But whatever, the other services do this so let's keep that format.
| configs, err := svc.DiscoverResources(ctx, req.GetExtra().AsMap()) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
we should also have a check if configs are nil here, and also test that we error nicely in the server test.
| SubtypeName = "discovery" | ||
| ) | ||
|
|
||
| // API is a variable that identifies the slam resource API. |
There was a problem hiding this comment.
| // API is a variable that identifies the slam resource API. | |
| // API is a variable that identifies the discovery resource API. |
Initial implementation of the discover service in Go. Currently working on GetModelsFromModules in a separate branch to make it easier on reviewers
ticket: https://viam.atlassian.net/browse/RSDK-9620