-
Notifications
You must be signed in to change notification settings - Fork 0
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
Adds minimal controller interface #193
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #193 +/- ##
==========================================
- Coverage 82.00% 81.95% -0.05%
==========================================
Files 31 32 +1
Lines 400 410 +10
==========================================
+ Hits 328 336 +8
- Misses 72 74 +2 ☔ View full report in Codecov by Sentry. |
Thanks for this @dalonsoa. I hadn't clocked that the ControllerDriver doesn't use async method so that makes the interface a bit simpler indeed. I've reproduced the same error as you report with the drunc-lite profile with the full drunc image so I think this is a potential bug with drunc. It may be because we're using a slightly older release of drunc. Could you please raise with the DUNE team? |
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.
drunc controller get status aside, the code looks fine. 👍
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.
LGTM
Needs #203, at least, to prove this works. So it might take a while to actually do anything. Having said that, this code is, as far as I can tell, correct even if we cannot use it. Would it be a problem to merge the PR, so this interface is in place, if just as a placeholder, while we develop the rest of the functionality that depends on it, @cc-a ? |
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.
I tried reviewing this but cannot because of #209
|
||
mock = mocker.patch("controller.controller_interface.get_controller_driver") | ||
get_controller_status() | ||
mock.assert_called_once() |
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.
Being pedantic, we probably want to check that the get_status()
method was called and not just get_controller_driver
Description
Adds a minimal code to interact with the Controller driver. This seems way simpler than the process manager - or I'm missing something.
I've added a script, as indicated in #160 , but when I run it using the
drunc-lite
profile - which I believe does not have the controller, anyway - I get the following error:And I cannot use the normal
drunc
profile because of the error reported in #187 . So, I'm not entirely sure at this point if things are actually working or what the output ofget_status
looks like.Fixes #160
Type of change
Key checklist
python -m pytest
)python -m sphinx -b html docs docs/build
)pre-commit run --all-files
)Further checks