CCI-MOC / hil

Hardware Isolation Layer, formerly Hardware as a Service
Apache License 2.0
24 stars 54 forks source link

Move show_console to the client library #972

Closed xuhang57 closed 6 years ago

xuhang57 commented 6 years ago

Simply move show_console to the client library.

Relates to: https://github.com/CCI-MOC/hil/issues/924

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 1713


Changes Missing Coverage Covered Lines Changed/Added Lines %
hil/cli.py 0 1 0.0%
hil/ext/obm/mock.py 6 7 85.71%
<!-- Total: 9 11 81.82% -->
Files with Coverage Reduction New Missed Lines %
hil/ext/obm/ipmi.py 2 53.85%
<!-- Total: 2 -->
Totals Coverage Status
Change from base Build 1686: 0.2%
Covered Lines: 2109
Relevant Lines: 3540

💛 - Coveralls
naved001 commented 6 years ago

just some pep8 failures. You also might wanna add a unit test for client library.

xuhang57 commented 6 years ago

Hmm, I am still getting a

FailedAPICallException: The console log for node-01 does not exist.

I started the console before I show it. Did I miss anything here?

zenhack commented 6 years ago

If it's using the mock obm driver, it looks like actually showing the console isn't implemented (hil/ext/obmd/mock.py). If not for the fact that we're looking at migrating stuff to obmd soonish, I might think it was worth stubbing more of this out and maybe putting some logic in there to just e.g. set a boolean when start/stop console are called, and check it in get_console, returning ether None or a string depending. That may actually not be a whole lot of work, in which case it might make sense to do it anyway, but it also seems sensible to just say screw it, and revisit the coverage of the console related calls when we re-do that API.

naved001 commented 6 years ago

I am happy with this PR, this seems simple enough to me. We can either update the mock driver to handle get_console or revisit it later (meaning not have the client lib test for it). I am fine either ways. Alternatively, can we use the ipmi driver with dry_run set to true?

xuhang57 commented 6 years ago

Thanks, guys!

naved001 commented 6 years ago

Turns out the client library was using the ipmi driver rather than the mock driver. Any reason? I changed it to use the mock driver and put some logic like you suggested @zenhack

naved001 commented 6 years ago

On a slightly related note, I don't understand why the client library is registering all sorts of drivers when it's not using those. What's the intention here?

naved001 commented 6 years ago

@zenhack done.

I'll let @xuhang57 merge master into this (or rebase) after #978 is in.

zenhack commented 6 years ago

I'm happy.