CCI-MOC / hil

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

Require setting a native vlan before adding any trunk vlans on the brocade switch (and correctly parse range of vlans) #958

Closed naved001 closed 6 years ago

naved001 commented 6 years ago

It's a work in progress because tests/deployment/vlan_networks.py is failing. Gotta dig into that and push a fix asap.

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 1676


Changes Missing Coverage Covered Lines Changed/Added Lines %
hil/ext/switches/dellnos9.py 0 4 0.0%
hil/ext/switches/brocade.py 0 9 0.0%
hil/ext/switches/common.py 0 20 0.0%
<!-- Total: 0 33 0.0% -->
Files with Coverage Reduction New Missed Lines %
hil/ext/switches/brocade.py 1 0.0%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 1675: -0.2%
Covered Lines: 2097
Relevant Lines: 3534

💛 - Coveralls
naved001 commented 6 years ago

figured out why the deployment tests were failing on the broacade

<hil.model.Port object at 0x7fd778e0e090>: [('vlan/native', '1514'), ('vlan/1511-1512', '1511-1512')]}

Now that I am attaching 3 networks to a port, the output is a range, which we don't parse correctly. Will push a fix soon.

naved001 commented 6 years ago

okey dokey. The deployment tests now pass for brocade switch.

(.venv) naved:~/brocade-native-vlan/hil$ py.test tests/deployment/*networks.py
========================================= test session starts =========================================
platform linux2 -- Python 2.7.12, pytest-3.4.1, py-1.5.2, pluggy-0.6.0
rootdir: /home/naved/brocade-native-vlan/hil, inifile: setup.cfg
plugins: cov-2.5.1, forked-0.2, xdist-1.22.2
collected 2 items                                                                                     

tests/deployment/native_networks.py .                                                           [  0%]
tests/deployment/vlan_networks.py .

====================================== 2 passed in 53.40 seconds ======================================
naved001 commented 6 years ago

coveralls doesn't seem to be happy, but I am already testing the stuff I need to :/

zenhack commented 6 years ago

LGTM, assuming travis patches I'll merge.

naved001 commented 6 years ago

@zenhack @xuhang57 are we ignoring coveralls here too, or is there anything that I can do to make it happy.

xuhang57 commented 6 years ago

Based on what the coverage tests are indicating, I would safe we should ignore the coveralls. We should merge.

naved001 commented 6 years ago

I am gonna go ahead and merge this.