Support hierarchical Juniper 'replace:' statement #3688

arifogel · 2019-04-23T22:29:07Z

No description provided.

batfish-bot · 2019-04-23T22:29:12Z

This change is

codecov · 2019-04-23T22:41:14Z

Codecov Report

Merging #3688 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #3688      +/-   ##
============================================
+ Coverage     73.79%    73.8%   +<.01%     
- Complexity    24822    24838      +16     
============================================
  Files          2118     2119       +1     
  Lines        101651   101705      +54     
  Branches      12006    12013       +7     
============================================
+ Hits          75014    75060      +46     
- Misses        21279    21280       +1     
- Partials       5358     5365       +7

Impacted Files	Coverage Δ	Complexity Δ
.../org/batfish/grammar/juniper/SetStatementTree.java	`100% <100%> (ø)`	`9 <9> (?)`
.../org/batfish/grammar/juniper/JuniperFlattener.java	`97.4% <100%> (+0.52%)`	`26 <3> (+3)`	⬆️
...g/batfish/datamodel/acl/IpAccessListLineIndex.java	`33.33% <0%> (-5.56%)`	`4% <0%> (-1%)`
...src/main/java/org/batfish/coordinator/PoolMgr.java	`59.55% <0%> (-4.5%)`	`13% <0%> (-2%)`
...ain/java/org/batfish/coordinator/WorkQueueMgr.java	`70.84% <0%> (-0.59%)`	`90% <0%> (-1%)`
...src/main/java/org/batfish/coordinator/WorkMgr.java	`69.49% <0%> (-0.28%)`	`277% <0%> (-1%)`
...org/batfish/dataplane/ibdp/OspfRoutingProcess.java	`89% <0%> (-0.16%)`	`161% <0%> (+3%)`
...tfish/representation/cisco/CiscoConfiguration.java	`83.8% <0%> (-0.13%)`	`530% <0%> (+2%)`
...batfish/representation/cisco/CiscoConversions.java	`90.41% <0%> (+0.06%)`	`147% <0%> (+1%)`	⬆️
... and 3 more

dhalperi

Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @arifogel)

projects/batfish/src/main/java/org/batfish/grammar/juniper/JuniperFlattener.java, line 65 at r1 (raw file):

            ctx.words.stream().map(ParserRuleContext::getText).collect(Collectors.joining(" "));
        if (ctx.REPLACE() != null) {
          _currentNode.getChildren().remove(nodeKey);

Add comments to this class? At least the new stuff.

it looks like this is saying, "the start of this statement is replace:, so we need to delete any existing part of the tree that matches here."

projects/batfish/src/main/java/org/batfish/grammar/juniper/JuniperFlattener.java, line 65 at r1 (raw file):

            ctx.words.stream().map(ParserRuleContext::getText).collect(Collectors.joining(" "));
        if (ctx.REPLACE() != null) {
          _currentNode.getChildren().remove(nodeKey);

why is it okay to remove children but not recreate the builder containing the set statements?

projects/batfish/src/main/java/org/batfish/grammar/juniper/LineTree.java, line 1 at r1 (raw file):

package org.batfish.grammar.juniper;

missing unit tests for this class

projects/batfish/src/main/java/org/batfish/grammar/juniper/LineTree.java, line 2 at r1 (raw file):

package org.batfish.grammar.juniper;

missing documentation for this class. I can't tell how it's supposed to function supposed to use it. Why are some fields mutable and some fields immutable builders? What are the main use patterns?

projects/batfish/src/test/resources/org/batfish/grammar/juniper/testconfigs/replace_filter, line 18 at r1 (raw file):

                }
            }
        }

add another filter, which should not be replaced.

arifogel

Reviewable status: 5 of 10 files reviewed, 3 unresolved discussions (waiting on @dhalperi)

projects/batfish/src/main/java/org/batfish/grammar/juniper/JuniperFlattener.java, line 65 at r1 (raw file):