cumulus frr ospf #4821
cumulus frr ospf #4821
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @anothermattbrown and @yifeiyuan)
projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_ospf.g4, line 17 at r1 (raw file):
; ro_null
Why not just properly name the rule? Let's avoid null rules unless we absolutely have to.
projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusNcluConfiguration.java, line 709 at r1 (raw file):
if (vrf == null) { return;
warn
projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusNcluConfiguration.java, line 713 at r1 (raw file):
org.batfish.datamodel.ospf.OspfProcess ospfProcess = toOspfProcess(ospfVrf); if (ospfProcess != null) {
warn
projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusNcluConfiguration.java, line 1140 at r1 (raw file):
org.batfish.datamodel.ospf.OspfProcess toOspfProcess(OspfVrf ospfVrf) { Ip routerId = ospfVrf.getRouterId(); if (routerId == null) {
factor out into a method and test.
Leave pointers to docs about how this works.
There was a problem hiding this comment.
Reviewed 8 of 9 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @progwriter and @yifeiyuan)
a discussion (no related file):
imo this should be 2 prs (or at least 2 separate commits): one for parsing/extraction, one for conversion. not a blocker, but a request for the future
projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_ospf.g4, line 17 at r1 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
Why not just properly name the rule? Let's avoid
nullrules unless we absolutely have to.
it's not clear what you want -- what is your concrete suggestion?
projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusNcluConfiguration.java, line 1138 at r1 (raw file):
@VisibleForTesting @Nullable org.batfish.datamodel.ospf.OspfProcess toOspfProcess(OspfVrf ospfVrf) {
Why is this class the right place for this method? It seems more natural to me to put it in OspfVrf, and pass in relevant data from the config
There was a problem hiding this comment.
Reviewed 1 of 9 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @progwriter and @yifeiyuan)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @anothermattbrown and @yifeiyuan)
projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_ospf.g4, line 17 at r1 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
it's not clear what you want -- what is your concrete suggestion?
I want a fleshed out rule following LL1 grammar. here:
s_router_ospf
:
ROUTER OSPF NEWLINE
(
ro_log_adjacency_changes
)*
;
ro_log_adjacency_changes
:
LOG_ADJACENCY_CHANGES DETAIL? NEWLINE
;
http://docs.frrouting.org/en/latest/ospfd.html#clicmd-log-adjacency-changes[detail]
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @progwriter and @yifeiyuan)
projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_ospf.g4, line 17 at r1 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
I want a fleshed out rule following LL1 grammar. here:
s_router_ospf : ROUTER OSPF NEWLINE ( ro_log_adjacency_changes )* ; ro_log_adjacency_changes : LOG_ADJACENCY_CHANGES DETAIL? NEWLINE ;http://docs.frrouting.org/en/latest/ospfd.html#clicmd-log-adjacency-changes[detail]
cool
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @progwriter and @yifeiyuan)
a discussion (no related file):
blocker
This is interesting.. I did have multiple commits. not sure why there is only one commit now.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @anothermattbrown, @progwriter, and @yifeiyuan)
projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_ospf.g4, line 17 at r1 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
cool
done
projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusNcluConfiguration.java, line 709 at r1 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
warn
done
projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusNcluConfiguration.java, line 713 at r1 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
warn
ospfProcess should not be null. so removed the test
projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusNcluConfiguration.java, line 1138 at r1 (raw file):
natural to me to put it in OspfVrf, and pass in relevant data from the config
move this function toospfVrfand then callospfVrf.toOspfProcess(..)?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @anothermattbrown and @progwriter)
projects/batfish/src/main/java/org/batfish/representation/cumulus/CumulusNcluConfiguration.java, line 1140 at r1 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
factor out into a method and test.
Leave pointers to docs about how this works.
done
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status: 8 of 9 files reviewed, 1 unresolved discussion (waiting on @anothermattbrown and @progwriter)
Codecov Report
@@ Coverage Diff @@
## master #4821 +/- ##
============================================
- Coverage 75.5% 75.49% -0.01%
- Complexity 28724 28743 +19
============================================
Files 2292 2294 +2
Lines 112836 112886 +50
Branches 13541 13544 +3
============================================
+ Hits 85196 85228 +32
- Misses 21131 21145 +14
- Partials 6509 6513 +4
|
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
parse, conversion to vs, conversion to vi for the following commands: