-
Notifications
You must be signed in to change notification settings - Fork 228
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
PAN: app-override parsing and extraction #6626
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6626 +/- ##
===========================================
Coverage 73.42% 73.42%
- Complexity 35888 36415 +527
===========================================
Files 2847 2916 +69
Lines 144818 146801 +1983
Branches 17523 17692 +169
===========================================
+ Hits 106328 107787 +1459
- Misses 30058 30521 +463
- Partials 8432 8493 +61 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 10 of 10 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dhalperi and @sfraint)
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 2467 at r1 (raw file):
public void exitSrao_application(Srao_applicationContext ctx) { String application = getText(ctx.application); _currentApplicationOverrideRule.setApplication(application);
we won't need the unique application name to be sure we're applying the rule to the right application? Same thought with endpoint and zone names.
projects/batfish/src/test/java/org/batfish/grammar/palo_alto/PaloAltoGrammarTest.java, line 3905 at r1 (raw file):
@Test public void testApplicationOverrideRuleExtraction() throws IOException { String hostname = "application-override-rule-reference";
Looks like you're using this test config for more than just reference checking. Could rename (but i don't feel strongly about it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 8 of 10 files reviewed, 1 unresolved discussion (waiting on @corinaminer and @dhalperi)
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 2467 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
we won't need the unique application name to be sure we're applying the rule to the right application? Same thought with endpoint and zone names.
Discussed offline, but: unique name isn't needed here as we know the zone/endpoint/app is in the same vsys as the rule
projects/batfish/src/test/java/org/batfish/grammar/palo_alto/PaloAltoGrammarTest.java, line 3905 at r1 (raw file):
Previously, corinaminer (Corina Miner) wrote…
Looks like you're using this test config for more than just reference checking. Could rename (but i don't feel strongly about it)
renamed
There was a problem hiding this 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.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dhalperi)
Add parsing and extraction for Palo Alto application-override rules.
Conversion will be done separately, in another PR.