cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chip Childers <chip.child...@sungard.com>
Subject Re: Palo Alto Firewall Integration - Review Process
Date Tue, 28 May 2013 18:07:25 GMT
On Tue, May 28, 2013 at 01:32:48PM -0400, Will Stevens wrote:
> Hey All,
> I am getting close to finishing up this integration, so I want to make sure
> I understand the process and what is required for submitting my code for
> review.
> 
> I have read this and am comfortable with its content:
> http://cloudstack.apache.org/develop/non-committer.html
> 
> You can check out more details regarding this integration here:
> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Palo+Alto+Firewall+Integration
> 
> Please let me know if you feel I am missing anything on that page.  That is
> still a work in progress, but it does cover the functionality being added
> pretty well.  The screenshots are not complete yet, but they are at about
> 90% right now.
> 
> On that page I have linked a public repo which has a recent working version
> of the code (not feature complete yet and still needs some clean up).

Thanks for doing that!

> 
> Here are the questions that I have about the process:
> - Do I need to include tests for this code?  If so, is this documented
> somewhere?  Since this is an integration with an external device, how would
> tests be written to pass without actually connecting to a device?

2 test types:

1 - unit tests using a mocking framework are needed for non-trivial
logic (complex methods)

2 - integration tests using the marvin framework are the best method of
providing automated testing of a specific integration.  However, you
might want to see if your functionality is *already* covered in the test
suite.  If you are only implementing a driver for a specific technology,
it might be easy to just play a set of tests against an environment with
that device enabled.

> - There is a small limitation in core which did not have any dependancies
> which I have fixed (Sheng and I have discussed this briefly).  Basically, I
> added support for multiple networks per account when the source nat type is
> 'per account' with an external device.  Question: Should I be submitting
> two patches; one which only addresses this core fix (about 5 lines of code)
> and one which addresses the addition of the palo_alto network plugin?  Or,
> should I submit it all as one patch?

Best to do it as 2.  Note in the new feature patch that it relies on the
"core" patch.

> - Since this is an integration with a 3rd party product; should I setup
> a publicly accessible system where the functionality can be reviewed, or
> should I work with Palo Alto to get demo licenses for their VM firewall
> appliances and provide the reviewers licences to test the functionality?  I
> am not sure how this aspect should work, so let me know what the best
> approach would be.

We don't have a good model for this.  Your demo license proposal sounds
interesting though.  Perhaps that's the model we *should* be using
whenever possible?

> 
> I think thats it.  Please let me know if something is not clear or if you
> feel I need to flush out some of the details somewhere.
> 
> Thanks,
> 
> Will

Mime
View raw message