From c1f289740e62c52c67c2de78ac42a3f4f61e61d3 Mon Sep 17 00:00:00 2001 From: John Date: Tue, 31 Dec 2024 16:16:29 -0500 Subject: [PATCH 01/12] implement initial discover service --- go.mod | 2 + go.sum | 4 +- services/discovery/client.go | 75 +++++++++++++++++++++++++++ services/discovery/discovery.go | 50 ++++++++++++++++++ services/discovery/server.go | 68 ++++++++++++++++++++++++ services/discovery/server_test.go | 73 ++++++++++++++++++++++++++ testutils/inject/discovery_service.go | 42 +++++++++++++++ 7 files changed, 312 insertions(+), 2 deletions(-) create mode 100644 services/discovery/client.go create mode 100644 services/discovery/discovery.go create mode 100644 services/discovery/server.go create mode 100644 services/discovery/server_test.go create mode 100644 testutils/inject/discovery_service.go diff --git a/go.mod b/go.mod index ae0d6f3459a..2449499cb82 100644 --- a/go.mod +++ b/go.mod @@ -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 diff --git a/go.sum b/go.sum index ff9673c7a94..d7f1ab3c889 100644 --- a/go.sum +++ b/go.sum @@ -788,6 +788,8 @@ github.com/jmespath/go-jmespath v0.4.0/go.mod h1:T8mJZnbsbmF+m6zOOFylbeCJqk5+pHW github.com/jmespath/go-jmespath/internal/testify v1.5.1 h1:shLQSRRSCCPj3f2gpwzGwWFoC7ycTf1rcQZHOlsJ6N8= github.com/jmespath/go-jmespath/internal/testify v1.5.1/go.mod h1:L3OGu8Wl2/fWfCI6z80xFu9LTZmf1ZRjMHUOPmWr69U= github.com/jmoiron/sqlx v1.2.0/go.mod h1:1FEQNm3xlJgrMD+FBdI9+xvCksHtbpVBBw5dYhBSsks= +github.com/johnn193/api v0.0.0-20241231164642-99f059defc82 h1:V2fQZbYvvjDK/oz7LxluzuKXdFGljuvbpJhs7cqT2jo= +github.com/johnn193/api v0.0.0-20241231164642-99f059defc82/go.mod h1:g5eipXHNm0rQmW7DWya6avKcmzoypLmxnMlAaIsE5Ls= github.com/jonboulle/clockwork v0.1.0/go.mod h1:Ii8DK3G1RaLaWxj9trq07+26W01tbo22gdxWY5EU2bo= github.com/jonboulle/clockwork v0.3.0 h1:9BSCMi8C+0qdApAp4auwX0RkLGUjs956h0EkuQymUhg= github.com/jonboulle/clockwork v0.3.0/go.mod h1:Pkfl5aHPm1nk2H9h0bjmnJD/BcgbGXUBGnn1kMkgxc8= @@ -1513,8 +1515,6 @@ go.uber.org/zap v1.18.1/go.mod h1:xg/QME4nWcxGxrpdeYfq7UvYrLh66cuVKdrbD1XF/NI= go.uber.org/zap v1.23.0/go.mod h1:D+nX8jyLsMHMYrln8A0rJjFt/T/9/bGgIhAqxv5URuY= go.uber.org/zap v1.27.0 h1:aJMhYGrd5QSmlpLMr2MftRKl7t8J8PTZPA732ud/XR8= go.uber.org/zap v1.27.0/go.mod h1:GB2qFLM7cTU87MWRP2mPIjqfIDnGu+VIO4V/SdhGo2E= -go.viam.com/api v0.1.372 h1:Al9P7yojBDdNVAF7nrr5BAbzCvb+vrSp8N7BitbV0mQ= -go.viam.com/api v0.1.372/go.mod h1:g5eipXHNm0rQmW7DWya6avKcmzoypLmxnMlAaIsE5Ls= go.viam.com/test v1.2.4 h1:JYgZhsuGAQ8sL9jWkziAXN9VJJiKbjoi9BsO33TW3ug= go.viam.com/test v1.2.4/go.mod h1:zI2xzosHdqXAJ/kFqcN+OIF78kQuTV2nIhGZ8EzvaJI= go.viam.com/utils v0.1.118 h1:Kp6ebrCBiYReeSC1XnWPTjtBJoTUsQ6YWAomQkQF/mE= diff --git a/services/discovery/client.go b/services/discovery/client.go new file mode 100644 index 00000000000..ee06084a6d5 --- /dev/null +++ b/services/discovery/client.go @@ -0,0 +1,75 @@ +package discovery + +import ( + "context" + + "go.opencensus.io/trace" + pb "go.viam.com/api/service/discovery/v1" + "go.viam.com/utils/protoutils" + "go.viam.com/utils/rpc" + + "go.viam.com/rdk/config" + "go.viam.com/rdk/logging" + rprotoutils "go.viam.com/rdk/protoutils" + "go.viam.com/rdk/resource" +) + +// client implements DiscoveryServiceClient. +type client struct { + resource.Named + resource.TriviallyReconfigurable + resource.TriviallyCloseable + name string + client pb.DiscoveryServiceClient + logger logging.Logger +} + +// NewClientFromConn constructs a new Client from the connection passed in. +func NewClientFromConn( + ctx context.Context, + conn rpc.ClientConn, + remoteName string, + name resource.Name, + logger logging.Logger, +) (Service, error) { + grpcClient := pb.NewDiscoveryServiceClient(conn) + c := &client{ + Named: name.PrependRemote(remoteName).AsNamed(), + name: name.ShortName(), + client: grpcClient, + logger: logger, + } + return c, nil +} + +func (c *client) DiscoverResources(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { + ctx, span := trace.StartSpan(ctx, "discovery::client::DoCommand") + defer span.End() + ext, err := protoutils.StructToStructPb(extra) + if err != nil { + return nil, err + } + + req := &pb.DiscoverResourcesRequest{Name: c.name, Extra: ext} + resp, err := c.client.DiscoverResources(ctx, req) + if err != nil { + return nil, err + } + discoveredConfigs := []*resource.Config{} + protoConfigs := resp.GetDiscovery() + for _, proto := range protoConfigs { + config, err := config.ComponentConfigFromProto(proto) + if err != nil { + return nil, err + } + discoveredConfigs = append(discoveredConfigs, config) + } + return discoveredConfigs, nil +} + +func (c *client) DoCommand(ctx context.Context, cmd map[string]interface{}) (map[string]interface{}, error) { + ctx, span := trace.StartSpan(ctx, "discovery::client::DoCommand") + defer span.End() + + return rprotoutils.DoFromResourceClient(ctx, c.client, c.name, cmd) +} diff --git a/services/discovery/discovery.go b/services/discovery/discovery.go new file mode 100644 index 00000000000..9c7ca428a37 --- /dev/null +++ b/services/discovery/discovery.go @@ -0,0 +1,50 @@ +// Package discovery implements the discovery service, which lets users surface resource configs for their machines to use. +package discovery + +import ( + "context" + + pb "go.viam.com/api/service/discovery/v1" + + "go.viam.com/rdk/resource" + "go.viam.com/rdk/robot" +) + +func init() { + resource.RegisterAPI(API, resource.APIRegistration[Service]{ + RPCServiceServerConstructor: NewRPCServiceServer, + RPCServiceHandler: pb.RegisterDiscoveryServiceHandlerFromEndpoint, + RPCServiceDesc: &pb.DiscoveryService_ServiceDesc, + RPCClient: NewClientFromConn, + }) +} + +// SubtypeName is the name of the type of service. +const ( + SubtypeName = "discovery" +) + +// API is a variable that identifies the slam resource API. +var API = resource.APINamespaceRDK.WithServiceType(SubtypeName) + +// Named is a helper for getting the named service's typed resource name. +func Named(name string) resource.Name { + return resource.NewName(API, name) +} + +// FromRobot is a helper for getting the named discovery service from the given Robot. +func FromRobot(r robot.Robot, name string) (Service, error) { + return robot.ResourceFromRobot[Service](r, Named(name)) +} + +// FromDependencies is a helper for getting the named discovery service from a collection of +// dependencies. +func FromDependencies(deps resource.Dependencies, name string) (Service, error) { + return resource.FromDependencies[Service](deps, Named(name)) +} + +// 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) +} diff --git a/services/discovery/server.go b/services/discovery/server.go new file mode 100644 index 00000000000..b979028b765 --- /dev/null +++ b/services/discovery/server.go @@ -0,0 +1,68 @@ +package discovery + +import ( + "context" + + "go.opencensus.io/trace" + apppb "go.viam.com/api/app/v1" + commonpb "go.viam.com/api/common/v1" + pb "go.viam.com/api/service/discovery/v1" + + "go.viam.com/rdk/config" + "go.viam.com/rdk/protoutils" + "go.viam.com/rdk/resource" +) + +// serviceServer implements the DiscoveryService from the discovery proto. +type serviceServer struct { + pb.UnimplementedDiscoveryServiceServer + coll resource.APIResourceCollection[Service] +} + +// NewRPCServiceServer constructs a the discovery gRPC service server. +// It is intentionally untyped to prevent use outside of tests. +func NewRPCServiceServer(coll resource.APIResourceCollection[Service]) interface{} { + return &serviceServer{coll: coll} +} + +// DiscoverResources returns a list of components discovered by a discovery service. +func (server *serviceServer) DiscoverResources(ctx context.Context, req *pb.DiscoverResourcesRequest) ( + *pb.DiscoverResourcesResponse, error, +) { + ctx, span := trace.StartSpan(ctx, "discovery::server::DiscoverResources") + defer span.End() + + svc, err := server.coll.Resource(req.Name) + if err != nil { + return nil, err + } + + configs, err := svc.DiscoverResources(ctx, req.GetExtra().AsMap()) + if err != nil { + return nil, err + } + protoConfigs := []*apppb.ComponentConfig{} + for _, cfg := range configs { + proto, err := config.ComponentConfigToProto(cfg) + if err != nil { + return nil, err + } + protoConfigs = append(protoConfigs, proto) + } + + return &pb.DiscoverResourcesResponse{Discovery: protoConfigs}, nil +} + +// DoCommand receives arbitrary commands. +func (server *serviceServer) DoCommand(ctx context.Context, + req *commonpb.DoCommandRequest, +) (*commonpb.DoCommandResponse, error) { + ctx, span := trace.StartSpan(ctx, "discovery::server::DoCommand") + defer span.End() + + svc, err := server.coll.Resource(req.Name) + if err != nil { + return nil, err + } + return protoutils.DoFromResourceServer(ctx, svc, req) +} diff --git a/services/discovery/server_test.go b/services/discovery/server_test.go new file mode 100644 index 00000000000..29b336c27dc --- /dev/null +++ b/services/discovery/server_test.go @@ -0,0 +1,73 @@ +package discovery_test + +import ( + "context" + "errors" + "testing" + + commonpb "go.viam.com/api/common/v1" + pb "go.viam.com/api/service/discovery/v1" + "go.viam.com/test" + "go.viam.com/utils/protoutils" + + "go.viam.com/rdk/resource" + "go.viam.com/rdk/services/discovery" + "go.viam.com/rdk/testutils" + "go.viam.com/rdk/testutils/inject" +) + +var errDoFailed = errors.New("do failed") + +func newServer() (pb.DiscoveryServiceServer, *inject.DiscoveryService, *inject.DiscoveryService, error) { + injectDiscovery1 := &inject.DiscoveryService{} + injectDiscovery2 := &inject.DiscoveryService{} + resourceMap := map[resource.Name]resource.Resource{ + discovery.Named(testDiscoveryName): injectDiscovery, + discovery.Named(failDiscoveryName): injectDiscovery2, + } + injectSvc, err := resource.NewAPIResourceCollection(discovery.API, resourceMap) + if err != nil { + return nil, nil, nil, err + } + return discovery.NewRPCServiceServer(injectSvc).(pb.DiscoveryServiceServer), injectDiscovery, injectDiscovery2, nil +} + +func TestDiscoveryDo(t *testing.T) { + discoveryServer, workingDiscovery, failingDiscovery, err := newServer() + test.That(t, err, test.ShouldBeNil) + + workingDiscovery.DoFunc = func( + ctx context.Context, + cmd map[string]interface{}, + ) ( + map[string]interface{}, + error, + ) { + return cmd, nil + } + failingDiscovery.DoFunc = func( + ctx context.Context, + cmd map[string]interface{}, + ) ( + map[string]interface{}, + error, + ) { + return nil, errDoFailed + } + + commandStruct, err := protoutils.StructToStructPb(testutils.TestCommand) + test.That(t, err, test.ShouldBeNil) + + req := commonpb.DoCommandRequest{Name: testDiscoveryName, Command: commandStruct} + resp, err := discoveryServer.DoCommand(context.Background(), &req) + test.That(t, err, test.ShouldBeNil) + test.That(t, resp, test.ShouldNotBeNil) + test.That(t, resp.Result.AsMap()["cmd"], test.ShouldEqual, testutils.TestCommand["cmd"]) + test.That(t, resp.Result.AsMap()["data"], test.ShouldEqual, testutils.TestCommand["data"]) + + req = commonpb.DoCommandRequest{Name: failDiscoveryName, Command: commandStruct} + resp, err = discoveryServer.DoCommand(context.Background(), &req) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err.Error(), test.ShouldContainSubstring, errDoFailed.Error()) + test.That(t, resp, test.ShouldBeNil) +} diff --git a/testutils/inject/discovery_service.go b/testutils/inject/discovery_service.go new file mode 100644 index 00000000000..fe4eb85cc47 --- /dev/null +++ b/testutils/inject/discovery_service.go @@ -0,0 +1,42 @@ +package inject + +import ( + "context" + + "go.viam.com/rdk/resource" + "go.viam.com/rdk/services/discovery" +) + +// GenericService is an injectable discovery service. +type DiscoveryService struct { + discovery.Service + name resource.Name + DiscoverResourcesFunc func(ctx context.Context, extra map[string]any) ([]*resource.Config, error) + DoFunc func(ctx context.Context, cmd map[string]interface{}) (map[string]interface{}, error) +} + +// NewGenericService returns a new injected generic service. +func NewDiscoveryService(name string) *DiscoveryService { + return &DiscoveryService{name: discovery.Named(name)} +} + +// Name returns the name of the resource. +func (disSvc *DiscoveryService) Name() resource.Name { + return disSvc.name +} + +// Position calls the injected PositionFunc or the real version. +func (disSvc *DiscoveryService) DiscoverResources(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { + if disSvc.DiscoverResourcesFunc == nil { + return disSvc.Service.DiscoverResources(ctx, extra) + } + return disSvc.DiscoverResourcesFunc(ctx, extra) +} + +// DoCommand calls the injected DoCommand or the real version. +func (disSvc *DiscoveryService) DoCommand(ctx context.Context, cmd map[string]interface{}) (map[string]interface{}, error) { + if disSvc.DoFunc == nil { + return disSvc.Service.DoCommand(ctx, cmd) + } + return disSvc.DoFunc(ctx, cmd) +} From 719daa25b375007c6e2ad0ebc7271206e35dc52e Mon Sep 17 00:00:00 2001 From: John Date: Tue, 31 Dec 2024 16:39:57 -0500 Subject: [PATCH 02/12] add initial server test --- services/discovery/server_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/services/discovery/server_test.go b/services/discovery/server_test.go index 29b336c27dc..6b1d6e45015 100644 --- a/services/discovery/server_test.go +++ b/services/discovery/server_test.go @@ -16,12 +16,17 @@ import ( "go.viam.com/rdk/testutils/inject" ) +const ( + testDiscoveryName = "discovery1" + failDiscoveryName = "discovery2" +) + var errDoFailed = errors.New("do failed") func newServer() (pb.DiscoveryServiceServer, *inject.DiscoveryService, *inject.DiscoveryService, error) { - injectDiscovery1 := &inject.DiscoveryService{} + injectDiscovery := &inject.DiscoveryService{} injectDiscovery2 := &inject.DiscoveryService{} - resourceMap := map[resource.Name]resource.Resource{ + resourceMap := map[resource.Name]discovery.Service{ discovery.Named(testDiscoveryName): injectDiscovery, discovery.Named(failDiscoveryName): injectDiscovery2, } From 584e4a29b7b4e59caca2f2bd87a9cda11a58ebe5 Mon Sep 17 00:00:00 2001 From: John Date: Fri, 3 Jan 2025 14:21:52 -0500 Subject: [PATCH 03/12] add initial client test --- services/discovery/client_test.go | 118 ++++++++++++++++++++++++++++++ services/discovery/server_test.go | 20 +++++ 2 files changed, 138 insertions(+) create mode 100644 services/discovery/client_test.go diff --git a/services/discovery/client_test.go b/services/discovery/client_test.go new file mode 100644 index 00000000000..0c5ac4ec6dd --- /dev/null +++ b/services/discovery/client_test.go @@ -0,0 +1,118 @@ +package discovery_test + +import ( + "context" + "net" + "testing" + + "go.viam.com/test" + "go.viam.com/utils/rpc" + + viamgrpc "go.viam.com/rdk/grpc" + "go.viam.com/rdk/logging" + "go.viam.com/rdk/resource" + "go.viam.com/rdk/services/discovery" + "go.viam.com/rdk/testutils" + "go.viam.com/rdk/testutils/inject" +) + +func TestClient(t *testing.T) { + logger := logging.NewTestLogger(t) + listener1, err := net.Listen("tcp", "localhost:0") + test.That(t, err, test.ShouldBeNil) + rpcServer, err := rpc.NewServer(logger, rpc.WithUnauthenticated()) + test.That(t, err, test.ShouldBeNil) + + workingDiscovery := &inject.DiscoveryService{} + failingDiscovery := &inject.DiscoveryService{} + + workingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { + return nil, nil + } + failingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { + return nil, errDiscoverFailed + } + workingDiscovery.DoFunc = testutils.EchoFunc + failingDiscovery.DoFunc = func( + ctx context.Context, + cmd map[string]interface{}, + ) ( + map[string]interface{}, + error, + ) { + return nil, errDoFailed + } + + resourceMap := map[resource.Name]resource.Resource{ + discovery.Named(testDiscoveryName): workingDiscovery, + discovery.Named(failDiscoveryName): failingDiscovery, + } + discoverySvc, err := resource.NewAPIResourceCollection(discovery.API, resourceMap) + test.That(t, err, test.ShouldBeNil) + resourceAPI, ok, err := resource.LookupAPIRegistration[resource.Resource](discovery.API) + test.That(t, err, test.ShouldBeNil) + test.That(t, ok, test.ShouldBeTrue) + test.That(t, resourceAPI.RegisterRPCService(context.Background(), rpcServer, discoverySvc), test.ShouldBeNil) + + go rpcServer.Serve(listener1) + defer rpcServer.Stop() + + t.Run("Failing client", func(t *testing.T) { + cancelCtx, cancel := context.WithCancel(context.Background()) + cancel() + _, err = viamgrpc.Dial(cancelCtx, listener1.Addr().String(), logger) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err, test.ShouldBeError, context.Canceled) + }) + + t.Run("client tests for working discovery", func(t *testing.T) { + conn, err := viamgrpc.Dial(context.Background(), listener1.Addr().String(), logger) + test.That(t, err, test.ShouldBeNil) + workingDiscoveryClient, err := discovery.NewClientFromConn(context.Background(), conn, "", discovery.Named(testDiscoveryName), logger) + test.That(t, err, test.ShouldBeNil) + + respDis, err := workingDiscoveryClient.DiscoverResources(context.Background(), nil) + test.That(t, err, test.ShouldBeNil) + test.That(t, respDis, test.ShouldNotBeNil) + + resp, err := workingDiscoveryClient.DoCommand(context.Background(), testutils.TestCommand) + test.That(t, err, test.ShouldBeNil) + test.That(t, resp["cmd"], test.ShouldEqual, testutils.TestCommand["cmd"]) + test.That(t, resp["data"], test.ShouldEqual, testutils.TestCommand["data"]) + + test.That(t, workingDiscoveryClient.Close(context.Background()), test.ShouldBeNil) + test.That(t, conn.Close(), test.ShouldBeNil) + }) + + t.Run("client tests for failing discovery", func(t *testing.T) { + conn, err := viamgrpc.Dial(context.Background(), listener1.Addr().String(), logger) + test.That(t, err, test.ShouldBeNil) + failingDiscoveryClient, err := discovery.NewClientFromConn(context.Background(), conn, "", discovery.Named(failDiscoveryName), logger) + test.That(t, err, test.ShouldBeNil) + + _, err = failingDiscoveryClient.DiscoverResources(context.Background(), nil) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err.Error(), test.ShouldContainSubstring, errDiscoverFailed.Error()) + + _, err = failingDiscoveryClient.DoCommand(context.Background(), testutils.TestCommand) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err.Error(), test.ShouldContainSubstring, errDoFailed.Error()) + + test.That(t, failingDiscoveryClient.Close(context.Background()), test.ShouldBeNil) + test.That(t, conn.Close(), test.ShouldBeNil) + }) + + t.Run("dialed client tests for working discovery", func(t *testing.T) { + conn, err := viamgrpc.Dial(context.Background(), listener1.Addr().String(), logger) + test.That(t, err, test.ShouldBeNil) + client, err := resourceAPI.RPCClient(context.Background(), conn, "", discovery.Named(testDiscoveryName), logger) + test.That(t, err, test.ShouldBeNil) + + resp, err := client.DoCommand(context.Background(), testutils.TestCommand) + test.That(t, err, test.ShouldBeNil) + test.That(t, resp["cmd"], test.ShouldEqual, testutils.TestCommand["cmd"]) + test.That(t, resp["data"], test.ShouldEqual, testutils.TestCommand["data"]) + + test.That(t, conn.Close(), test.ShouldBeNil) + }) +} diff --git a/services/discovery/server_test.go b/services/discovery/server_test.go index 6b1d6e45015..2d5667bdd5f 100644 --- a/services/discovery/server_test.go +++ b/services/discovery/server_test.go @@ -22,6 +22,7 @@ const ( ) var errDoFailed = errors.New("do failed") +var errDiscoverFailed = errors.New("discover failed") func newServer() (pb.DiscoveryServiceServer, *inject.DiscoveryService, *inject.DiscoveryService, error) { injectDiscovery := &inject.DiscoveryService{} @@ -37,6 +38,25 @@ func newServer() (pb.DiscoveryServiceServer, *inject.DiscoveryService, *inject.D return discovery.NewRPCServiceServer(injectSvc).(pb.DiscoveryServiceServer), injectDiscovery, injectDiscovery2, nil } +func TestDiscoverResources(t *testing.T) { + discoveryServer, workingDiscovery, failingDiscovery, err := newServer() + test.That(t, err, test.ShouldBeNil) + + workingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { + return nil, nil + } + failingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { + return nil, errDiscoverFailed + } + resp, err := discoveryServer.DiscoverResources(context.Background(), &pb.DiscoverResourcesRequest{Name: testDiscoveryName}) + test.That(t, err, test.ShouldBeNil) + test.That(t, resp, test.ShouldNotBeNil) + + respFail, err := discoveryServer.DiscoverResources(context.Background(), &pb.DiscoverResourcesRequest{Name: failDiscoveryName}) + test.That(t, err, test.ShouldEqual, errDiscoverFailed) + test.That(t, respFail, test.ShouldBeNil) +} + func TestDiscoveryDo(t *testing.T) { discoveryServer, workingDiscovery, failingDiscovery, err := newServer() test.That(t, err, test.ShouldBeNil) From e6b230f61a3a7343465537ad263c92a8399f0472 Mon Sep 17 00:00:00 2001 From: John Date: Fri, 3 Jan 2025 15:29:22 -0500 Subject: [PATCH 04/12] add discovery specific tests --- services/discovery/client_test.go | 14 ++-- services/discovery/server_test.go | 109 +++++++++++++++++++++++++++++- 2 files changed, 117 insertions(+), 6 deletions(-) diff --git a/services/discovery/client_test.go b/services/discovery/client_test.go index 0c5ac4ec6dd..1a51362a80d 100644 --- a/services/discovery/client_test.go +++ b/services/discovery/client_test.go @@ -26,8 +26,10 @@ func TestClient(t *testing.T) { workingDiscovery := &inject.DiscoveryService{} failingDiscovery := &inject.DiscoveryService{} + testComponents := []*resource.Config{createTestComponent("component-1"), createTestComponent("component-2")} + workingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { - return nil, nil + return testComponents, nil } failingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { return nil, errDiscoverFailed @@ -43,13 +45,13 @@ func TestClient(t *testing.T) { return nil, errDoFailed } - resourceMap := map[resource.Name]resource.Resource{ + resourceMap := map[resource.Name]discovery.Service{ discovery.Named(testDiscoveryName): workingDiscovery, discovery.Named(failDiscoveryName): failingDiscovery, } discoverySvc, err := resource.NewAPIResourceCollection(discovery.API, resourceMap) test.That(t, err, test.ShouldBeNil) - resourceAPI, ok, err := resource.LookupAPIRegistration[resource.Resource](discovery.API) + resourceAPI, ok, err := resource.LookupAPIRegistration[discovery.Service](discovery.API) test.That(t, err, test.ShouldBeNil) test.That(t, ok, test.ShouldBeTrue) test.That(t, resourceAPI.RegisterRPCService(context.Background(), rpcServer, discoverySvc), test.ShouldBeNil) @@ -73,7 +75,11 @@ func TestClient(t *testing.T) { respDis, err := workingDiscoveryClient.DiscoverResources(context.Background(), nil) test.That(t, err, test.ShouldBeNil) - test.That(t, respDis, test.ShouldNotBeNil) + test.That(t, len(respDis), test.ShouldEqual, len(testComponents)) + for index, actual := range respDis { + expected := testComponents[index] + validateComponent(t, *actual, *expected) + } resp, err := workingDiscoveryClient.DoCommand(context.Background(), testutils.TestCommand) test.That(t, err, test.ShouldBeNil) diff --git a/services/discovery/server_test.go b/services/discovery/server_test.go index 2d5667bdd5f..31409743fb9 100644 --- a/services/discovery/server_test.go +++ b/services/discovery/server_test.go @@ -5,15 +5,21 @@ import ( "errors" "testing" + "github.com/golang/geo/r3" commonpb "go.viam.com/api/common/v1" pb "go.viam.com/api/service/discovery/v1" "go.viam.com/test" "go.viam.com/utils/protoutils" + "go.viam.com/rdk/config" + "go.viam.com/rdk/logging" + "go.viam.com/rdk/referenceframe" "go.viam.com/rdk/resource" "go.viam.com/rdk/services/discovery" + spatial "go.viam.com/rdk/spatialmath" "go.viam.com/rdk/testutils" "go.viam.com/rdk/testutils/inject" + "go.viam.com/rdk/utils" ) const ( @@ -24,6 +30,56 @@ const ( var errDoFailed = errors.New("do failed") var errDiscoverFailed = errors.New("discover failed") +// this was taken from proto_conversions_test and represents all of the information that a discovery service can provide about a component +func createTestComponent(name string) *resource.Config { + testOrientation, _ := spatial.NewOrientationConfig(spatial.NewZeroOrientation()) + + testFrame := &referenceframe.LinkConfig{ + Parent: "world", + Translation: r3.Vector{X: 1, Y: 2, Z: 3}, + Orientation: testOrientation, + Geometry: &spatial.GeometryConfig{Type: "box", X: 1, Y: 2, Z: 1}, + } + + testComponent := resource.Config{ + Name: name, + API: resource.NewAPI("some-namespace", "component", "some-type"), + Model: resource.DefaultModelFamily.WithModel("some-model"), + DependsOn: []string{"dep1", "dep2"}, + Attributes: utils.AttributeMap{ + "attr1": 1, + "attr2": "attr-string", + }, + AssociatedResourceConfigs: []resource.AssociatedResourceConfig{ + // these will get rewritten in tests to simulate API data + { + // will resemble the same but become "foo:bar:baz" + API: resource.NewAPI("foo", "bar", "baz"), + Attributes: utils.AttributeMap{ + "attr1": 1, + }, + }, + { + // will stay the same + API: resource.APINamespaceRDK.WithServiceType("some-type-2"), + Attributes: utils.AttributeMap{ + "attr1": 2, + }, + }, + { + // will resemble the same but be just "some-type-3" + API: resource.APINamespaceRDK.WithServiceType("some-type-3"), + Attributes: utils.AttributeMap{ + "attr1": 3, + }, + }, + }, + Frame: testFrame, + LogConfiguration: &resource.LogConfig{Level: logging.DEBUG}, + } + return &testComponent +} + func newServer() (pb.DiscoveryServiceServer, *inject.DiscoveryService, *inject.DiscoveryService, error) { injectDiscovery := &inject.DiscoveryService{} injectDiscovery2 := &inject.DiscoveryService{} @@ -41,16 +97,24 @@ func newServer() (pb.DiscoveryServiceServer, *inject.DiscoveryService, *inject.D func TestDiscoverResources(t *testing.T) { discoveryServer, workingDiscovery, failingDiscovery, err := newServer() test.That(t, err, test.ShouldBeNil) + testComponents := []*resource.Config{createTestComponent("component-1"), createTestComponent("component-2")} workingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { - return nil, nil + return testComponents, nil } failingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { return nil, errDiscoverFailed } resp, err := discoveryServer.DiscoverResources(context.Background(), &pb.DiscoverResourcesRequest{Name: testDiscoveryName}) test.That(t, err, test.ShouldBeNil) - test.That(t, resp, test.ShouldNotBeNil) + test.That(t, len(resp.GetDiscovery()), test.ShouldEqual, len(testComponents)) + for index, proto := range resp.GetDiscovery() { + expected := testComponents[index] + test.That(t, proto.Name, test.ShouldEqual, expected.Name) + actual, err := config.ComponentConfigFromProto(proto) + test.That(t, err, test.ShouldBeNil) + validateComponent(t, *actual, *expected) + } respFail, err := discoveryServer.DiscoverResources(context.Background(), &pb.DiscoverResourcesRequest{Name: failDiscoveryName}) test.That(t, err, test.ShouldEqual, errDiscoverFailed) @@ -96,3 +160,44 @@ func TestDiscoveryDo(t *testing.T) { test.That(t, err.Error(), test.ShouldContainSubstring, errDoFailed.Error()) test.That(t, resp, test.ShouldBeNil) } + +// this was taken from proto_conversion_test.go +// +//nolint:thelper +func validateComponent(t *testing.T, actual, expected resource.Config) { + test.That(t, actual.Name, test.ShouldEqual, expected.Name) + test.That(t, actual.API, test.ShouldResemble, expected.API) + test.That(t, actual.Model, test.ShouldResemble, expected.Model) + test.That(t, actual.DependsOn, test.ShouldResemble, expected.DependsOn) + test.That(t, actual.Attributes.Int("attr1", 0), test.ShouldEqual, expected.Attributes.Int("attr1", -1)) + test.That(t, actual.Attributes.String("attr2"), test.ShouldEqual, expected.Attributes.String("attr2")) + + test.That(t, actual.AssociatedResourceConfigs, test.ShouldHaveLength, 3) + test.That(t, actual.AssociatedResourceConfigs[0].API, test.ShouldResemble, expected.AssociatedResourceConfigs[0].API) + test.That(t, + actual.AssociatedResourceConfigs[0].Attributes.Int("attr1", 0), + test.ShouldEqual, + expected.AssociatedResourceConfigs[0].Attributes.Int("attr1", -1)) + test.That(t, + actual.AssociatedResourceConfigs[1].API, + test.ShouldResemble, + expected.AssociatedResourceConfigs[1].API) + test.That(t, + actual.AssociatedResourceConfigs[1].Attributes.Int("attr1", 0), + test.ShouldEqual, + expected.AssociatedResourceConfigs[1].Attributes.Int("attr1", -1)) + test.That(t, + actual.AssociatedResourceConfigs[2].API, + test.ShouldResemble, + expected.AssociatedResourceConfigs[2].API) + test.That(t, + actual.AssociatedResourceConfigs[2].Attributes.Int("attr1", 0), + test.ShouldEqual, + expected.AssociatedResourceConfigs[2].Attributes.Int("attr1", -1)) + + f1, err := actual.Frame.ParseConfig() + test.That(t, err, test.ShouldBeNil) + f2, err := expected.Frame.ParseConfig() + test.That(t, err, test.ShouldBeNil) + test.That(t, f1, test.ShouldResemble, f2) +} From fa5359fdbfdb2c8c32f79cfa10b20a57b118a33e Mon Sep 17 00:00:00 2001 From: John Date: Fri, 3 Jan 2025 15:41:39 -0500 Subject: [PATCH 05/12] lint --- services/discovery/server_test.go | 8 +++++--- testutils/inject/discovery_service.go | 6 +++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/services/discovery/server_test.go b/services/discovery/server_test.go index 31409743fb9..8e68becc640 100644 --- a/services/discovery/server_test.go +++ b/services/discovery/server_test.go @@ -27,10 +27,12 @@ const ( failDiscoveryName = "discovery2" ) -var errDoFailed = errors.New("do failed") -var errDiscoverFailed = errors.New("discover failed") +var ( + errDoFailed = errors.New("do failed") + errDiscoverFailed = errors.New("discover failed") +) -// this was taken from proto_conversions_test and represents all of the information that a discovery service can provide about a component +// this was taken from proto_conversions_test and represents all of the information that a discovery service can provide about a component. func createTestComponent(name string) *resource.Config { testOrientation, _ := spatial.NewOrientationConfig(spatial.NewZeroOrientation()) diff --git a/testutils/inject/discovery_service.go b/testutils/inject/discovery_service.go index fe4eb85cc47..ec9b8aaccad 100644 --- a/testutils/inject/discovery_service.go +++ b/testutils/inject/discovery_service.go @@ -7,7 +7,7 @@ import ( "go.viam.com/rdk/services/discovery" ) -// GenericService is an injectable discovery service. +// DiscoveryService is an injectable discovery service. type DiscoveryService struct { discovery.Service name resource.Name @@ -15,7 +15,7 @@ type DiscoveryService struct { DoFunc func(ctx context.Context, cmd map[string]interface{}) (map[string]interface{}, error) } -// NewGenericService returns a new injected generic service. +// NewDiscoveryService returns a new injected discovery service. func NewDiscoveryService(name string) *DiscoveryService { return &DiscoveryService{name: discovery.Named(name)} } @@ -25,7 +25,7 @@ func (disSvc *DiscoveryService) Name() resource.Name { return disSvc.name } -// Position calls the injected PositionFunc or the real version. +// DiscoverResources calls the injected DiscoverResourcesFunc or the real version. func (disSvc *DiscoveryService) DiscoverResources(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { if disSvc.DiscoverResourcesFunc == nil { return disSvc.Service.DiscoverResources(ctx, extra) From 8cd995df0e7ef442713f7935f721df4363edcf9c Mon Sep 17 00:00:00 2001 From: John Date: Tue, 7 Jan 2025 10:42:56 -0500 Subject: [PATCH 06/12] bump api --- go.mod | 2 +- go.sum | 4 ++-- services/discovery/client.go | 2 +- services/discovery/server.go | 2 +- services/discovery/server_test.go | 4 ++-- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index 2449499cb82..4c989b7184f 100644 --- a/go.mod +++ b/go.mod @@ -432,4 +432,4 @@ require ( golang.org/x/exp v0.0.0-20240904232852-e7e105dedf7e ) -replace go.viam.com/api => github.com/johnn193/api v0.0.0-20241231164642-99f059defc82 +replace go.viam.com/api => github.com/johnn193/api v0.0.0-20250107153511-969bffad5413 diff --git a/go.sum b/go.sum index d7f1ab3c889..61c858dc29d 100644 --- a/go.sum +++ b/go.sum @@ -788,8 +788,8 @@ github.com/jmespath/go-jmespath v0.4.0/go.mod h1:T8mJZnbsbmF+m6zOOFylbeCJqk5+pHW github.com/jmespath/go-jmespath/internal/testify v1.5.1 h1:shLQSRRSCCPj3f2gpwzGwWFoC7ycTf1rcQZHOlsJ6N8= github.com/jmespath/go-jmespath/internal/testify v1.5.1/go.mod h1:L3OGu8Wl2/fWfCI6z80xFu9LTZmf1ZRjMHUOPmWr69U= github.com/jmoiron/sqlx v1.2.0/go.mod h1:1FEQNm3xlJgrMD+FBdI9+xvCksHtbpVBBw5dYhBSsks= -github.com/johnn193/api v0.0.0-20241231164642-99f059defc82 h1:V2fQZbYvvjDK/oz7LxluzuKXdFGljuvbpJhs7cqT2jo= -github.com/johnn193/api v0.0.0-20241231164642-99f059defc82/go.mod h1:g5eipXHNm0rQmW7DWya6avKcmzoypLmxnMlAaIsE5Ls= +github.com/johnn193/api v0.0.0-20250107153511-969bffad5413 h1:PCqsSwUxnzFbVY1u2GnxttuwrrCIGMd75mhroTGx42A= +github.com/johnn193/api v0.0.0-20250107153511-969bffad5413/go.mod h1:g5eipXHNm0rQmW7DWya6avKcmzoypLmxnMlAaIsE5Ls= github.com/jonboulle/clockwork v0.1.0/go.mod h1:Ii8DK3G1RaLaWxj9trq07+26W01tbo22gdxWY5EU2bo= github.com/jonboulle/clockwork v0.3.0 h1:9BSCMi8C+0qdApAp4auwX0RkLGUjs956h0EkuQymUhg= github.com/jonboulle/clockwork v0.3.0/go.mod h1:Pkfl5aHPm1nk2H9h0bjmnJD/BcgbGXUBGnn1kMkgxc8= diff --git a/services/discovery/client.go b/services/discovery/client.go index ee06084a6d5..2f60bf16cf1 100644 --- a/services/discovery/client.go +++ b/services/discovery/client.go @@ -56,7 +56,7 @@ func (c *client) DiscoverResources(ctx context.Context, extra map[string]any) ([ return nil, err } discoveredConfigs := []*resource.Config{} - protoConfigs := resp.GetDiscovery() + protoConfigs := resp.GetDiscoveries() for _, proto := range protoConfigs { config, err := config.ComponentConfigFromProto(proto) if err != nil { diff --git a/services/discovery/server.go b/services/discovery/server.go index b979028b765..cc88c444c78 100644 --- a/services/discovery/server.go +++ b/services/discovery/server.go @@ -50,7 +50,7 @@ func (server *serviceServer) DiscoverResources(ctx context.Context, req *pb.Disc protoConfigs = append(protoConfigs, proto) } - return &pb.DiscoverResourcesResponse{Discovery: protoConfigs}, nil + return &pb.DiscoverResourcesResponse{Discoveries: protoConfigs}, nil } // DoCommand receives arbitrary commands. diff --git a/services/discovery/server_test.go b/services/discovery/server_test.go index 8e68becc640..2eb02f5f11a 100644 --- a/services/discovery/server_test.go +++ b/services/discovery/server_test.go @@ -109,8 +109,8 @@ func TestDiscoverResources(t *testing.T) { } resp, err := discoveryServer.DiscoverResources(context.Background(), &pb.DiscoverResourcesRequest{Name: testDiscoveryName}) test.That(t, err, test.ShouldBeNil) - test.That(t, len(resp.GetDiscovery()), test.ShouldEqual, len(testComponents)) - for index, proto := range resp.GetDiscovery() { + test.That(t, len(resp.GetDiscoveries()), test.ShouldEqual, len(testComponents)) + for index, proto := range resp.GetDiscoveries() { expected := testComponents[index] test.That(t, proto.Name, test.ShouldEqual, expected.Name) actual, err := config.ComponentConfigFromProto(proto) From 6d59aec4ce23d294fd102a435434bf39264cd285 Mon Sep 17 00:00:00 2001 From: John Date: Tue, 7 Jan 2025 10:55:10 -0500 Subject: [PATCH 07/12] change interface to use slice of structs --- services/discovery/client.go | 6 +++--- services/discovery/client_test.go | 8 ++++---- services/discovery/discovery.go | 2 +- services/discovery/server.go | 2 +- services/discovery/server_test.go | 12 ++++++------ testutils/inject/discovery_service.go | 4 ++-- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/services/discovery/client.go b/services/discovery/client.go index 2f60bf16cf1..b2623eebd41 100644 --- a/services/discovery/client.go +++ b/services/discovery/client.go @@ -42,7 +42,7 @@ func NewClientFromConn( return c, nil } -func (c *client) DiscoverResources(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { +func (c *client) DiscoverResources(ctx context.Context, extra map[string]any) ([]resource.Config, error) { ctx, span := trace.StartSpan(ctx, "discovery::client::DoCommand") defer span.End() ext, err := protoutils.StructToStructPb(extra) @@ -55,14 +55,14 @@ func (c *client) DiscoverResources(ctx context.Context, extra map[string]any) ([ if err != nil { return nil, err } - discoveredConfigs := []*resource.Config{} + discoveredConfigs := []resource.Config{} protoConfigs := resp.GetDiscoveries() for _, proto := range protoConfigs { config, err := config.ComponentConfigFromProto(proto) if err != nil { return nil, err } - discoveredConfigs = append(discoveredConfigs, config) + discoveredConfigs = append(discoveredConfigs, *config) } return discoveredConfigs, nil } diff --git a/services/discovery/client_test.go b/services/discovery/client_test.go index 1a51362a80d..9c2747570c1 100644 --- a/services/discovery/client_test.go +++ b/services/discovery/client_test.go @@ -26,12 +26,12 @@ func TestClient(t *testing.T) { workingDiscovery := &inject.DiscoveryService{} failingDiscovery := &inject.DiscoveryService{} - testComponents := []*resource.Config{createTestComponent("component-1"), createTestComponent("component-2")} + testComponents := []resource.Config{createTestComponent("component-1"), createTestComponent("component-2")} - workingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { + workingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]resource.Config, error) { return testComponents, nil } - failingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { + failingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]resource.Config, error) { return nil, errDiscoverFailed } workingDiscovery.DoFunc = testutils.EchoFunc @@ -78,7 +78,7 @@ func TestClient(t *testing.T) { test.That(t, len(respDis), test.ShouldEqual, len(testComponents)) for index, actual := range respDis { expected := testComponents[index] - validateComponent(t, *actual, *expected) + validateComponent(t, actual, expected) } resp, err := workingDiscoveryClient.DoCommand(context.Background(), testutils.TestCommand) diff --git a/services/discovery/discovery.go b/services/discovery/discovery.go index 9c7ca428a37..31bfecad39a 100644 --- a/services/discovery/discovery.go +++ b/services/discovery/discovery.go @@ -46,5 +46,5 @@ func FromDependencies(deps resource.Dependencies, name string) (Service, error) // 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) + DiscoverResources(ctx context.Context, extra map[string]any) ([]resource.Config, error) } diff --git a/services/discovery/server.go b/services/discovery/server.go index cc88c444c78..dd4f96cfebc 100644 --- a/services/discovery/server.go +++ b/services/discovery/server.go @@ -43,7 +43,7 @@ func (server *serviceServer) DiscoverResources(ctx context.Context, req *pb.Disc } protoConfigs := []*apppb.ComponentConfig{} for _, cfg := range configs { - proto, err := config.ComponentConfigToProto(cfg) + proto, err := config.ComponentConfigToProto(&cfg) if err != nil { return nil, err } diff --git a/services/discovery/server_test.go b/services/discovery/server_test.go index 2eb02f5f11a..17631f23bff 100644 --- a/services/discovery/server_test.go +++ b/services/discovery/server_test.go @@ -33,7 +33,7 @@ var ( ) // this was taken from proto_conversions_test and represents all of the information that a discovery service can provide about a component. -func createTestComponent(name string) *resource.Config { +func createTestComponent(name string) resource.Config { testOrientation, _ := spatial.NewOrientationConfig(spatial.NewZeroOrientation()) testFrame := &referenceframe.LinkConfig{ @@ -79,7 +79,7 @@ func createTestComponent(name string) *resource.Config { Frame: testFrame, LogConfiguration: &resource.LogConfig{Level: logging.DEBUG}, } - return &testComponent + return testComponent } func newServer() (pb.DiscoveryServiceServer, *inject.DiscoveryService, *inject.DiscoveryService, error) { @@ -99,12 +99,12 @@ func newServer() (pb.DiscoveryServiceServer, *inject.DiscoveryService, *inject.D func TestDiscoverResources(t *testing.T) { discoveryServer, workingDiscovery, failingDiscovery, err := newServer() test.That(t, err, test.ShouldBeNil) - testComponents := []*resource.Config{createTestComponent("component-1"), createTestComponent("component-2")} + testComponents := []resource.Config{createTestComponent("component-1"), createTestComponent("component-2")} - workingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { + workingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]resource.Config, error) { return testComponents, nil } - failingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { + failingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]resource.Config, error) { return nil, errDiscoverFailed } resp, err := discoveryServer.DiscoverResources(context.Background(), &pb.DiscoverResourcesRequest{Name: testDiscoveryName}) @@ -115,7 +115,7 @@ func TestDiscoverResources(t *testing.T) { test.That(t, proto.Name, test.ShouldEqual, expected.Name) actual, err := config.ComponentConfigFromProto(proto) test.That(t, err, test.ShouldBeNil) - validateComponent(t, *actual, *expected) + validateComponent(t, *actual, expected) } respFail, err := discoveryServer.DiscoverResources(context.Background(), &pb.DiscoverResourcesRequest{Name: failDiscoveryName}) diff --git a/testutils/inject/discovery_service.go b/testutils/inject/discovery_service.go index ec9b8aaccad..5953a330ca7 100644 --- a/testutils/inject/discovery_service.go +++ b/testutils/inject/discovery_service.go @@ -11,7 +11,7 @@ import ( type DiscoveryService struct { discovery.Service name resource.Name - DiscoverResourcesFunc func(ctx context.Context, extra map[string]any) ([]*resource.Config, error) + DiscoverResourcesFunc func(ctx context.Context, extra map[string]any) ([]resource.Config, error) DoFunc func(ctx context.Context, cmd map[string]interface{}) (map[string]interface{}, error) } @@ -26,7 +26,7 @@ func (disSvc *DiscoveryService) Name() resource.Name { } // DiscoverResources calls the injected DiscoverResourcesFunc or the real version. -func (disSvc *DiscoveryService) DiscoverResources(ctx context.Context, extra map[string]any) ([]*resource.Config, error) { +func (disSvc *DiscoveryService) DiscoverResources(ctx context.Context, extra map[string]any) ([]resource.Config, error) { if disSvc.DiscoverResourcesFunc == nil { return disSvc.Service.DiscoverResources(ctx, extra) } From 451b0568f18c485781470e57cf2268e555d4efc8 Mon Sep 17 00:00:00 2001 From: John Date: Tue, 7 Jan 2025 11:34:33 -0500 Subject: [PATCH 08/12] add nil check --- services/discovery/client.go | 4 + services/discovery/discovery.go | 5 ++ services/discovery/server.go | 4 + services/discovery/server_test.go | 125 ++++++++++++++++-------------- 4 files changed, 79 insertions(+), 59 deletions(-) diff --git a/services/discovery/client.go b/services/discovery/client.go index b2623eebd41..6c804186fb5 100644 --- a/services/discovery/client.go +++ b/services/discovery/client.go @@ -57,6 +57,10 @@ func (c *client) DiscoverResources(ctx context.Context, extra map[string]any) ([ } discoveredConfigs := []resource.Config{} protoConfigs := resp.GetDiscoveries() + if protoConfigs == nil { + return nil, ErrNilResponse + } + for _, proto := range protoConfigs { config, err := config.ComponentConfigFromProto(proto) if err != nil { diff --git a/services/discovery/discovery.go b/services/discovery/discovery.go index 31bfecad39a..cfcb704a1d2 100644 --- a/services/discovery/discovery.go +++ b/services/discovery/discovery.go @@ -3,6 +3,7 @@ package discovery import ( "context" + "errors" pb "go.viam.com/api/service/discovery/v1" @@ -27,6 +28,10 @@ const ( // API is a variable that identifies the slam resource API. var API = resource.APINamespaceRDK.WithServiceType(SubtypeName) +var ( + ErrNilResponse = errors.New("discovery service returned a nil response") +) + // Named is a helper for getting the named service's typed resource name. func Named(name string) resource.Name { return resource.NewName(API, name) diff --git a/services/discovery/server.go b/services/discovery/server.go index dd4f96cfebc..60dee6f71d0 100644 --- a/services/discovery/server.go +++ b/services/discovery/server.go @@ -41,6 +41,10 @@ func (server *serviceServer) DiscoverResources(ctx context.Context, req *pb.Disc if err != nil { return nil, err } + if configs == nil { + return nil, ErrNilResponse + } + protoConfigs := []*apppb.ComponentConfig{} for _, cfg := range configs { proto, err := config.ComponentConfigToProto(&cfg) diff --git a/services/discovery/server_test.go b/services/discovery/server_test.go index 17631f23bff..89863b03a9c 100644 --- a/services/discovery/server_test.go +++ b/services/discovery/server_test.go @@ -96,77 +96,84 @@ func newServer() (pb.DiscoveryServiceServer, *inject.DiscoveryService, *inject.D return discovery.NewRPCServiceServer(injectSvc).(pb.DiscoveryServiceServer), injectDiscovery, injectDiscovery2, nil } -func TestDiscoverResources(t *testing.T) { +func TestDiscoveryServiceServer(t *testing.T) { discoveryServer, workingDiscovery, failingDiscovery, err := newServer() test.That(t, err, test.ShouldBeNil) testComponents := []resource.Config{createTestComponent("component-1"), createTestComponent("component-2")} - workingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]resource.Config, error) { - return testComponents, nil - } - failingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]resource.Config, error) { - return nil, errDiscoverFailed - } - resp, err := discoveryServer.DiscoverResources(context.Background(), &pb.DiscoverResourcesRequest{Name: testDiscoveryName}) - test.That(t, err, test.ShouldBeNil) - test.That(t, len(resp.GetDiscoveries()), test.ShouldEqual, len(testComponents)) - for index, proto := range resp.GetDiscoveries() { - expected := testComponents[index] - test.That(t, proto.Name, test.ShouldEqual, expected.Name) - actual, err := config.ComponentConfigFromProto(proto) + t.Run("Test DiscoverResources", func(t *testing.T) { + workingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]resource.Config, error) { + return testComponents, nil + } + failingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]resource.Config, error) { + return nil, errDiscoverFailed + } + resp, err := discoveryServer.DiscoverResources(context.Background(), &pb.DiscoverResourcesRequest{Name: testDiscoveryName}) + test.That(t, err, test.ShouldBeNil) + test.That(t, len(resp.GetDiscoveries()), test.ShouldEqual, len(testComponents)) + for index, proto := range resp.GetDiscoveries() { + expected := testComponents[index] + test.That(t, proto.Name, test.ShouldEqual, expected.Name) + actual, err := config.ComponentConfigFromProto(proto) + test.That(t, err, test.ShouldBeNil) + validateComponent(t, *actual, expected) + } + + respFail, err := discoveryServer.DiscoverResources(context.Background(), &pb.DiscoverResourcesRequest{Name: failDiscoveryName}) + test.That(t, err, test.ShouldEqual, errDiscoverFailed) + test.That(t, respFail, test.ShouldBeNil) + }) + t.Run("Test nil DiscoverResources response", func(t *testing.T) { + failingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]resource.Config, error) { + return nil, nil + } + resp, err := discoveryServer.DiscoverResources(context.Background(), &pb.DiscoverResourcesRequest{Name: failDiscoveryName}) + test.That(t, err, test.ShouldEqual, discovery.ErrNilResponse) + test.That(t, resp, test.ShouldEqual, nil) + }) + + t.Run("Test DoCommand", func(t *testing.T) { + workingDiscovery.DoFunc = func( + ctx context.Context, + cmd map[string]interface{}, + ) ( + map[string]interface{}, + error, + ) { + return cmd, nil + } + failingDiscovery.DoFunc = func( + ctx context.Context, + cmd map[string]interface{}, + ) ( + map[string]interface{}, + error, + ) { + return nil, errDoFailed + } + + commandStruct, err := protoutils.StructToStructPb(testutils.TestCommand) test.That(t, err, test.ShouldBeNil) - validateComponent(t, *actual, expected) - } - - respFail, err := discoveryServer.DiscoverResources(context.Background(), &pb.DiscoverResourcesRequest{Name: failDiscoveryName}) - test.That(t, err, test.ShouldEqual, errDiscoverFailed) - test.That(t, respFail, test.ShouldBeNil) -} - -func TestDiscoveryDo(t *testing.T) { - discoveryServer, workingDiscovery, failingDiscovery, err := newServer() - test.That(t, err, test.ShouldBeNil) - workingDiscovery.DoFunc = func( - ctx context.Context, - cmd map[string]interface{}, - ) ( - map[string]interface{}, - error, - ) { - return cmd, nil - } - failingDiscovery.DoFunc = func( - ctx context.Context, - cmd map[string]interface{}, - ) ( - map[string]interface{}, - error, - ) { - return nil, errDoFailed - } + req := commonpb.DoCommandRequest{Name: testDiscoveryName, Command: commandStruct} + resp, err := discoveryServer.DoCommand(context.Background(), &req) + test.That(t, err, test.ShouldBeNil) + test.That(t, resp, test.ShouldNotBeNil) + test.That(t, resp.Result.AsMap()["cmd"], test.ShouldEqual, testutils.TestCommand["cmd"]) + test.That(t, resp.Result.AsMap()["data"], test.ShouldEqual, testutils.TestCommand["data"]) - commandStruct, err := protoutils.StructToStructPb(testutils.TestCommand) - test.That(t, err, test.ShouldBeNil) + req = commonpb.DoCommandRequest{Name: failDiscoveryName, Command: commandStruct} + resp, err = discoveryServer.DoCommand(context.Background(), &req) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err.Error(), test.ShouldContainSubstring, errDoFailed.Error()) + test.That(t, resp, test.ShouldBeNil) + }) - req := commonpb.DoCommandRequest{Name: testDiscoveryName, Command: commandStruct} - resp, err := discoveryServer.DoCommand(context.Background(), &req) - test.That(t, err, test.ShouldBeNil) - test.That(t, resp, test.ShouldNotBeNil) - test.That(t, resp.Result.AsMap()["cmd"], test.ShouldEqual, testutils.TestCommand["cmd"]) - test.That(t, resp.Result.AsMap()["data"], test.ShouldEqual, testutils.TestCommand["data"]) - - req = commonpb.DoCommandRequest{Name: failDiscoveryName, Command: commandStruct} - resp, err = discoveryServer.DoCommand(context.Background(), &req) - test.That(t, err, test.ShouldNotBeNil) - test.That(t, err.Error(), test.ShouldContainSubstring, errDoFailed.Error()) - test.That(t, resp, test.ShouldBeNil) } // this was taken from proto_conversion_test.go -// -//nolint:thelper func validateComponent(t *testing.T, actual, expected resource.Config) { + t.Helper() test.That(t, actual.Name, test.ShouldEqual, expected.Name) test.That(t, actual.API, test.ShouldResemble, expected.API) test.That(t, actual.Model, test.ShouldResemble, expected.Model) From 6d431081874fb902c4951f23122f66edbd9bcd03 Mon Sep 17 00:00:00 2001 From: John Date: Tue, 7 Jan 2025 12:18:37 -0500 Subject: [PATCH 09/12] lint --- services/discovery/client_test.go | 17 +++++++++++++++++ services/discovery/discovery.go | 7 +++---- services/discovery/server_test.go | 3 +-- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/services/discovery/client_test.go b/services/discovery/client_test.go index 9c2747570c1..5221e463031 100644 --- a/services/discovery/client_test.go +++ b/services/discovery/client_test.go @@ -108,6 +108,23 @@ func TestClient(t *testing.T) { test.That(t, conn.Close(), test.ShouldBeNil) }) + t.Run("client tests for failing discovery due to nil response", func(t *testing.T) { + conn, err := viamgrpc.Dial(context.Background(), listener1.Addr().String(), logger) + test.That(t, err, test.ShouldBeNil) + failingDiscoveryClient, err := discovery.NewClientFromConn(context.Background(), conn, "", discovery.Named(failDiscoveryName), logger) + test.That(t, err, test.ShouldBeNil) + + failingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]resource.Config, error) { + return nil, nil + } + _, err = failingDiscoveryClient.DiscoverResources(context.Background(), nil) + test.That(t, err, test.ShouldNotBeNil) + test.That(t, err.Error(), test.ShouldContainSubstring, discovery.ErrNilResponse.Error()) + + test.That(t, failingDiscoveryClient.Close(context.Background()), test.ShouldBeNil) + test.That(t, conn.Close(), test.ShouldBeNil) + }) + t.Run("dialed client tests for working discovery", func(t *testing.T) { conn, err := viamgrpc.Dial(context.Background(), listener1.Addr().String(), logger) test.That(t, err, test.ShouldBeNil) diff --git a/services/discovery/discovery.go b/services/discovery/discovery.go index cfcb704a1d2..3572717576a 100644 --- a/services/discovery/discovery.go +++ b/services/discovery/discovery.go @@ -25,12 +25,11 @@ const ( SubtypeName = "discovery" ) -// API is a variable that identifies the slam resource API. +// API is a variable that identifies the discovery resource API. var API = resource.APINamespaceRDK.WithServiceType(SubtypeName) -var ( - ErrNilResponse = errors.New("discovery service returned a nil response") -) +// ErrNilResponse is the error for when a nil response is returned from a discovery service +var ErrNilResponse = errors.New("discovery service returned a nil response") // Named is a helper for getting the named service's typed resource name. func Named(name string) resource.Name { diff --git a/services/discovery/server_test.go b/services/discovery/server_test.go index 89863b03a9c..89fc4b0046d 100644 --- a/services/discovery/server_test.go +++ b/services/discovery/server_test.go @@ -168,10 +168,9 @@ func TestDiscoveryServiceServer(t *testing.T) { test.That(t, err.Error(), test.ShouldContainSubstring, errDoFailed.Error()) test.That(t, resp, test.ShouldBeNil) }) - } -// this was taken from proto_conversion_test.go +// this was taken from proto_conversion_test.go. func validateComponent(t *testing.T, actual, expected resource.Config) { t.Helper() test.That(t, actual.Name, test.ShouldEqual, expected.Name) From 3557ee8bcaf1b56e6ff6ae702d12e0b43fbeae5b Mon Sep 17 00:00:00 2001 From: John Date: Tue, 7 Jan 2025 13:07:15 -0500 Subject: [PATCH 10/12] bump api --- go.mod | 4 +--- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 4c989b7184f..83e5680b9af 100644 --- a/go.mod +++ b/go.mod @@ -75,7 +75,7 @@ require ( go.uber.org/atomic v1.11.0 go.uber.org/multierr v1.11.0 go.uber.org/zap v1.27.0 - go.viam.com/api v0.1.372 + go.viam.com/api v0.1.380 go.viam.com/test v1.2.4 go.viam.com/utils v0.1.118 goji.io v2.0.2+incompatible @@ -431,5 +431,3 @@ 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-20250107153511-969bffad5413 diff --git a/go.sum b/go.sum index 61c858dc29d..55b2f7196c5 100644 --- a/go.sum +++ b/go.sum @@ -788,8 +788,6 @@ github.com/jmespath/go-jmespath v0.4.0/go.mod h1:T8mJZnbsbmF+m6zOOFylbeCJqk5+pHW github.com/jmespath/go-jmespath/internal/testify v1.5.1 h1:shLQSRRSCCPj3f2gpwzGwWFoC7ycTf1rcQZHOlsJ6N8= github.com/jmespath/go-jmespath/internal/testify v1.5.1/go.mod h1:L3OGu8Wl2/fWfCI6z80xFu9LTZmf1ZRjMHUOPmWr69U= github.com/jmoiron/sqlx v1.2.0/go.mod h1:1FEQNm3xlJgrMD+FBdI9+xvCksHtbpVBBw5dYhBSsks= -github.com/johnn193/api v0.0.0-20250107153511-969bffad5413 h1:PCqsSwUxnzFbVY1u2GnxttuwrrCIGMd75mhroTGx42A= -github.com/johnn193/api v0.0.0-20250107153511-969bffad5413/go.mod h1:g5eipXHNm0rQmW7DWya6avKcmzoypLmxnMlAaIsE5Ls= github.com/jonboulle/clockwork v0.1.0/go.mod h1:Ii8DK3G1RaLaWxj9trq07+26W01tbo22gdxWY5EU2bo= github.com/jonboulle/clockwork v0.3.0 h1:9BSCMi8C+0qdApAp4auwX0RkLGUjs956h0EkuQymUhg= github.com/jonboulle/clockwork v0.3.0/go.mod h1:Pkfl5aHPm1nk2H9h0bjmnJD/BcgbGXUBGnn1kMkgxc8= @@ -1515,6 +1513,8 @@ go.uber.org/zap v1.18.1/go.mod h1:xg/QME4nWcxGxrpdeYfq7UvYrLh66cuVKdrbD1XF/NI= go.uber.org/zap v1.23.0/go.mod h1:D+nX8jyLsMHMYrln8A0rJjFt/T/9/bGgIhAqxv5URuY= go.uber.org/zap v1.27.0 h1:aJMhYGrd5QSmlpLMr2MftRKl7t8J8PTZPA732ud/XR8= go.uber.org/zap v1.27.0/go.mod h1:GB2qFLM7cTU87MWRP2mPIjqfIDnGu+VIO4V/SdhGo2E= +go.viam.com/api v0.1.380 h1:VgRHDlPBku+kjIp4omxmPNmRVZezytFUUOFJ2xpRFR8= +go.viam.com/api v0.1.380/go.mod h1:g5eipXHNm0rQmW7DWya6avKcmzoypLmxnMlAaIsE5Ls= go.viam.com/test v1.2.4 h1:JYgZhsuGAQ8sL9jWkziAXN9VJJiKbjoi9BsO33TW3ug= go.viam.com/test v1.2.4/go.mod h1:zI2xzosHdqXAJ/kFqcN+OIF78kQuTV2nIhGZ8EzvaJI= go.viam.com/utils v0.1.118 h1:Kp6ebrCBiYReeSC1XnWPTjtBJoTUsQ6YWAomQkQF/mE= From dc66eebc2e73ed37a730c0082123607a4ff0c1d1 Mon Sep 17 00:00:00 2001 From: John Date: Tue, 7 Jan 2025 13:19:03 -0500 Subject: [PATCH 11/12] lint --- app/billing_client.go | 2 +- app/billing_client_test.go | 4 ++++ services/discovery/discovery.go | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/app/billing_client.go b/app/billing_client.go index bfab8670353..d6cd7e981bb 100644 --- a/app/billing_client.go +++ b/app/billing_client.go @@ -192,7 +192,7 @@ func (c *BillingClient) SendPaymentRequiredEmail(ctx context.Context, customerOr } func usageCostTypeFromProto(costType pb.UsageCostType) UsageCostType { - //nolint:exhaustive + //nolint:exhaustive,deprecated,staticcheck switch costType { case pb.UsageCostType_USAGE_COST_TYPE_UNSPECIFIED: return UsageCostTypeUnspecified diff --git a/app/billing_client_test.go b/app/billing_client_test.go index b79fe387da0..1cba60d0420 100644 --- a/app/billing_client_test.go +++ b/app/billing_client_test.go @@ -101,18 +101,22 @@ func usageCostTypeToProto(costType UsageCostType) pb.UsageCostType { case UsageCostTypeUnspecified: return pb.UsageCostType_USAGE_COST_TYPE_UNSPECIFIED case UsageCostTypeDataUpload: + //nolint:deprecated,staticcheck return pb.UsageCostType_USAGE_COST_TYPE_DATA_UPLOAD case UsageCostTypeDataEgress: + //nolint:deprecated,staticcheck return pb.UsageCostType_USAGE_COST_TYPE_DATA_EGRESS case UsageCostTypeRemoteControl: return pb.UsageCostType_USAGE_COST_TYPE_REMOTE_CONTROL case UsageCostTypeStandardCompute: return pb.UsageCostType_USAGE_COST_TYPE_STANDARD_COMPUTE case UsageCostTypeCloudStorage: + //nolint:deprecated,staticcheck return pb.UsageCostType_USAGE_COST_TYPE_CLOUD_STORAGE case UsageCostTypeBinaryDataCloudStorage: return pb.UsageCostType_USAGE_COST_TYPE_BINARY_DATA_CLOUD_STORAGE case UsageCostTypeOtherCloudStorage: + //nolint:deprecated,staticcheck return pb.UsageCostType_USAGE_COST_TYPE_OTHER_CLOUD_STORAGE case UsageCostTypePerMachine: return pb.UsageCostType_USAGE_COST_TYPE_PER_MACHINE diff --git a/services/discovery/discovery.go b/services/discovery/discovery.go index 3572717576a..981c4341259 100644 --- a/services/discovery/discovery.go +++ b/services/discovery/discovery.go @@ -28,7 +28,7 @@ const ( // API is a variable that identifies the discovery resource API. var API = resource.APINamespaceRDK.WithServiceType(SubtypeName) -// ErrNilResponse is the error for when a nil response is returned from a discovery service +// ErrNilResponse is the error for when a nil response is returned from a discovery service. var ErrNilResponse = errors.New("discovery service returned a nil response") // Named is a helper for getting the named service's typed resource name. From 78a45dd5be8fd4bc0a4a47f08badfefc2821caba Mon Sep 17 00:00:00 2001 From: John Date: Tue, 7 Jan 2025 17:01:58 -0500 Subject: [PATCH 12/12] address comments --- services/discovery/client.go | 2 +- services/discovery/client_test.go | 8 ++++---- services/discovery/server_test.go | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/services/discovery/client.go b/services/discovery/client.go index 6c804186fb5..08db3f6097e 100644 --- a/services/discovery/client.go +++ b/services/discovery/client.go @@ -55,12 +55,12 @@ func (c *client) DiscoverResources(ctx context.Context, extra map[string]any) ([ if err != nil { return nil, err } - discoveredConfigs := []resource.Config{} protoConfigs := resp.GetDiscoveries() if protoConfigs == nil { return nil, ErrNilResponse } + discoveredConfigs := []resource.Config{} for _, proto := range protoConfigs { config, err := config.ComponentConfigFromProto(proto) if err != nil { diff --git a/services/discovery/client_test.go b/services/discovery/client_test.go index 5221e463031..761d6527d99 100644 --- a/services/discovery/client_test.go +++ b/services/discovery/client_test.go @@ -23,18 +23,18 @@ func TestClient(t *testing.T) { rpcServer, err := rpc.NewServer(logger, rpc.WithUnauthenticated()) test.That(t, err, test.ShouldBeNil) - workingDiscovery := &inject.DiscoveryService{} - failingDiscovery := &inject.DiscoveryService{} - testComponents := []resource.Config{createTestComponent("component-1"), createTestComponent("component-2")} + workingDiscovery := inject.NewDiscoveryService(testDiscoveryName) workingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]resource.Config, error) { return testComponents, nil } + workingDiscovery.DoFunc = testutils.EchoFunc + + failingDiscovery := inject.NewDiscoveryService(failDiscoveryName) failingDiscovery.DiscoverResourcesFunc = func(ctx context.Context, extra map[string]any) ([]resource.Config, error) { return nil, errDiscoverFailed } - workingDiscovery.DoFunc = testutils.EchoFunc failingDiscovery.DoFunc = func( ctx context.Context, cmd map[string]interface{}, diff --git a/services/discovery/server_test.go b/services/discovery/server_test.go index 89fc4b0046d..f7e1ffeb97a 100644 --- a/services/discovery/server_test.go +++ b/services/discovery/server_test.go @@ -83,8 +83,8 @@ func createTestComponent(name string) resource.Config { } func newServer() (pb.DiscoveryServiceServer, *inject.DiscoveryService, *inject.DiscoveryService, error) { - injectDiscovery := &inject.DiscoveryService{} - injectDiscovery2 := &inject.DiscoveryService{} + injectDiscovery := inject.NewDiscoveryService(testDiscoveryName) + injectDiscovery2 := inject.NewDiscoveryService(failDiscoveryName) resourceMap := map[resource.Name]discovery.Service{ discovery.Named(testDiscoveryName): injectDiscovery, discovery.Named(failDiscoveryName): injectDiscovery2,