[RSDK-515] Modular Resources #1363
Conversation
|
|
| @@ -21,7 +21,7 @@ | |||
| { | |||
| "name": "base1", | |||
| "type": "base", | |||
| "model": "fake", | |||
| "model": "acme:test:model", | |||
There was a problem hiding this comment.
assuming making these changes doesn't have us lose test coverage in other places.
There was a problem hiding this comment.
Not as far as I know. If find a case where you think it does, please let me know.
| return err | ||
| web, err := r.webService() | ||
| if err == nil { | ||
| web.Stop() |
There was a problem hiding this comment.
so we Stop and Close web?
There was a problem hiding this comment.
Yes, Stop() stops just the network facing web service, preventing any further client connections, while leaving the module grpc running, in case teardown needs to communicate with modules. (E.g. a close on a base might want to send a Stop() to its motors.) The final close then shuts down everything.
| } | ||
|
|
||
| switch req["command"] { | ||
| case "sleep": |
resource/model.go
Outdated
| return err | ||
| } | ||
| if m.Name == "" { | ||
| return errors.New("model name field for resource missing") |
There was a problem hiding this comment.
isn't it just name missing?
There was a problem hiding this comment.
Until one of your previous comments today, I didn't realize we tried to use the json field names in errors. I was just using plain english and saying "model name". Reworded it to say "name field for model missing"
components/arm/server.go
Outdated
|
|
||
| // IsMoving queries of a component is in motion. | ||
| func (s *subtypeServer) IsMoving(ctx context.Context, req *pb.IsMovingRequest) (*pb.IsMovingResponse, error) { | ||
| base, err := s.getArm(req.GetName()) |
components/gripper/server.go
Outdated
|
|
||
| // IsMoving queries of a component is in motion. | ||
| func (s *subtypeServer) IsMoving(ctx context.Context, req *pb.IsMovingRequest) (*pb.IsMovingResponse, error) { | ||
| base, err := s.getGripper(req.GetName()) |
| @@ -63,6 +64,12 @@ func (c *Config) Ensure(fromCloud bool) error { | |||
| } | |||
| } | |||
|
|
|||
| for idx := 0; idx < len(c.Modules); idx++ { | |||
| if err := c.Modules[idx].Validate(fmt.Sprintf("%s.%d", "modules", idx)); err != nil { | |||
| return err | |||
There was a problem hiding this comment.
since we allow partial start up now, should we error out just because a module config is wrong?
viambot
added
safe to test
There was a problem hiding this comment.
I believe I've addressed everything from this round of reviews, except for two pending questions. One to @npmenard about subtype, as I want confirmation the tests I added there cover the case he's concerned about (and therefore it wasn't a problem.) Second, @edaniels I have questions on how you want me to approach the web_test.go comment about opID testing on streams.
Otherwise, hopefully all good. I'll get to the api/goutils comments in the morning.
| @@ -21,7 +21,7 @@ | |||
| { | |||
| "name": "base1", | |||
| "type": "base", | |||
| "model": "fake", | |||
| "model": "acme:test:model", | |||
There was a problem hiding this comment.
Not as far as I know. If find a case where you think it does, please let me know.
|
|
||
| if config.Model.Namespace == "" { | ||
| config.Model.Namespace = resource.ResourceNamespaceRDK | ||
| config.Model.ModelFamily = resource.DefaultModelFamily |
There was a problem hiding this comment.
I wrote a couple of short paragraphs at the top of resource/resource.go so please look over those there.
resource/model.go
Outdated
| return err | ||
| } | ||
| if m.Name == "" { | ||
| return errors.New("model name field for resource missing") |
There was a problem hiding this comment.
Until one of your previous comments today, I didn't realize we tried to use the json field names in errors. I was just using plain english and saying "model name". Reworded it to say "name field for model missing"
module/modmanager/manager_test.go
Outdated
| Name: "simple-module", | ||
| ExePath: modExe, | ||
| } | ||
| err := mgr.AddModule(ctx, modCfg) |
There was a problem hiding this comment.
I didn't realize subtests fired off in goroutines, I thought it was JUST a labeling thing. Switched to t.Log() for that, as I think this is better to test in sequence. (Rather than setup/teardown for every function.)
| and useful mostly for organization/grouping. Note that each "tier" contains the tier to the left it. Such that ModelFamily contains | ||
| Namespace, and Model itself contains ModelFamily. | ||
|
|
||
| An example resource (say, a motor) may use the motor API, and thus have the Subtype "rdk:component:motor" and have a model such as |
|
left remaining comments! thanks for that quick turnaround time |
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
|
Lots of changes involved, so I'm not expecting any speedy reviews here. Also, I'd like at least one solid pass of reviews (especially about the higher level architecture) before starting to create proper tests for the new module methods as well.
As for where to look, two main places to start with: There is a new "module" section in the top level, which includes a doc.go file with a fair bit of explanation for how this is expected to work. https://github.com/Otterverse/rdk/blob/modular-resources/module/doc.go
The most obvious entrypoint though, is a through set of examples collected in https://github.com/Otterverse/rdk/tree/modular-resources/examples/customresources There is a README there with instructions for how to run things, and it's all tied up in a bow with makefile targets, so it should "just run."
Other points of interest include:
Lots of other changes (obviously) as well. Please don't hesitate to ask me questions via Slack! This needs to all make sense to everyone, and be the right way forward.
Also, note that this depends on patches to both api and goutils. PRs are in place for those as well.