Cumulus NCLU configuration parsing skeleton #3453
Conversation
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #3453 +/- ##
============================================
- Coverage 73.13% 73.11% -0.03%
- Complexity 23787 23864 +77
============================================
Files 2062 2077 +15
Lines 99540 99905 +365
Branches 11935 11959 +24
============================================
+ Hits 72800 73042 +242
- Misses 21416 21503 +87
- Partials 5324 5360 +36
|
arifogel
force-pushed
the
ari-cumulus-nclu-skeleton
branch
from
c202d2b
to
c02c9db
Compare
There was a problem hiding this comment.
Reviewed 16 of 16 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @arifogel, @corinaminer, and @sfraint)
a discussion (no related file):
Javadoc everything not overridden.
projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 16 at r1 (raw file):
import org.batfish.vendor.StructureType; @ParametersAreNonnullByDefault
put this at the package-info.java module level rather than individual files?
projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 20 at r1 (raw file):
length() == 0
isEmpty()?
projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 26 at r1 (raw file):
return text; } else if (text.charAt(text.length() - 1) != '"') { throw new BatfishException("Improperly-quoted string");
include string in error?
projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 45 at r1 (raw file):
private String convErrorMessage(Class<?> type, ParserRuleContext ctx) { return String.format("Could not convert to %s: %s", type.getSimpleName(), getFullText(ctx));
inline
projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 56 at r1 (raw file):
/** Mark the specified structure as defined on each line in the supplied context */ @SuppressWarnings("unused") private void defineStructure(StructureType type, String name, RuleContext ctx) {
I don't understand this one. Is this a pre-post-processing-group thing from juniper? Does it apply here?
projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 73 at r1 (raw file):
} public CumulusNcluConfiguration getConfiguration() {
tests/parsing-tests/networks/unit-tests/configs/cumulus_nclu, line 76 at r1 (raw file):
# There are some configuration commands that are not yet supported by nclu. # ======================================================================== sudo sh -c "printf 'username cumulus nopassword\n' >> /etc/frr/frr.conf"
these lines are not lexed/parsed correctly - I think it has to do with >>, plus escaped inner contents? If that's intended for now, okay.
" IP_PREFIX:'0.0.0.0/0'",
" WORD:'192.0.2.1\\n''",
" WORD:'>>'",
" WORD:'/etc/frr/frr.conf\"'",
" NEWLINE:'\\n\\n')))",
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @arifogel, @corinaminer, @dhalperi, and @sfraint)
projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 16 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
put this at the
package-info.javamodule level rather than individual files?
What about the generated Java files in this package? I don't intend it to apply to those.
projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 20 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
length() == 0
isEmpty()?
Removing until needed.
projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 26 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
include string in error?
N/A
projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 45 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
inline
done
projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 56 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
I don't understand this one. Is this a pre-post-processing-group thing from juniper? Does it apply here?
lazy copypaste. removed as unnecessary.
projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 73 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
done
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @arifogel, @corinaminer, and @sfraint)
projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 16 at r1 (raw file):
Previously, arifogel (Ari Fogel) wrote…
What about the generated Java files in this package? I don't intend it to apply to those.
Are you sure? The context objects in enter/exit rules are assumed to be Nonnull, for instance. But:
You can always either:
- override and add nullable for functions you need to call with null values, if such exist.
- use a different package (should probably do this in the long term, actually).
There was a problem hiding this comment.
Reviewable status: 12 of 16 files reviewed, 3 unresolved discussions (waiting on @arifogel, @corinaminer, @dhalperi, and @sfraint)
tests/parsing-tests/networks/unit-tests/configs/cumulus_nclu, line 76 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
these lines are not lexed/parsed correctly - I think it has to do with
>>, plus escaped inner contents? If that's intended for now, okay." IP_PREFIX:'0.0.0.0/0'", " WORD:'192.0.2.1\\n''", " WORD:'>>'", " WORD:'/etc/frr/frr.conf\"'", " NEWLINE:'\\n\\n')))",
I'm not sure what you mean. To be clear, that is correct for the line below your comment.
This is the whole output for the `sudo lines:
" (statement",
" (s_null",
" SUDO:'sudo'",
" (null_rest_of_line",
" WORD:'sh'",
" WORD:'-c'",
" WORD:'\"printf'",
" WORD:''username'",
" WORD:'cumulus'",
" WORD:'nopassword\\n''",
" WORD:'>>'",
" WORD:'/etc/frr/frr.conf\"'",
" NEWLINE:'\\n')))",
" (statement",
" (s_null",
" SUDO:'sudo'",
" (null_rest_of_line",
" WORD:'sh'",
" WORD:'-c'",
" WORD:'\"printf'",
" WORD:''vrf'",
" WORD:'vrf1\\n'",
" WORD:'ip'",
" WORD:'route'",
" IP_PREFIX:'0.0.0.0/0'",
" WORD:'192.0.2.1\\n''",
" WORD:'>>'",
" WORD:'/etc/frr/frr.conf\"'",
" NEWLINE:'\\n\\n')))",
There was a problem hiding this comment.
Reviewable status: 12 of 16 files reviewed, 3 unresolved discussions (waiting on @arifogel, @corinaminer, @dhalperi, and @sfraint)
tests/parsing-tests/networks/unit-tests/configs/cumulus_nclu, line 76 at r1 (raw file):
Previously, arifogel (Ari Fogel) wrote…
I'm not sure what you mean. To be clear, that is correct for the line below your comment.
This is the whole output for the `sudo lines:" (statement", " (s_null", " SUDO:'sudo'", " (null_rest_of_line", " WORD:'sh'", " WORD:'-c'", " WORD:'\"printf'", " WORD:''username'", " WORD:'cumulus'", " WORD:'nopassword\\n''", " WORD:'>>'", " WORD:'/etc/frr/frr.conf\"'", " NEWLINE:'\\n')))", " (statement", " (s_null", " SUDO:'sudo'", " (null_rest_of_line", " WORD:'sh'", " WORD:'-c'", " WORD:'\"printf'", " WORD:''vrf'", " WORD:'vrf1\\n'", " WORD:'ip'", " WORD:'route'", " IP_PREFIX:'0.0.0.0/0'", " WORD:'192.0.2.1\\n''", " WORD:'>>'", " WORD:'/etc/frr/frr.conf\"'", " NEWLINE:'\\n\\n')))",
Oh if you are talking about the 192.0.2.1\n not being lexed as IP_PREFIX followed by NEWLINE, then yeah that is intended for now.
There was a problem hiding this comment.
Reviewable status: 12 of 16 files reviewed, 3 unresolved discussions (waiting on @arifogel, @corinaminer, @dhalperi, and @sfraint)
tests/parsing-tests/networks/unit-tests/configs/cumulus_nclu, line 76 at r1 (raw file):
Previously, arifogel (Ari Fogel) wrote…
Oh if you are talking about the
192.0.2.1\nnot being lexed asIP_PREFIXfollowed byNEWLINE, then yeah that is intended for now.
I mean IP_PREFIX => IP_ADDRESS
There was a problem hiding this comment.
Reviewable status: 12 of 18 files reviewed, 2 unresolved discussions (waiting on @corinaminer, @dhalperi, and @sfraint)
a discussion (no related file):
Previously, dhalperi (Dan Halperin) wrote…
Javadoc everything not overridden.
done
projects/batfish/src/main/java/org/batfish/grammar/cumulus_nclu/CumulusNcluConfigurationBuilder.java, line 16 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
Are you sure? The context objects in enter/exit rules are assumed to be Nonnull, for instance. But:
You can always either:
- override and add nullable for functions you need to call with null values, if such exist.
- use a different package (should probably do this in the long term, actually).
k done
There was a problem hiding this comment.
Reviewed 6 of 6 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @corinaminer and @sfraint)
No description provided.