-
Notifications
You must be signed in to change notification settings - Fork 4
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
HilNetworkDriver implementation #67
Conversation
This is HilNetworkDriver implementations. It uses fuctions from hilapi.py and from HilInventoryDriver. Since HilInventoryDriver the following functions relying on HilInventoryDriver are also not implemented: __read_current_state(self, argv) and __write_current_state(self, argv)
There was a problem with importing NetworkService
This variable was hardcoded. Now it's not necessary
As Peter suggested we moved these functions to hilapi.
Without sleep detach and attach node don't work properly
time.sleep(1) | ||
|
||
node_nics = hilapi.show_node(host_to_move)['nics'] | ||
print("TESTING: After move " + str(node_nics)) |
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.
Perhaps put this under a debug conditional, like: if _DEBUG > 0:
and then have a module global you can play with if necessary.
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.
Will do this.
|
||
def __move_one_host(self, argv): | ||
if len(argv) != 3: | ||
sys.exit("Incorrect number of arguments. Should be host_to_move old_cloud new_cloud.") |
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.
Call sys.exit
in a library makes it difficult to know how to use the library in different situations. Instead, raise and exception.
However, if __move_one_host
is only called within this module, why not just explicitly list all the arguments? Python has variable length argument support built in, so using a list is not considered "pythonic".
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.
Will do this.
@@ -10,25 +10,14 @@ | |||
import logging | |||
from subprocess import call | |||
from subprocess import check_call | |||
from os import path | |||
|
|||
sys.path.append(path.dirname(path.dirname(path.abspath(__file__)))) |
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.
Why do we need this? Won't this be adding lib/
to the class path?
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.
Is lib/ already added? Why?
@@ -11,6 +11,9 @@ | |||
import logging | |||
from subprocess import call | |||
from subprocess import check_call | |||
from os import path | |||
|
|||
sys.path.append(path.dirname(path.dirname(path.abspath(__file__)))) |
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.
Why do we need this? Won't this be adding lib/
to the class path?
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.
@@ -14,19 +14,74 @@ | |||
from subprocess import call | |||
from subprocess import check_call | |||
|
|||
from hardware_services.network_service import NetworkService | |||
sys.path.append(os.path.join(os.path.dirname(__file__), "..", "util")) |
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.
Do we need this? If we remove this line ...
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.
Would you be so kind to briefly explain why we don't need it?
You can just give me a link. @portante
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 know we can do
from . import echo
from .. import formats
from ..filters import equalizer
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.
Is this what you want?
|
||
# added for EC528 HIL-QUADS integration project | ||
hil_url = 'http://127.0.0.1:5000' | ||
import hilapi |
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.
And change this to import lib.hardward_services.utils.hilapi
, where we add a init.py module to the utils
directory, won't we avoid appending this to the path?
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.
Most likely, I don't quiet know how Python sys.paths work.
Is the path to ......./quads/ added by default somehow?
Removed extra sys.path's from network drivers. Cosmetic changes to HilNetworkDriver.
This is HilNetworkDriver implementations. It uses fuctions from hilapi.py and from HilInventoryDriver. Since HilInventoryDriver the following functions relying on HilInventoryDriver are also not implemented: __read_current_state(self, argv) and __write_current_state(self, argv)