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 max med administrative #6086

Merged
merged 5 commits into from Aug 18, 2020

Conversation

kylehoferamzn
Copy link
Contributor

No description provided.

@batfish-bot
Copy link

This change is Reviewable

Copy link
Contributor

@progwriter progwriter 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)

a discussion (no related file):
please add conversion test as well



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

;

max_med_administrative_value

This isn't used, I think you can remove.


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

  public void exitSbb_max_med_administrative(Sbb_max_med_administrativeContext ctx) {
    if (ctx.med != null) _currentBgpVrf.setMaxMedAdministrative(Long.parseLong(ctx.med.getText()));
    else _currentBgpVrf.setMaxMedAdministrative(4294967294L);

what's the magic value? please pull out in to named constant.


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

  private static List<Statement> getAcceptStatements(BgpNeighbor neighbor, BgpVrf bgpVrf) {
    ArrayList<Statement> acceptStatements = new ArrayList<>();

prefer ImmutableList.builder() to which you can add conditionally, then just return builder.build()

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.

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

a discussion (no related file):

Previously, progwriter (Victor Heorhiadi) wrote…

please add conversion test as well

Were you looking for something more than testGetSetMaxMedMetric?


Copy link
Contributor

@progwriter progwriter left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kylehoferamzn)

a discussion (no related file):

Previously, kylehoferamzn wrote…

Were you looking for something more than testGetSetMaxMedMetric?

thats a good start, but what's missing is a check that the statement actually makes it to peer's export policy


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.

Reviewable status: 3 of 7 files reviewed, 1 unresolved discussion (waiting on @progwriter)

a discussion (no related file):

Previously, progwriter (Victor Heorhiadi) wrote…

thats a good start, but what's missing is a check that the statement actually makes it to peer's export policy

Good call, added that in.


Copy link
Contributor

@progwriter progwriter left a comment

Choose a reason for hiding this comment

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

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

@codecov
Copy link

codecov bot commented Aug 18, 2020

Codecov Report

Merging #6086 into master will increase coverage by 0.15%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master    #6086      +/-   ##
============================================
+ Coverage     72.51%   72.67%   +0.15%     
- Complexity    34575    34625      +50     
============================================
  Files          2816     2815       -1     
  Lines        141407   141276     -131     
  Branches      16989    16965      -24     
============================================
+ Hits         102548   102676     +128     
+ Misses        30946    30436     -510     
- Partials       7913     8164     +251     
Impacted Files Coverage Δ Complexity Δ
...ar/cumulus_frr/CumulusFrrConfigurationBuilder.java 74.49% <80.00%> (+0.30%) 148.00 <2.00> (+4.00)
...ish/representation/cumulus/CumulusConversions.java 90.19% <80.00%> (+0.10%) 190.00 <4.00> (+3.00)
...ava/org/batfish/representation/cumulus/BgpVrf.java 97.67% <100.00%> (+0.17%) 25.00 <2.00> (+2.00)
...er/question/searchroutepolicies/ResultOrUnsat.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-4.00%)
...va/org/batfish/minesweeper/bdd/PolicyQuotient.java 0.00% <0.00%> (-92.16%) 0.00% <0.00%> (-20.00%)
.../main/java/org/batfish/main/CoordinatorClient.java 43.75% <0.00%> (-11.25%) 2.00% <0.00%> (ø%)
...ain/java/org/batfish/representation/cisco/Vrf.java 51.35% <0.00%> (-5.41%) 11.00% <0.00%> (-1.00%)
...src/main/java/org/batfish/coordinator/PoolMgr.java 59.25% <0.00%> (-4.79%) 15.00% <0.00%> (-1.00%)
.../org/batfish/dataplane/rib/RouteAdvertisement.java 84.78% <0.00%> (-4.35%) 16.00% <0.00%> (-1.00%)
...c/main/java/org/batfish/dataplane/rib/RibTree.java 89.58% <0.00%> (-4.17%) 27.00% <0.00%> (-1.00%)
... and 53 more

Copy link
Contributor

@progwriter progwriter 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

@progwriter progwriter merged commit 31b094e into batfish:master Aug 18, 2020
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