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

FRR: Adding max-metric router-lsa administrative support #6577

Merged
merged 1 commit into from Jan 20, 2021

Conversation

kylehoferamzn
Copy link
Contributor

Nothing special here. Should generally align with FRR documentation and is setup similar to Cisco's implementation.

@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented Jan 19, 2021

Codecov Report

Merging #6577 (49c2b01) into master (f31dab4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             master    #6577    +/-   ##
==========================================
  Coverage     73.39%   73.39%            
- Complexity    35719    35757    +38     
==========================================
  Files          2837     2838     +1     
  Lines        144177   144334   +157     
  Branches      17431    17466    +35     
==========================================
+ Hits         105819   105938   +119     
- Misses        29972    29987    +15     
- Partials       8386     8409    +23     
Impacted Files Coverage Δ Complexity Δ
...ar/cumulus_frr/CumulusFrrConfigurationBuilder.java 75.00% <100.00%> (+0.07%) 168.00 <1.00> (+1.00)
...ish/representation/cumulus/CumulusConversions.java 91.77% <100.00%> (+0.02%) 191.00 <0.00> (+1.00)
...rg/batfish/representation/cumulus/OspfProcess.java 100.00% <100.00%> (ø) 9.00 <2.00> (+2.00)
...tfish/representation/cisco/CiscoIosDynamicNat.java 72.41% <0.00%> (-12.21%) 52.00% <0.00%> (+6.00%) ⬇️
...n/java/org/batfish/datamodel/VrfLeakingConfig.java 90.90% <0.00%> (-3.54%) 16.00% <0.00%> (+2.00%) ⬇️
...atfish/representation/cisco/CiscoIosStaticNat.java 62.31% <0.00%> (-2.90%) 16.00% <0.00%> (+3.00%) ⬇️
...org/batfish/datamodel/flow/BidirectionalTrace.java 81.81% <0.00%> (-2.28%) 14.00% <0.00%> (-1.00%)
...main/java/org/batfish/datamodel/acl/AclTracer.java 60.37% <0.00%> (-1.26%) 43.00% <0.00%> (-1.00%)
...java/org/batfish/dataplane/ibdp/VirtualRouter.java 90.45% <0.00%> (-0.13%) 206.00% <0.00%> (ø%)
.../org/batfish/dataplane/ibdp/BgpRoutingProcess.java 83.35% <0.00%> (-0.06%) 145.00% <0.00%> (+1.00%) ⬇️
... and 13 more

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @kylehoferamzn)


projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_ospf.g4, line 18 at r1 (raw file):

ro_max_metric_router_lsa_administrative

move up to alphabetize?


projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4, line 741 at r1 (raw file):

Quoted 4 lines of code…
ROUTER_LSA
:
  'router-lsa'
;

move down 1 to alphabetize?


projects/batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java, line 692 at r1 (raw file):

_max_metric_router_lsa_administ

move up to alphabetize?


projects/batfish/src/main/java/org/batfish/representation/cumulus/OspfProcess.java, line 21 at r1 (raw file):

  private Map<CumulusRoutingProtocol, RedistributionPolicy> _redistributionPolicies;
  private boolean _maxMetricRouterLsa;

normally we make these @Nullable Boolean so we can distinguish between whether they've been configured (to true or false).

Then we can enforce the default explicitly during conversion rather than implicitly during class construction.

Using boolean value = firstNonNull(o.getNullableBoolean(), Boolean.FALSE).

Copy link
Contributor Author

@kylehoferamzn kylehoferamzn left a comment

Choose a reason for hiding this comment

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

Updated per comments

Reviewable status: 2 of 7 files reviewed, all discussions resolved (waiting on @dhalperi)

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dhalperi dhalperi merged commit f5664d8 into batfish:master Jan 20, 2021
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