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

fix #638 - update aws code and add init, node tests #642

Merged
merged 1 commit into from
Nov 6, 2017
Merged

Conversation

arifogel
Copy link
Member

@arifogel arifogel commented Nov 6, 2017

This change is Reviewable

@arifogel arifogel requested review from dhalperi and removed request for dhalperi November 6, 2017 19:22
@dhalperi
Copy link
Member

dhalperi commented Nov 6, 2017

Reviewed 35 of 35 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


projects/batfish/src/main/java/org/batfish/representation/aws_vpcs/InternetGateway.java, line 62 at r1 (raw file):

      vpcIface.setPrefix(vpcIfacePrefix);
      vpcConfigNode.getInterfaces().put(vpcIfaceName, vpcIface);
      vpcConfigNode.initDefaultVrf().getInterfaces().put(vpcIfaceName, vpcIface);

given that you call the function twice and it may only initialize if not yet present, seems actually better to call it get or at least getOrInit.


projects/batfish/src/main/java/org/batfish/representation/aws_vpcs/Subnet.java, line 169 at r1 (raw file):

    // add the interface to the vpc router
    Configuration vpcConfigNode = awsVpcConfiguration.getConfigurationNodes().get(_vpcId);
    vpcConfigNode.setConfigurationFormat(ConfigurationFormat.AWS_VPC);

question for self, why was this not done already?


tests/basic/init-example-aws.ref, line 19 at r1 (raw file):

    "numFailed" : 0,
    "numPassed" : 0,
    "numResults" : 0

is this the right output for parsing an AWS network? Seems useless.


Comments from Reviewable

@arifogel
Copy link
Member Author

arifogel commented Nov 6, 2017

Review status: all files reviewed at latest revision, 3 unresolved discussions.


projects/batfish/src/main/java/org/batfish/representation/aws_vpcs/InternetGateway.java, line 62 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

given that you call the function twice and it may only initialize if not yet present, seems actually better to call it get or at least getOrInit.

How would you feel about an AWS helper function that generates a Configuration with the right format and a default VRF? Then we could eliminate Configuration.initVrf()


projects/batfish/src/main/java/org/batfish/representation/aws_vpcs/Subnet.java, line 169 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

question for self, why was this not done already?

See reply about helper function.


tests/basic/init-example-aws.ref, line 19 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

is this the right output for parsing an AWS network? Seems useless.

In general, JSON parsing produces no meaningful output when successful. If you are referring only to the summary portion, then I would posit that its presence is useless in almost every ref (but that is out of scope for this PR). Is there a specific modification you propose?


Comments from Reviewable

@dhalperi
Copy link
Member

dhalperi commented Nov 6, 2017

Review status: all files reviewed at latest revision, 2 unresolved discussions.


projects/batfish/src/main/java/org/batfish/representation/aws_vpcs/InternetGateway.java, line 62 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

How would you feel about an AWS helper function that generates a Configuration with the right format and a default VRF? Then we could eliminate Configuration.initVrf()

No strong opinion - I find the code fairly confusing as is.


tests/basic/init-example-aws.ref, line 19 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

In general, JSON parsing produces no meaningful output when successful. If you are referring only to the summary portion, then I would posit that its presence is useless in almost every ref (but that is out of scope for this PR). Is there a specific modification you propose?

For "normal" testrigs, both ParseVendorConfiguration andInitInfo answers tell you files they read and devices they set up. I don't think your outstanding #561 changes this. But we're not providing any of that info here.


Comments from Reviewable

@dhalperi
Copy link
Member

dhalperi commented Nov 6, 2017

projects/batfish/src/main/java/org/batfish/representation/aws_vpcs/InternetGateway.java, line 62 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

No strong opinion - I find the code fairly confusing as is.

Confusing is wrong .. just complicated :). Not sure I have built a good mental model.


Comments from Reviewable

@arifogel
Copy link
Member Author

arifogel commented Nov 6, 2017

Review status: all files reviewed at latest revision, 2 unresolved discussions.


projects/batfish/src/main/java/org/batfish/representation/aws_vpcs/InternetGateway.java, line 62 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Confusing is wrong .. just complicated :). Not sure I have built a good mental model.

OK I'll just do it then.


tests/basic/init-example-aws.ref, line 19 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

For "normal" testrigs, both ParseVendorConfiguration andInitInfo answers tell you files they read and devices they set up. I don't think your outstanding #561 changes this. But we're not providing any of that info here.

Gotcha. What you are looking for could be an addon to whatever we end up doing with #561. I will file defer and file an issue.


Comments from Reviewable

@arifogel
Copy link
Member Author

arifogel commented Nov 6, 2017

Review status: 29 of 36 files reviewed at latest revision, 2 unresolved discussions.


tests/basic/init-example-aws.ref, line 19 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Gotcha. What you are looking for could be an addon to whatever we end up doing with #561. I will file defer and file an issue.

Filed #647


Comments from Reviewable

@dhalperi
Copy link
Member

dhalperi commented Nov 6, 2017

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


Comments from Reviewable

@dhalperi
Copy link
Member

dhalperi commented Nov 6, 2017

:lgtm:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@arifogel arifogel merged commit 0b515e1 into master Nov 6, 2017
@arifogel arifogel deleted the ari-fix-aws branch November 6, 2017 21:59
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