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 vrf extraction #3501

Merged
merged 11 commits into from
Mar 29, 2019

Conversation

arifogel
Copy link
Member

  • Harmonize naming restrictions and structure creation
    • bonds
    • interfaces
    • vrfs

@arifogel arifogel requested a review from sfraint March 28, 2019 21:55
@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented Mar 28, 2019

Codecov Report

Merging #3501 into master will decrease coverage by <.01%.
The diff coverage is 88.23%.

@@             Coverage Diff              @@
##             master    #3501      +/-   ##
============================================
- Coverage     73.27%   73.26%   -0.01%     
- Complexity    24100    24124      +24     
============================================
  Files          2091     2092       +1     
  Lines        100531   100639     +108     
  Branches      12008    12025      +17     
============================================
+ Hits          73666    73736      +70     
- Misses        21484    21512      +28     
- Partials       5381     5391      +10
Impacted Files Coverage Δ Complexity Δ
...h/representation/cumulus/CumulusStructureType.java 100% <100%> (ø) 3 <0> (ø) ⬇️
...n/java/org/batfish/representation/cumulus/Vrf.java 100% <100%> (ø) 5 <5> (?)
.../representation/cumulus/CumulusStructureUsage.java 100% <100%> (ø) 3 <0> (ø) ⬇️
...presentation/cumulus/CumulusNcluConfiguration.java 100% <100%> (ø) 13 <1> (+1) ⬆️
.../org/batfish/representation/cumulus/Interface.java 96.55% <100%> (+0.39%) 17 <2> (+2) ⬆️
.../cumulus_nclu/CumulusNcluConfigurationBuilder.java 93.49% <85.71%> (-4.05%) 109 <29> (+29)
...c/main/java/org/batfish/dataplane/rib/RibTree.java 78.72% <0%> (-10.64%) 23% <0%> (-1%)
.../src/main/java/org/batfish/datamodel/flow/Hop.java 53.33% <0%> (-6.67%) 5% <0%> (-1%)
...g/batfish/datamodel/acl/IpAccessListLineIndex.java 33.33% <0%> (-5.56%) 4% <0%> (-1%)
...col/src/main/java/org/batfish/role/InferRoles.java 90.62% <0%> (-1.57%) 68% <0%> (-2%)
... and 5 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 17 of 17 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


projects/batfish/src/test/java/org/batfish/grammar/cumulus_nclu/CumulusNcluGrammarTest.java, line 303 at r1 (raw file):

    assertThat(ans, hasNumReferrers(filename, CumulusStructureType.VRF, "vrf1", 1));
    assertThat(ans, hasNumReferrers(filename, CumulusStructureType.VRF, "vrf2", 1));
    assertThat(ans, hasNumReferrers(filename, CumulusStructureType.VRF, "vrf3", 1));

could optionally add another vrf ref for one of these, so they aren't all 1

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: :shipit: complete! all files reviewed, all discussions resolved


projects/batfish/src/test/java/org/batfish/grammar/cumulus_nclu/CumulusNcluGrammarTest.java, line 303 at r1 (raw file):

Previously, sfraint (Spencer Fraint) wrote…
    assertThat(ans, hasNumReferrers(filename, CumulusStructureType.VRF, "vrf1", 1));
    assertThat(ans, hasNumReferrers(filename, CumulusStructureType.VRF, "vrf2", 1));
    assertThat(ans, hasNumReferrers(filename, CumulusStructureType.VRF, "vrf3", 1));

could optionally add another vrf ref for one of these, so they aren't all 1

better now?

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 3 of 4 files at r2, 4 of 4 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @arifogel)


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

initInterfacesIfAbsent

Do any calls to this need to pass null for usage?

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: 16 of 18 files reviewed, 1 unresolved discussion (waiting on @sfraint)


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

Previously, sfraint (Spencer Fraint) wrote…
initInterfacesIfAbsent

Do any calls to this need to pass null for usage?

Yes.
enterA_interface does

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 2 of 2 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@arifogel arifogel merged commit 04a961f into batfish:master Mar 29, 2019
@arifogel arifogel deleted the ari-cumulus-extraction-vrf branch March 29, 2019 17:43
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