Skip to content
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

Cumulus NCLU interface extraction #3493

Merged
merged 3 commits into from
Mar 28, 2019

Conversation

arifogel
Copy link
Member

No description provided.

@arifogel arifogel requested a review from sfraint March 27, 2019 23:58
@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented Mar 28, 2019

Codecov Report

Merging #3493 into master will increase coverage by <.01%.
The diff coverage is 98.34%.

@@             Coverage Diff              @@
##             master    #3493      +/-   ##
============================================
+ Coverage     73.23%   73.23%   +<.01%     
- Complexity    24024    24063      +39     
============================================
  Files          2089     2090       +1     
  Lines        100361   100494     +133     
  Branches      11989    12005      +16     
============================================
+ Hits          73498    73600     +102     
- Misses        21481    21505      +24     
- Partials       5382     5389       +7
Impacted Files Coverage Δ Complexity Δ
...h/representation/cumulus/CumulusInterfaceType.java 100% <100%> (ø) 1 <1> (?)
.../representation/cumulus/CumulusStructureUsage.java 100% <100%> (ø) 3 <0> (ø) ⬇️
...presentation/cumulus/CumulusNcluConfiguration.java 100% <100%> (ø) 11 <0> (ø) ⬇️
.../org/batfish/representation/cumulus/Interface.java 96.15% <95.83%> (+21.15%) 15 <14> (+14) ⬆️
.../cumulus_nclu/CumulusNcluConfigurationBuilder.java 97.46% <98.87%> (+4.24%) 77 <30> (+28) ⬆️
...c/main/java/org/batfish/dataplane/rib/RibTree.java 78.72% <0%> (-10.64%) 23% <0%> (-1%)
...ava/org/batfish/bddreachability/transition/Or.java 40.9% <0%> (-9.1%) 5% <0%> (+2%)
.../src/main/java/org/batfish/datamodel/flow/Hop.java 53.33% <0%> (-6.67%) 5% <0%> (-1%)
...src/main/java/org/batfish/coordinator/PoolMgr.java 59.55% <0%> (-5.62%) 13% <0%> (-3%)
...in/java/org/batfish/dataplane/rib/AbstractRib.java 95.52% <0%> (-1.5%) 33% <0%> (-1%)
... and 12 more

Copy link
Member

@sfraint sfraint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 15 files at r1, 6 of 6 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @arifogel)


projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 386 at r2 (raw file):

_currentInterfaces.forEach(

Maybe add a warning in the places where this setting cannot apply to multiple interfaces? specifically in the places where the device would complain on commit

Copy link
Member Author

@arifogel arifogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sfraint)


projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 386 at r2 (raw file):

Previously, sfraint (Spencer Fraint) wrote…
_currentInterfaces.forEach(

Maybe add a warning in the places where this setting cannot apply to multiple interfaces? specifically in the places where the device would complain on commit

Believe it or not NCLU does not complain when using multiple interfaces for any of these commands.

@arifogel arifogel merged commit 6aaff4a into batfish:master Mar 28, 2019
@arifogel arifogel deleted the ari-cumulus-extraction-interface branch March 28, 2019 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants