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 Set Weight Support #6033

Merged
merged 2 commits into from Aug 4, 2020
Merged

FRR Set Weight Support #6033

merged 2 commits into from Aug 4, 2020

Conversation

kylehoferamzn
Copy link
Contributor

No description provided.

@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented Jul 28, 2020

Codecov Report

Merging #6033 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #6033      +/-   ##
============================================
+ Coverage     72.26%   72.32%   +0.06%     
- Complexity    34367    34422      +55     
============================================
  Files          2810     2816       +6     
  Lines        140953   141344     +391     
  Branches      16938    16981      +43     
============================================
+ Hits         101856   102225     +369     
+ Misses        31144    31096      -48     
- Partials       7953     8023      +70     
Impacted Files Coverage Δ Complexity Δ
...ar/cumulus_frr/CumulusFrrConfigurationBuilder.java 74.18% <100.00%> (+0.13%) 144.00 <1.00> (+1.00)
.../batfish/representation/cumulus/RouteMapEntry.java 98.27% <100.00%> (+0.09%) 36.00 <2.00> (+2.00)
...fish/representation/cumulus/RouteMapSetWeight.java 100.00% <100.00%> (ø) 3.00 <3.00> (?)
.../src/main/java/org/batfish/datamodel/FibEntry.java 83.33% <0.00%> (-12.13%) 12.00% <0.00%> (ø%)
...ain/java/org/batfish/datamodel/EvpnType5Route.java 76.34% <0.00%> (-10.62%) 13.00% <0.00%> (-16.00%)
...ain/java/org/batfish/datamodel/EvpnType2Route.java 75.45% <0.00%> (-8.95%) 15.00% <0.00%> (-16.00%)
...ain/java/org/batfish/datamodel/EvpnType3Route.java 77.45% <0.00%> (-8.69%) 17.00% <0.00%> (-14.00%)
.../org/batfish/dataplane/rib/RouteAdvertisement.java 84.78% <0.00%> (-8.08%) 16.00% <0.00%> (+1.00%) ⬇️
...in/java/org/batfish/datamodel/BgpPeerConfigId.java 82.81% <0.00%> (-5.72%) 22.00% <0.00%> (ø%)
...ain/java/org/batfish/datamodel/GeneratedRoute.java 90.00% <0.00%> (-4.96%) 26.00% <0.00%> (-7.00%)
... and 64 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 8 of 8 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kylehoferamzn)


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

public class RouteMapSetWeight implements RouteMapSet {

  private int _weight;

just double checking: This can only be literal integer, or can it be an expression (like metric) where you're allowed to say +5 or -10, etc.


projects/batfish/src/test/java/org/batfish/grammar/cumulus_frr/CumulusFrrGrammarTest.java, line 1727 at r1 (raw file):

     Features tested:
     1) Set comm
     2) Match comm + set tag + set weight

this is too much stuff in a single test

why can't the advertiser unconditionally send a single route, and the listener unconditionally accept, but set the weight on import?
that way we are testing one thing: weight setting. And since there is only one route being exchanged to additional filtering is needed.

@kylehoferamzn
Copy link
Contributor Author

"just double checking: This can only be literal integer, or can it be an expression (like metric) where you're allowed to say +5 or -10, etc."

From my experience with the CLI you can only set a literal integer and doesn't allow any increment or expressions. (And now I know why you guys had IntExpr on Metric!)

"this is too much stuff in a single test"

As discussed, will rename this test and create a new, specific test.

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

@progwriter progwriter merged commit de23e1b into batfish:master Aug 4, 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