Finish removing teuthology-worker #1960
Conversation
zmc
force-pushed
the
worker-supervisor
branch
4 times, most recently
from
13ca937
to
c531ecb
Compare
|
@kamoltat @VallariAg I was mistaken when I mentioned a test regression in this PR; it's ready for review! |
|
with docopt it looked nicer imho |
|
the head of PR: "Finish removing teuthology-worker" does not reflect what is in this patch, in fact there is introducing teuthology-supervisor as separate tool, dropping worker may be probably in separate PR. |
|
Why underscoring unit tests by the way? Following the logic, other modules also must have underscore suffixes? |
|
@kshtsk I think you're ok with removing the worker at this point, is that the case? Also, responses to your earlier comments:
It may, but it's less flexible and harder to maintain
That bit is just adding the entry point; the tool has been in use for quite some time, sharing
I didn't claim the worker tests weren't passing, only that the dispatcher and supervisor were not being tested.
Sorry I'm not sure what you're asking here |
if you want to add test for the scripts, I suggest to use test_script_dispatcher.py and test_script_supervisor.py for them and test_dispatcher.py and test_supervisor.py leave for the modules, otherwise you're breaking name conventions. |
okay, that sounds reasonable, but then we need to refactor the other scripts too, in scheduled manner, probably someone would love to volunteer to do it |
The dispatcher and supervisor were added in #1546, but code was copied and pasted into the new modules, leaving the worker untouched. Also untouched were the unit tests, meaning that the dispatcher and supervisor were never unit tested. As the copied code changed, the dispatcher and supervisor were not being tested for regressions, while the worker - which wasn't being anymore - had passing unit tests, giving some false sense of security. This commit removes the old worker code, and adapts the old worker tests to apply to the dispatcher and supervisor. It also splits out teuthology-supervisor into its own command. Signed-off-by: Zack Cerza <[email protected]>
The old dispatcher expects to be able to invoke the supervisor via `teuthology-dispatcher --supervisor`, so add this compatibility shim for the time being. Signed-off-by: Zack Cerza <[email protected]>
Originally, #1960 was intended to do this, but in some review back-and-forth the commit removing the file was dropped. Let's actually remove the file. Signed-off-by: Zack Cerza <[email protected]>
Originally, #1960 was intended to do this, but in some review back-and-forth the commit removing the file was dropped. Let's actually remove the file. Signed-off-by: Zack Cerza <[email protected]>
Originally, ceph#1960 was intended to do this, but in some review back-and-forth the commit removing the file was dropped. Let's actually remove the file. Signed-off-by: Zack Cerza <[email protected]>
The dispatcher and supervisor were added in #1546, but code was copied and pasted into the new modules, leaving the worker untouched. Also untouched were the unit tests, meaning that the dispatcher and supervisor were never unit tested. As the copied code changed, the dispatcher and supervisor were not being tested for regressions, while the worker - which wasn't being anymore - had passing unit tests, giving some false sense of security.
This commit removes the old worker code, and adapts the old worker tests to apply to the dispatcher and supervisor. It also splits out teuthology-supervisor into its own command.