[RSDK-507] Dependencies in service config #1518
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Kschappacher
added
the
safe to test
viambot
removed
the
safe to test
Kschappacher
added
the
safe to test
viambot
removed
the
safe to test
Kschappacher
added
the
safe to test
viambot
removed
the
safe to test
Kschappacher
added
the
safe to test
viambot
removed
the
safe to test
Kschappacher
added
the
safe to test
Kschappacher
added
safe to test
viambot
removed
the
safe to test
Kschappacher
added
the
safe to test
viambot
removed
the
safe to test
Kschappacher
added
the
safe to test
|
gonna switch myself out for @maximpertsov |
ghost
suggested changes
Nov 3, 2022
There was a problem hiding this comment.
Spoke with Kurt regarding some SLAM changes which would store camera components instead of the camera stream as was done previously. He is reverting some of those changes until SLAM has had a chance to discuss internally and will make required changes ourselves should we decide to go down that route.
Will re-review once new push is submitted.
maximpertsov
reviewed
Nov 3, 2022
viambot
removed
the
safe to test
Kschappacher
added
the
safe to test
viambot
removed
the
safe to test
Kschappacher
added
the
safe to test
maximpertsov
reviewed
Nov 7, 2022
| default: | ||
| return nil, rdkutils.NewResourceNotFoundError(name) | ||
| deps[camera.Named(sensor)] = cam |
There was a problem hiding this comment.
just to verify, should we still be adding a camera to deps here? it looks like this threw an error before
There was a problem hiding this comment.
NIT: oh i see, there's a new case for a sensor that is supposed to be missing (gibberish) - I think it's clearer to just to not add the camera to deps in the default case instead of having a specific case for not adding. i'll defer to slam team on this though since it's their test
There was a problem hiding this comment.
We will discuss it internally with the team but for now, don't want to hold back Kurt's PR since it is correctly updating the code with the current way we are testing.
Thanks for bring it to my attention though Maxim!
maximpertsov
approved these changes
Nov 7, 2022
ghost
reviewed
Nov 7, 2022
| @@ -527,7 +529,7 @@ func TestGeneralNew(t *testing.T) { | |||
| // Create slam service | |||
| logger := golog.NewTestLogger(t) | |||
| _, err := createSLAMService(t, attrCfg, logger, false, false) | |||
| test.That(t, fmt.Sprint(err), test.ShouldContainSubstring, "error with slam service slam process:") | |||
| test.That(t, fmt.Sprint(err), test.ShouldContainSubstring, "runtime slam service error: invalid slam algorithm \"test\"") | |||
There was a problem hiding this comment.
[q] is this error change due to a change in error logging from the switch to dependencies as i don't see it reflected in the slam code
There was a problem hiding this comment.
Yeah I think there was a bug in this test. The original test didnt have a valid camera so it skipped the runtimeServiceValidation check because of this line: https://github.com/viamrobotics/rdk/blob/main/services/slam/builtin/builtin.go#L195. So I added a valid camera and the test get caught because of the invalid algo now
ghost
approved these changes
Nov 7, 2022
viambot
removed
the
safe to test
Kschappacher
added
the
safe to test
|
Note: merge base coverage results not available, comparing against closest 38fa524~1=(3aa621d) instead
|
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add implicit depends on to all services