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

Adding support for ClusterList and Originator Id to VI Question #5857

Merged
merged 11 commits into from Jun 6, 2020

Conversation

kylehoferamzn
Copy link
Contributor

@kylehoferamzn kylehoferamzn commented Jun 3, 2020

I realize Im missing tests here, I'll add those in but before I do so I wanted to know if the general approach is what you all want

@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 2 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kylehoferamzn)


projects/question/src/main/java/org/batfish/question/routes/RoutesAnswerer.java, line 293 at r1 (raw file):

                    Boolean.FALSE,
                    Boolean.TRUE))
            .add(

small nit: I'd flip the order of these since originator is mandatory and is generally more useful (IMO)


projects/question/src/main/java/org/batfish/question/routes/RoutesAnswererUtil.java, line 326 at r1 (raw file):

        .put(
            COL_CLUSTER_LIST,
            bgpv4Route.getClusterList().size() == 0 ? null : bgpv4Route.getClusterList())

I'd do isEmpty()

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

@codecov
Copy link

codecov bot commented Jun 6, 2020

Codecov Report

Merging #5857 into master will increase coverage by 0.10%.
The diff coverage is 81.52%.

@@             Coverage Diff              @@
##             master    #5857      +/-   ##
============================================
+ Coverage     71.90%   72.01%   +0.10%     
- Complexity    33875    33964      +89     
============================================
  Files          2791     2803      +12     
  Lines        139545   139767     +222     
  Branches      16754    16784      +30     
============================================
+ Hits         100346   100657     +311     
+ Misses        31317    31227      -90     
- Partials       7882     7883       +1     
Impacted Files Coverage Δ Complexity Δ
...col/src/main/java/org/batfish/common/BfConsts.java 83.33% <ø> (ø) 1.00 <0.00> (ø)
...src/main/java/org/batfish/datamodel/Interface.java 84.14% <ø> (-0.32%) 216.00 <0.00> (-2.00)
...atfish/src/main/java/org/batfish/main/Batfish.java 74.92% <28.57%> (+0.06%) 275.00 <2.00> (-1.00) ⬆️
.../main/java/org/batfish/main/CoordinatorClient.java 55.00% <55.00%> (ø) 2.00 <2.00> (?)
...rg/batfish/question/routes/RoutesAnswererUtil.java 91.87% <66.66%> (-0.39%) 111.00 <0.00> (ø)
.../java/org/batfish/job/ConvertConfigurationJob.java 78.63% <69.23%> (+8.23%) 21.00 <2.00> (-2.00) ⬆️
...atamodel/questions/InterfacePropertySpecifier.java 93.42% <75.00%> (-2.17%) 9.00 <3.00> (+3.00) ⬇️
...src/main/java/org/batfish/coordinator/WorkMgr.java 77.19% <83.13%> (+4.43%) 275.00 <108.00> (-5.00) ⬆️
...n/java/org/batfish/coordinator/WorkMgrService.java 30.30% <83.33%> (+1.04%) 30.00 <1.00> (ø)
...rg/batfish/representation/cumulus/StaticRoute.java 85.36% <87.50%> (-1.13%) 17.00 <3.00> (+2.00) ⬇️
... and 62 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 r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@progwriter progwriter merged commit 5067a2a into batfish:master Jun 6, 2020
kylehoferamzn added a commit to kylehoferamzn/batfish that referenced this pull request Jun 22, 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