-
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
Identify metamako configuration format #1042
Conversation
Reviewed 3 of 3 files at r1. projects/batfish/src/main/java/org/batfish/grammar/VendorConfigurationFormatDetector.java, line 188 at r1 (raw file):
So the goal of this part of the code is to find something that is unique to the vendor at hand. In this case, I think this is actually very common (Metamako, like many others, seems to be Cisco-like). The better detector might be something that finds lines like these:
Especially since the I would use a gracious regex to catch lines like this. Comments from Reviewable |
Review status: all files reviewed at latest revision, 1 unresolved discussion. projects/batfish/src/main/java/org/batfish/grammar/VendorConfigurationFormatDetector.java, line 188 at r1 (raw file): Previously, dhalperi (Dan Halperin) wrote…
isn't that a comment that could be changed arbitrarily? seems like most of the other possible formats are identified by unique phrases in the config text. Comments from Reviewable |
Devices are now identified as Metamako if they have any of the following:
Of 29 sample Metamako configurations, 21 matched (at least) one of the first two criteria; all of them matched the third. Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion. projects/batfish/src/main/java/org/batfish/grammar/VendorConfigurationFormatDetector.java, line 188 at r1 (raw file): Previously, corinaminer (Corina Miner) wrote…
Added better identification criteria. Comments from Reviewable |
Tests are failing because there are tests for it. I'm of mixed opinion -- we don't have grammar or interesting tests of this stuff -- as indicated by the failing cisco-assumptions that led to this work -- but we do parse its structure somewhat usefully in a cisco-like way right now. @arifogel thoughts? Review status: 2 of 3 files reviewed at latest revision, all discussions resolved, some commit checks failed. projects/batfish/src/main/java/org/batfish/grammar/VendorConfigurationFormatDetector.java, line 191 at r2 (raw file):
why two ifs vs three things projects/batfish/src/main/java/org/batfish/grammar/VendorConfigurationFormatDetector.java, line 192 at r2 (raw file):
compile the regex once ( Comments from Reviewable |
Reviewed 1 of 1 files at r2. Comments from Reviewable |
As discussed offline, this format for now should be detected but unsupported. Reviewed 2 of 3 files at r1, 1 of 1 files at r2. Comments from Reviewable |
Review status: 2 of 6 files reviewed at latest revision, 2 unresolved discussions. projects/batfish/src/main/java/org/batfish/grammar/VendorConfigurationFormatDetector.java, line 191 at r2 (raw file): Previously, dhalperi (Dan Halperin) wrote…
it was to avoid creating the matcher for ~75% of metamako configurations. new setup manages to do that without splitting up the if. projects/batfish/src/main/java/org/batfish/grammar/VendorConfigurationFormatDetector.java, line 192 at r2 (raw file): Previously, dhalperi (Dan Halperin) wrote…
done for this and all other patterns in the file Comments from Reviewable |
Reviewed 4 of 4 files at r3. projects/batfish/src/main/java/org/batfish/grammar/VendorConfigurationFormatDetector.java, line 192 at r2 (raw file): Previously, corinaminer (Corina Miner) wrote…
In future, would prefer wide-scale refactoring changes in a separate PR. It confuses this PR (e.g., if we want to revert this PR we revert the refactoring) and is not a blocker for this PR. LGTM. Comments from Reviewable |
Identifies Metamako configurations by the phrase "username admin secret". As far as I could tell with the configurations at hand, that phrase appears in only and all Metamako device configurations. @saparikh @arifogel will that work more broadly, as far as you know?