refactor(update-server): Split name_management into submodules
#10523
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
SyntaxColoring
added
refactor
update server
robot-svcs
Codecov Report
@@ Coverage Diff @@
## avahi_name_conflict #10523 +/- ##
====================================================
Coverage 73.67% 73.68%
====================================================
Files 2138 2141 +3
Lines 57457 57469 +12
Branches 5777 5777
====================================================
+ Hits 42333 42344 +11
- Misses 13917 13918 +1
Partials 1207 1207
Flags with carried forward coverage won't be shown. Click here to find out more.
|
mcous
approved these changes
May 31, 2022
| try: | ||
| import dbus | ||
|
|
||
| class DBusState: |
There was a problem hiding this comment.
Adding this note for future cleanup since this PR only moved this code, it did not introduce it: a whole class definition inside a try is a little funky. Can this be done some other way?
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.
Overview
Refactors to
update-server, in pursuit of a fix for #10126.Note that this PR will merge into the
avahi_name_conflictfeature branch, not intoedge. (This is per discussion on the Robot Services team. We're trying it out!)Changelog
The
otupdate.common.name_managementmodule was getting unwieldy, so this PR splits it into several submodules:otupdate.common.name_management: Top-level HTTP endpoint functions; an English overview of the robot's various names; and selected re-exports from submodules.otupdate.common.name_management.avahi: Restarting the Avahi daemon and telling the Avahi daemon what service name it should use when advertising the machine.otupdate.common.name_management.pretty_hostname: Reading and storing the machine's pretty hostname.otupdate.common.name_management.static_hostname: Setting up the machine's static hostname.No behavioral code changes. Just moving stuff around.
Review requests
Do the names and organization make sense?
Do the docstrings make sense?
In particular, each submodule points back to the main
otupdate.common.name_managementdocstring for background information on the various names. Would it be clearer if this relationship were reversed, and the names were canonically explained in the various submodules?The distinctions between the different names are both important and subtle, so I feel like it's important to organize this documentation well.
I wouldn't bother testing this on a robot just yet. This PR is merging into a feature branch, not into
edge, and it's going to be followed by more meaningful behavioral changes. Those changes will have to be tested thoroughly, so we may as well wait to test the whole thing then.Risk assessment
Low so far, since there are no behavioral code changes.