-
Notifications
You must be signed in to change notification settings - Fork 179
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
feat(notify-server): Delete notify-server #14280
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14280 +/- ##
==========================================
- Coverage 68.24% 68.23% -0.02%
==========================================
Files 2512 2512
Lines 71877 71868 -9
Branches 9175 9175
==========================================
- Hits 49055 49041 -14
- Misses 20656 20661 +5
Partials 2166 2166
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ca8b8cc
to
e008992
Compare
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 looks good and shouldn't affect robot operations but still we should check on robots (esp the systemd notifications part). I can do an OT2 test if you haven't already.
I’m seeing robot server startup failures. I think they’re related to the general dependency upgrades, outside of this PR, but we should hold off on merging until that’s confirmed. |
Resolve conflicts in: * api/release-notes.md * robot-server/Pipfile * robot-server/Pipfile.lock
Resolve conflicts in Pipfile.lock.
Overview
This deletes the
notify-server
project.Years ago, we added
notify-server
because we thought it would be helpful for getting the robot to provide realtime notifications to clients. However, shifting priorities meant that it never developed beyond a proof-of-concept. It let you listen for door-opened/door-closed events, but that's it.Realtime notifications are getting renewed interest, but we no longer think
notify-server
is the way to do them.Deleting the project saves some CI time and relieves us from maintaining its dependencies.
This PR has an OT-2 Buildroot co-dependency: Opentrons/buildroot#215
Test Plan
Grab images from the build server (
ot2-system.zip
/system-update.zip
) and push them to an OT-2 and Flex. Make sure robot-server still starts, andjournalctl
doesn't show any systemd errors complaining about a missing unit or anything like that.Changelog
The user-facing changes entailed by deleting this project are:
/notifications/subscribe
WebSocket endpoint. As far as I can tell, it was not publicly documented.notify_server
package and script from the robot. These were described innotify-server/README.rst
, but otherwise not advertised, as far as I can tell.There is currently no replacement for these removals, although we are planning one. Meanwhile, I recommend simply polling
GET /robot/door/status
.Review requests
If anyone has reason to believe that anyone cares about these features, speak now or forever fold your pizza.
Risk assessment
Very low, assuming no one cares about these features.