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

Junos: Add missing vxlan grammar #8112

Merged
merged 22 commits into from Mar 10, 2022
Merged

Junos: Add missing vxlan grammar #8112

merged 22 commits into from Mar 10, 2022

Conversation

jeffkala
Copy link
Contributor

@jeffkala jeffkala commented Mar 4, 2022

Few supported evpn/vxlan grammars were missing, this should add them. This is my first time contributing, thanks for providing feedback.

@batfish-bot
Copy link

This change is Reviewable

@arifogel arifogel self-requested a review March 5, 2022 21:35
Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jeffkala)

a discussion (no related file):
I see that checkstyle is failing. To fix this, run tools/fix_java_format.sh


a discussion (no related file):
Thank you so much for your contribution! I'll be your shepherd to get this merged.

This is optional (really), but the review process will be smoother if you use reviewable to reply to my comments. Just click the reviewable logo at the top of the github conversation to get started. If you find it a hassle, replying on github is fine.
Either way, the full conversation will be available there.



projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniperParser.g4, line 227 at r1 (raw file):

vlt_vni_id
:
   VXLAN VNI id = dec

This isn't obvious to outsider contributors, but dec is deprecated project-wide.
Instead, please use a custom rule like the following:

vni_number
:
  // 4096-16777215 (or whatever the range is for Juniper if different)
  uint32

Then add the following in ConfigurationBuilder:

private static final IntegerSpace VNI_NUMBER_RANGE = IntegerSpace.of(new SubRange(4096, 16777215));

private static @Nonnull Optional<Integer> toInteger(ParserRuleContext messageCtx, Vni_numberContext ctx) {
  return toIntegerInSpace(messageCtx, ctx.uint32(), "vni", VNI_NUMBER_RANGE);
}

and handle the case where Optional.empty() is returned in the caller (exitVlt_vni_id in this case) with:

Optional<Integer> maybeVniNumber = toInteger(ctx, ctx.vni_number());
if (!maybeVniNumber.isPresent()) {
  // already warned
  return;
}
int vniNumber = maybeVniNumber.get();
...

projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java, line 2392 at r1 (raw file):

  @Override
  public void exitFo_vxlan_routing(Fo_vxlan_routingContext ctx) {
    warn(ctx, "Batfish does not enable two-level equal-cost multipath next hops.");

nit: usually we just do todo(ctx) for the standard unsupported feature message.

On that note, please add exit rules with todo(ctx) to cover all new grammar you added (just need one per top-level rule) that will eventually be supported. If something you added will never be supported , make the name for the grammar rule for it end in _null so that it shows up in our annotation tool as silently-ignored syntax.
An example line that will never be supported is:

set protocols evpn duplicate-mac-detection auto-recovery-time 15

Ask if you are unsure.

Note that the todo(ctx) for each feature should remain until support is added all the way through the full Batfish pipeline.


projects/batfish/src/main/java/org/batfish/representation/juniper/Vlan.java, line 52 at r1 (raw file):

  }

  public void setVniId(int vniId) {

nit: use @Nullable Integer so the field can be cleared without throwing an exception.


projects/batfish/src/test/java/org/batfish/grammar/flatjuniper/FlatJuniperGrammarTest.java, line 4987 at r1 (raw file):

  }

  @Test

Please add tests for the other files you added. If they don't parse and adding parsing for them is out of scope for this PR (your choice), then use the annotation:
@Test(expected = BatfishException.class)
or @Test(expected = ParserException.class) depending on which exception gets thrown, and add a /// TODO <something> style comment for each.


projects/batfish/src/test/resources/org/batfish/grammar/juniper/testconfigs/interface-vni, line 4 at r1 (raw file):

set system host-name interface-vni
#
set interfaces ge-0/0/0 unit 0 family ethernet-switching vlan members VLAN_TEST

I believe by default this will become an access-mode switchport. Is that intentional? Fine if so.

Copy link
Contributor Author

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @arifogel and @jeffkala)

a discussion (no related file):

Previously, arifogel (Ari Fogel) wrote…

Thank you so much for your contribution! I'll be your shepherd to get this merged.

This is optional (really), but the review process will be smoother if you use reviewable to reply to my comments. Just click the reviewable logo at the top of the github conversation to get started. If you find it a hassle, replying on github is fine.
Either way, the full conversation will be available there.

Thanks for the review. I will look through the comments and work on the fixes. I appreciate the details you provided.


Copy link
Contributor Author

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @arifogel and @jeffkala)

a discussion (no related file):

Previously, arifogel (Ari Fogel) wrote…

I see that checkstyle is failing. To fix this, run tools/fix_java_format.sh

Done.



projects/batfish/src/main/java/org/batfish/representation/juniper/Vlan.java, line 52 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

nit: use @Nullable Integer so the field can be cleared without throwing an exception.

This is true even on a set call?

Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jeffkala)

a discussion (no related file):

Previously, jeffkala (Jeff Kala) wrote…

Done.

Did you forget to push? I don't see any changes



projects/batfish/src/main/java/org/batfish/representation/juniper/Vlan.java, line 52 at r1 (raw file):

Previously, jeffkala (Jeff Kala) wrote…

This is true even on a set call?

I suppose for Juniper it's not super important based on how we preprocess (if you add a delete at the end, batfish will never see the original setting). You can ignore.

Copy link
Contributor Author

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @arifogel and @jeffkala)


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniperParser.g4, line 227 at r1 (raw file):

private static @nonnull Optional toInteger(ParserRuleContext messageCtx, Vni_numberContext ctx) {
return toIntegerInSpace(messageCtx, ctx.uint32(), "vni", VNI_NUMBER_RANGE);
}
I'm not having much luck with these fixes. toIntegerInSpace doesn't exist in the FlatJuniper, I've looked and found examples in other vendors and implemented it without luck.

As far as the other parts, would vni_number be defined in the FlatJuniper_common.g4 file? And then it referenced in the FlatJuniperParser.g4 under the vlt_vni_id that was created? Like below?

vlt_vni_id
:
   VXLAN VNI vni_number
;

Copy link
Contributor Author

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @arifogel and @jeffkala)

a discussion (no related file):

Previously, arifogel (Ari Fogel) wrote…

Did you forget to push? I don't see any changes

Correct, waiting to push until I can get some of the other fixes implemented.


@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #8112 (74bd9b4) into master (633806b) will decrease coverage by 0.01%.
The diff coverage is 25.00%.

❗ Current head 74bd9b4 differs from pull request most recent head af563f1. Consider uploading reports for the commit af563f1 to get more accurate results

@@             Coverage Diff              @@
##             master    #8112      +/-   ##
============================================
- Coverage     74.48%   74.47%   -0.02%     
+ Complexity    43273    43267       -6     
============================================
  Files          3372     3372              
  Lines        169312   169320       +8     
  Branches      20268    20268              
============================================
- Hits         126117   126098      -19     
- Misses        33517    33540      +23     
- Partials       9678     9682       +4     
Impacted Files Coverage Δ
.../java/org/batfish/representation/juniper/Vlan.java 82.35% <0.00%> (-17.65%) ⬇️
...fish/grammar/flatjuniper/ConfigurationBuilder.java 75.39% <40.00%> (-0.05%) ⬇️
...src/main/java/org/batfish/coordinator/PoolMgr.java 54.76% <0.00%> (-4.77%) ⬇️
...batfish/representation/aws/LoadBalancerTarget.java 54.16% <0.00%> (-4.17%) ⬇️
...fish/bddreachability/BDDLoopDetectionAnalysis.java 80.23% <0.00%> (-2.33%) ⬇️
...main/java/org/batfish/datamodel/acl/AclTracer.java 63.69% <0.00%> (-1.28%) ⬇️
...ain/java/org/batfish/storage/FileBasedStorage.java 85.47% <0.00%> (-0.62%) ⬇️
...ain/java/org/batfish/coordinator/WorkQueueMgr.java 70.93% <0.00%> (-0.59%) ⬇️
...batfish/src/main/java/org/batfish/main/Driver.java 48.00% <0.00%> (-0.45%) ⬇️
...src/main/java/org/batfish/coordinator/WorkMgr.java 75.57% <0.00%> (-0.31%) ⬇️

Copy link
Member

@arifogel arifogel 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 r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jeffkala)


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniperParser.g4, line 227 at r1 (raw file):
toIntegerInSpace is already implemented in master on line 5318 in ConfigurationBuilder.java, packageorg.batfish.grammar.flatjuniper.

  private @Nonnull Optional<Integer> toIntegerInSpace(
      ParserRuleContext messageCtx, ParserRuleContext ctx, IntegerSpace space, String name) {
    int num = Integer.parseInt(ctx.getText());
    if (!space.contains(num)) {
      warn(messageCtx, String.format("Expected %s in range %s, but got '%d'", name, space, num));
      return Optional.empty();
    }
    return Optional.of(num);
  }

Are you up to date? May want to try merging with master if you don't see it.

As far as the other parts, would vni_number be defined in the FlatJuniper_common.g4 file? And then it referenced in the FlatJuniperParser.g4 under the vlt_vni_id that was created? Like below?

Exactly.

Copy link
Contributor Author

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @arifogel)


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniperParser.g4, line 227 at r1 (raw file):

Previously, jeffkala (Jeff Kala) wrote…

private static @nonnull Optional toInteger(ParserRuleContext messageCtx, Vni_numberContext ctx) {
return toIntegerInSpace(messageCtx, ctx.uint32(), "vni", VNI_NUMBER_RANGE);
}
I'm not having much luck with these fixes. toIntegerInSpace doesn't exist in the FlatJuniper, I've looked and found examples in other vendors and implemented it without luck.

As far as the other parts, would vni_number be defined in the FlatJuniper_common.g4 file? And then it referenced in the FlatJuniperParser.g4 under the vlt_vni_id that was created? Like below?

vlt_vni_id
:
   VXLAN VNI vni_number
;

Thing I figured it out.


projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java, line 2392 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

nit: usually we just do todo(ctx) for the standard unsupported feature message.

On that note, please add exit rules with todo(ctx) to cover all new grammar you added (just need one per top-level rule) that will eventually be supported. If something you added will never be supported , make the name for the grammar rule for it end in _null so that it shows up in our annotation tool as silently-ignored syntax.
An example line that will never be supported is:

set protocols evpn duplicate-mac-detection auto-recovery-time 15

Ask if you are unsure.

Note that the todo(ctx) for each feature should remain until support is added all the way through the full Batfish pipeline.

Done.


projects/batfish/src/test/java/org/batfish/grammar/flatjuniper/FlatJuniperGrammarTest.java, line 4987 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Please add tests for the other files you added. If they don't parse and adding parsing for them is out of scope for this PR (your choice), then use the annotation:
@Test(expected = BatfishException.class)
or @Test(expected = ParserException.class) depending on which exception gets thrown, and add a /// TODO <something> style comment for each.

Done.

Copy link
Contributor Author

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @arifogel)


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniperParser.g4, line 227 at r1 (raw file):

Previously, jeffkala (Jeff Kala) wrote…

Thing I figured it out.

Ahh, let me merge in master been a week or so since I've merged.

Copy link
Member

@arifogel arifogel 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 r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @arifogel)

Copy link
Member

@arifogel arifogel 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 r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jeffkala)


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_evpn.g4, line 22 at r4 (raw file):

:
    EXTENDED_VNI_LIST (
        range

Please instead make a vni_range grammar rule whose individual elements are vni_number. Using range is unsafe because it uses dec, which might result in an exception in ConfigurationBuilder if VNIs are extracted using Integer.parseInt.

Note that if the allowed syntax does not accept comma-separated lists (e.g. 1-5,6) but only dashed ranges (e.g. 1-5), you should adjust the definition accordingly.

No need to add extraction function in ConfigurationBuilder for vni_range if that's out of scope for this PR.


projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java, line 6547 at r4 (raw file):

  @Override
  public void exitVlt_vni_id(Vlt_vni_idContext ctx) {
    //    int vni = toInteger(ctx, ctx);

please remove.


projects/batfish/src/test/java/org/batfish/grammar/flatjuniper/FlatJuniperGrammarTest.java, line 1544 at r4 (raw file):

  @Test
  public void testEvpn() {

nit: if this will eventually just test the vendor model, it should be named testEvpnExtraction. Later there should be a test called testEvpnConversion which tests conversion to the vendor-independent model.


projects/batfish/src/test/java/org/batfish/grammar/flatjuniper/FlatJuniperGrammarTest.java, line 2900 at r4 (raw file):

  @Test
  public void testInterfaceVni() {

nit: change name to testInterfaceVniExtraction

Copy link
Contributor Author

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @arifogel and @jeffkala)


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_evpn.g4, line 22 at r4 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Please instead make a vni_range grammar rule whose individual elements are vni_number. Using range is unsafe because it uses dec, which might result in an exception in ConfigurationBuilder if VNIs are extracted using Integer.parseInt.

Note that if the allowed syntax does not accept comma-separated lists (e.g. 1-5,6) but only dashed ranges (e.g. 1-5), you should adjust the definition accordingly.

No need to add extraction function in ConfigurationBuilder for vni_range if that's out of scope for this PR.

the range was what was already implemented, the command just also support an "all" option. Would me changing the range as mentioned cause issues elsewhere? I left it alone since that was what existed previously.

Copy link
Member

@arifogel arifogel 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 r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jeffkala)


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_evpn.g4, line 22 at r4 (raw file):

Previously, jeffkala (Jeff Kala) wrote…

the range was what was already implemented, the command just also support an "all" option. Would me changing the range as mentioned cause issues elsewhere? I left it alone since that was what existed previously.

Don't change range; just make a new vni_range rule. range is currently used elsewhere, but it is deprecated because deep in its definition it uses dec. At some point range, subrange, and dec will be removed, so there should be no new usages.

Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jeffkala)

a discussion (no related file):
Looks like you need to fix:

  • testOverlayEcmp, which needs to be updated after switching to todo(ctx). The warning comment will be "This feature is not currently supported", and the warning text will be the text matched by the grammar rule that is the type of ctx.
  • org/batfish/grammar/flatjuniper/FlatJuniperGrammarTest.java:1545: UnusedLocalVariable: Avoid unused local variables such as 'c'. (just don't declare a variable).

Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jeffkala)


projects/batfish/src/test/java/org/batfish/grammar/flatjuniper/FlatJuniperGrammarTest.java, line 5020 at r5 (raw file):

  @Test
  public void testOverlayEcmp() throws IOException {
    String hostname = "juniper-overlay-ecmp";

You should re-add this test as a simple parse test like the others to prove the file can be parsed.

Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jeffkala)

a discussion (no related file):

Previously, arifogel (Ari Fogel) wrote…

Looks like you need to fix:

  • testOverlayEcmp, which needs to be updated after switching to todo(ctx). The warning comment will be "This feature is not currently supported", and the warning text will be the text matched by the grammar rule that is the type of ctx.
  • org/batfish/grammar/flatjuniper/FlatJuniperGrammarTest.java:1545: UnusedLocalVariable: Avoid unused local variables such as 'c'. (just don't declare a variable).

(Fine to not bother checking warning content for testOverlayEcmp)


Copy link
Contributor Author

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @arifogel and @jeffkala)


projects/batfish/src/test/java/org/batfish/grammar/flatjuniper/FlatJuniperGrammarTest.java, line 5020 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

You should re-add this test as a simple parse test like the others to prove the file can be parsed.

Doing it now, sorry for the miscommunication.

Copy link
Contributor Author

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 13 files reviewed, 1 unresolved discussion (waiting on @arifogel and @jeffkala)

a discussion (no related file):
Working through the remaining test failure. Looking at juniper docs I didn't see set routing-instances fabric protocols evpn extended-vni-list 100 being supported. This was already supported and my changes broke it, looking into docs to validate this is supported along with the "list" type view [ 100 ].


Copy link
Contributor Author

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 13 files reviewed, 1 unresolved discussion (waiting on @arifogel and @jeffkala)

a discussion (no related file):

Previously, jeffkala (Jeff Kala) wrote…

Working through the remaining test failure. Looking at juniper docs I didn't see set routing-instances fabric protocols evpn extended-vni-list 100 being supported. This was already supported and my changes broke it, looking into docs to validate this is supported along with the "list" type view [ 100 ].

Modified the grammar, but still having a failure that I'm not currently understanding as it appears to be in a section that I didn't modify. Looking through it now.


Copy link
Contributor Author

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 13 files reviewed, 1 unresolved discussion (waiting on @arifogel and @jeffkala)

a discussion (no related file):

Previously, jeffkala (Jeff Kala) wrote…

Modified the grammar, but still having a failure that I'm not currently understanding as it appears to be in a section that I didn't modify. Looking through it now.

The error in the test makes sense because I did change the grammar that was used. I searched through PRs/Issues to see how to fix the problem but couldn't determine the proper way to fix it. Not sure if the file tests/parsing-tests/unit-tests.ref.testout should be moved into tests/parsing-tests/unit-tests.ref or if that should be done in the pipeline somewhere.


Copy link
Member

@ratulm ratulm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 13 files reviewed, 1 unresolved discussion (waiting on @arifogel, @jeffkala, and @ratulm)

a discussion (no related file):

Previously, jeffkala (Jeff Kala) wrote…

The error in the test makes sense because I did change the grammar that was used. I searched through PRs/Issues to see how to fix the problem but couldn't determine the proper way to fix it. Not sure if the file tests/parsing-tests/unit-tests.ref.testout should be moved into tests/parsing-tests/unit-tests.ref or if that should be done in the pipeline somewhere.

Yes, if the .testout file looks good to you (i.e., differences make sense), make it the new .ref file and commit it.


Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 6 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jeffkala)


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_common.g4, line 688 at r8 (raw file):

  vni_number
  | WS

WS is always either skipped or on the hidden channel. Either way, the parser never sees it (it only sees tokens on the default channel), so it should never feature in a parser rule. Note the action skip or channel(HIDDEN) in all WS-type token definitions in FlatJuniperLexer.g4.

Just remove the | WS, since that will never be matched.


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_evpn.g4, line 22 at r8 (raw file):

:
    EXTENDED_VNI_LIST (
        OPEN_BRACKET (

nit: this would be more legible if you Just put all of this alternative on one line and ditch the parentheses. The + operator has highest precedence so it will be functionally equivalent.
That is, put OPEN_BRACKET vni_range+ CLOSE_BRACKET on one line.

Copy link
Member

@arifogel arifogel 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 r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jeffkala)

Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jeffkala)


projects/batfish/src/main/antlr4/org/batfish/grammar/flatjuniper/FlatJuniper_common.g4, line 687 at r10 (raw file):

  // Example extended-vni-list [ 10-50 60 70]

  vni_number

nit: change this to start = vni_number (or any other alias) so this vni_number can be distinguished easily inConfigurationBuilder from the other two below

Copy link
Member

@arifogel arifogel 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 r11, all commit messages.
Dismissed @jeffkala from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jeffkala)

Copy link
Contributor Author

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @arifogel and @ratulm)

a discussion (no related file):
After the update to EXTENDED_VNI_LIST: 'extended-vni-list' -> pushMode(M_ExtendedVniList); it no longer catches this version of the command set protocols evpn extended-vni-list all. Even though in the grammar I have an option to capture ALL. When using that pushMode in the Lexer do I need to account for the ALL there instead?


Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jeffkala)

a discussion (no related file):

Previously, jeffkala (Jeff Kala) wrote…

After the update to EXTENDED_VNI_LIST: 'extended-vni-list' -> pushMode(M_ExtendedVniList); it no longer catches this version of the command set protocols evpn extended-vni-list all. Even though in the grammar I have an option to capture ALL. When using that pushMode in the Lexer do I need to account for the ALL there instead?

I sent you a PR to fix this: https://github.com/jeffkala/batfish/pull/2


Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jeffkala)

a discussion (no related file):

When using that pushMode in the Lexer do I need to account for the ALL there instead?

Whatever mode the lexer is currently in, only the tokens in that mode may be emitted. Any text not matched by the definition of a token in that mode is rejected and causes a parse error. I forgot to add a token for all in the mode entered after extended-vni-list, so that's what's fixed in the linked PR.


Copy link
Contributor Author

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 13 of 14 files reviewed, 1 unresolved discussion (waiting on @arifogel)

a discussion (no related file):

Previously, arifogel (Ari Fogel) wrote…

When using that pushMode in the Lexer do I need to account for the ALL there instead?

Whatever mode the lexer is currently in, only the tokens in that mode may be emitted. Any text not matched by the definition of a token in that mode is rejected and causes a parse error. I forgot to add a token for all in the mode entered after extended-vni-list, so that's what's fixed in the linked PR.

Just merged that in, cant thank you enough for all the assistance. Future PRs should go much more smooth.


Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jeffkala)

Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissed @jeffkala from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jeffkala)

@arifogel arifogel merged commit ef61a3a into batfish:master Mar 10, 2022
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

4 participants