-
Notifications
You must be signed in to change notification settings - Fork 125
RSDK-12105 + RSDK-12106: Require min viam-server version for reload and detect both shell type/api #5353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RSDK-12105 + RSDK-12106: Require min viam-server version for reload and detect both shell type/api #5353
Conversation
|
ping @gmulz @stuqdog @abe-winter. LMK if you have any concerns with the changes around requiring a min version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally lgtm though I want to make sure this doesn't make it impossible to do a reload on a dev CLI build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a very cool ergonomics feature
| ) | ||
| if slices.ContainsFunc(services, func(service ResourceMap) bool { return service["type"] == "shell" }) { | ||
| if slices.ContainsFunc(services, func(service ResourceMap) bool { | ||
| return service["type"] == "shell" || service["api"] == "rdk:service:shell" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guessing this is not wrong but for my learning, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of historical change. The type field was before the RDK supported modules. It was how the config told the RDK what builtin to run. When modules were released, we started supporting the new api field with a triple. For a long time afterwards, fleet would still write "type" and would not write the triple into config for builtins, even though it was converted to api and a triple in the config retriever. We changed that in fleet in the past year so the config is more WYSIWYG and less confusing with docs and seeing logs from the RDK. So at this point, its an upgrade to make sure all things which write config to the part, do so in the new way consistently
I tested hot reloading on an old machine mistakenly that did not yet support the tilde file path. It was on v75. I spent a bit trying to find the exact version, which should be v73 but the fact that it didn't work on v75 concerns me. Taking Tenzing's advice and enforcing that v90 has to be installed, which is the version that hot reloading with cloud build was introduced.
Grant also brought to my attention that service config is being added to parts with the old "type" field instead of "api". I made code changes to detect the shell service in either state, and now start adding shell service with the "api" field.
Testing
Service config as a result (after deleting):