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

Cisco OSPF reference bandwidth #886

Merged
merged 4 commits into from
Feb 1, 2018
Merged

Cisco OSPF reference bandwidth #886

merged 4 commits into from
Feb 1, 2018

Conversation

dhalperi
Copy link
Member

@dhalperi dhalperi commented Jan 30, 2018

Draft PR to set up larger work on #873


This change is Reviewable

@arifogel
Copy link
Member

Mostly good. Seems like a prime candidate for junit tests.


Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@dhalperi dhalperi force-pushed the cisco-ospf branch 3 times, most recently from b093414 to bbe6c00 Compare January 31, 2018 05:44
@dhalperi
Copy link
Member Author

Review status: 6 of 19 files reviewed at latest revision, 1 unresolved discussion.


projects/batfish/src/main/java/org/batfish/representation/cisco/OspfProcess.java, line 87 at r2 (raw file):

      case ARISTA: // EOS manual, Chapter 27, "auto-cost reference-bandwidth (OSPFv2)"
        return DEFAULT_REFERENCE_BANDWIDTH_10_MBPS;

@arifogel which other vendors use the Cisco parser?

Before the recent revision, the code would have crashed on Arista devices (since the default throws), but it was not caught by any existing test.

Should I make the default just use IOS default?


Comments from Reviewable

@dhalperi dhalperi changed the title (DRAFT) Cisco OSPF reference bandwidth Cisco OSPF reference bandwidth Jan 31, 2018
@arifogel
Copy link
Member

:lgtm:


Reviewed 12 of 13 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


projects/batfish/src/main/java/org/batfish/representation/cisco/OspfProcess.java, line 87 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

@arifogel which other vendors use the Cisco parser?

Before the recent revision, the code would have crashed on Arista devices (since the default throws), but it was not caught by any existing test.

Should I make the default just use IOS default?

I don't remember offhand which other vendors use that parse, but it's easy enough to check the switch case in ParseVendorConfigurationJob.
I don't think it's a good idea to have a default. I'd rather it throw by default.


Comments from Reviewable

@dhalperi
Copy link
Member Author

dhalperi commented Feb 1, 2018

Review status: 19 of 20 files reviewed at latest revision, 1 unresolved discussion.


projects/batfish/src/main/java/org/batfish/representation/cisco/OspfProcess.java, line 87 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

I don't remember offhand which other vendors use that parse, but it's easy enough to check the switch case in ParseVendorConfigurationJob.
I don't think it's a good idea to have a default. I'd rather it throw by default.

Done.


Comments from Reviewable

@dhalperi
Copy link
Member Author

dhalperi commented Feb 1, 2018

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@dhalperi dhalperi merged commit 5012fa7 into master Feb 1, 2018
@dhalperi dhalperi deleted the cisco-ospf branch February 1, 2018 07:55
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

2 participants