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: route-map match source-protocol #6618

Merged
merged 2 commits into from Feb 10, 2021

Conversation

dhalperi
Copy link
Member

No description provided.

@batfish-bot
Copy link

This change is Reviewable

Copy link
Member

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

Reviewed 10 of 10 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dhalperi)


projects/batfish/src/test/java/org/batfish/representation/cumulus/RouteMapMatchSourceProtocolTest.java, line 30 at r1 (raw file):

import org.junit.Test;

public class RouteMapMatchSourceProtocolTest {

nitty nit: final and javadoc


projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/IsisRoute.java, line 90 at r1 (raw file):

  }

  /** Return a route builder with pre-filled mandatory values. To be used in tests only */

nit: why not put it in test lib? same question for ospf

@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #6618 (f739163) into master (4f1b7e9) will decrease coverage by 0.00%.
The diff coverage is 93.42%.

@@             Coverage Diff              @@
##             master    #6618      +/-   ##
============================================
- Coverage     73.42%   73.42%   -0.01%     
- Complexity    35861    35875      +14     
============================================
  Files          2845     2846       +1     
  Lines        144714   144773      +59     
  Branches      17515    17524       +9     
============================================
+ Hits         106254   106293      +39     
- Misses        30028    30038      +10     
- Partials       8432     8442      +10     
Impacted Files Coverage Δ Complexity Δ
...sentation/cumulus/RouteMapMatchSourceProtocol.java 87.50% <87.50%> (ø) 10.00 <10.00> (?)
...ar/cumulus_frr/CumulusFrrConfigurationBuilder.java 77.80% <94.59%> (+0.34%) 201.00 <11.00> (+8.00)
...src/main/java/org/batfish/datamodel/IsisRoute.java 86.86% <100.00%> (+0.84%) 21.00 <1.00> (+1.00)
.../org/batfish/datamodel/OspfExternalType1Route.java 40.00% <100.00%> (+18.94%) 4.00 <1.00> (+1.00)
.../batfish/representation/cumulus/RouteMapEntry.java 98.43% <100.00%> (+0.07%) 40.00 <2.00> (+2.00)
...g/batfish/datamodel/flow/IncomingSessionScope.java 84.61% <0.00%> (-15.39%) 7.00% <0.00%> (-1.00%)
.../org/batfish/dataplane/rib/RouteAdvertisement.java 83.67% <0.00%> (-4.09%) 18.00% <0.00%> (-1.00%)
...col/src/main/java/org/batfish/role/InferRoles.java 89.54% <0.00%> (-1.37%) 50.00% <0.00%> (-1.00%)
...src/main/java/org/batfish/coordinator/WorkMgr.java 75.42% <0.00%> (-0.39%) 245.00% <0.00%> (-2.00%)
...a/org/batfish/representation/aws/LoadBalancer.java 82.18% <0.00%> (-0.32%) 70.00% <0.00%> (-1.00%)
... and 4 more

Copy link
Member

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dhalperi)

Copy link
Member Author

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

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


projects/batfish/src/test/java/org/batfish/representation/cumulus/RouteMapMatchSourceProtocolTest.java, line 30 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

nitty nit: final and javadoc

for test classes? meh.


projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/IsisRoute.java, line 90 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

nit: why not put it in test lib? same question for ospf

Following pattern for all existing routes.

Copy link
Member Author

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


projects/batfish/src/test/java/org/batfish/representation/cumulus/RouteMapMatchSourceProtocolTest.java, line 30 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

for test classes? meh.

(I did fix for the original class, which is what I had assumed you were asking about :p)

Copy link
Member

@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/representation/cumulus/RouteMapMatchSourceProtocolTest.java, line 30 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

(I did fix for the original class, which is what I had assumed you were asking about :p)

I wasn't, but good think you noticed.

Copy link
Member Author

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


projects/batfish/src/test/java/org/batfish/representation/cumulus/RouteMapMatchSourceProtocolTest.java, line 30 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

I wasn't, but good think you noticed.

Ha, yeah. Thanks for making me look twice :)

@dhalperi dhalperi merged commit 65defd0 into batfish:master Feb 10, 2021
@dhalperi dhalperi deleted the frr-match-source-protocol branch February 10, 2021 23:37
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