search

CCI-MOC / hil

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

Refactor dellnos9 and brocade switch drivers #1000

Closed naved001 closed 6 years ago

naved001 commented 6 years ago

Just some things that I think could be changed. There's info in the commit message.

Deployment tests passed for both switches.

Modify port looks quite similar, but there are some differences. I could make those changes and push modify port to the super class if we are going to have switches that are unlike the console switches. Other than that, we could put it in switches.common and then add the method from there to the switch class.

I can also remove saving-to-switch from this PR which would help simplify modify port.

naved001 commented 6 years ago

I am thinking I might as well make a superclass for non-console like drivers.

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 1894

Changes Missing Coverage Covered Lines Changed/Added Lines %
hil/ext/switches/dellnos9.py 0 4 0.0%
hil/ext/switches/brocade.py 0 4 0.0%
hil/ext/switches/_vlan_http.py 49 64 76.56%
<!-- Total: 49 72 68.06% -->
Totals Coverage Status
Change from base Build 1877: 1.4%
Covered Lines: 2119
Relevant Lines: 3841
💛 - Coveralls
naved001 commented 6 years ago

@zenhack: made a bunch of changes this time; moves the repeated stuff in a new module for which I couldn't come up with an appropriate name yet. With the new module, @SahilTikale wouldn't have to rewrite the same bunch of stuff in his 2 PRs.

naved001 commented 6 years ago

@zenhack: Should I do #973 and/or #970 while am at it, or in separate PRs? Besides that, is there anything else that you think can be factored out?

zenhack commented 6 years ago

Let's keep this one self contained; those issues can be addressed in separate prs.

naved001 commented 6 years ago

To make modify_port and revert_port generic, I removed the the calls to save_running_config in dellnos9.

naved001 commented 6 years ago

Missing docstrings

Done. Wondering why didn't pylint complain about it given many of these methods aren't declared in the SwitchSession() class in hil/model.py

zenhack commented 6 years ago

LGTM

naved001 commented 6 years ago

@zenhack I have one doubt here; If I wanted to use this new module with the openVswitch driver it will contain the _make_request method which it doesnt need. Is that okay in theory for that class to have that attribute?

Edit: I am worried that I might have made this too specific. What do you think?

zenhack commented 6 years ago

It shouldn't break anything. That said it may be a bit cleaner to have it be a stand-alone function (and make auth a parameter, rather than a separate property). I'm fine either way.

naved001 commented 6 years ago

Just rebased this and fixed conflicts.

zenhack commented 6 years ago

Cool. Can someone do a second review on this?

naved001 commented 6 years ago

@izhmash was a bit busy this week with other things. He'll be back tomorrow though.

On Thu, May 3, 2018, 8:59 PM Ian Denhardt notifications@github.com wrote:

Cool. Can someone do a second review on this?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/CCI-MOC/hil/pull/1000#issuecomment-386480451, or mute the thread https://github.com/notifications/unsubscribe-auth/ATLmqDl5DT8EOvfl1T9mJXSm_7-HVb6cks5tu6gCgaJpZM4Tcl_v .