cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Hugo Trippaers" <htrippa...@schubergphilis.com>
Subject Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin
Date Wed, 09 Jul 2014 12:01:44 GMT


> On July 8, 2014, 9:29 a.m., Hugo Trippaers wrote:
> > Thanks for make this a separate review, much easier to read.
> > 
> > The licenses are not added to all files and in some files they appear not to be
complete. A good trick to solve this is to include the license-maven-plugin in the build section.
See plugins/network-elements/nicira-nvp/pom.xml for an example.
> > 
> > Also the plugin doesn't contain any tests. We need unit tests or functional tests
(and preferably both) to validate that this plugin works and keeps on working in future versions
of CloudStack. It would be great if you could supply unit tests to validate this inner workings
of this module and marvin based integration tests that we can run to see if this work with
the intended hardware.
> > 
> > The last general point is that we would also need documentation on this plugin so
administrators will know how to use this.
> 
> Ritu  Sabharwal wrote:
>     Thanks for reviewing the code.
>     
>     I have fixed the license in all the files.
>     
>     I am working on writing the unit tests for the plugin.
>     
>     I am working on providing the documentation on using the plugin.
>     
>     I have tested the plugin with the simulator and see the configurations on Brocade
switches happening. Currently, I am setting up a real hardware setup with Vmware hypervisor
and Brocade switches so test the connectivity between spawned VMs.
>     
>     Can I continue writing the unit tests and marvin integration tests and documentation
after the July 19 date so that this plugin makes to 4.5 feature freeze date.
>     
>     I have updated the diffs with the changes for the license and other comments.
> 
> Ritu  Sabharwal wrote:
>     I am planning to include the following in the documentation:
>      - Brocade switch setup
>      - Connections to the Hypervisor hosts
>      - the flow to use the plugin from UI

Thanks for fixing the open issues.

I think it's ok to work on the documentation while a release is being tested (eventhough it
is far better to have it ready for the testers). However unittests and functional testcases
need to be there before we can merge the plugin. Without it we can not properly test the plugin
to see if it is fit to be part of the release. 

I'd rather have a completely done plugin in the 4.6 release than something that is not complete
in 4.5. So currently the blockers for a merge into master are the two property files and we
need unit and integration tests.


- Hugo


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23314/#review47435
-----------------------------------------------------------


On July 8, 2014, 6:49 p.m., Ritu  Sabharwal wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23314/
> -----------------------------------------------------------
> 
> (Updated July 8, 2014, 6:49 p.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Bugs: CLOUDSTACK-6823
>     https://issues.apache.org/jira/browse/CLOUDSTACK-6823
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> Plugin specific code.
> 
> 
> Diffs
> -----
> 
>   plugins/network-elements/brocade-vcs/pom.xml PRE-CREATION 
>   plugins/network-elements/brocade-vcs/resources/BrocadeInterfaceSchema.xsd PRE-CREATION

>   plugins/network-elements/brocade-vcs/resources/BrocadePortProfileSchema.xsd PRE-CREATION

>   plugins/network-elements/brocade-vcs/resources/BrocadeShowVcsSchema.xsd PRE-CREATION

>   plugins/network-elements/brocade-vcs/resources/META-INF/cloudstack/vcs/module.properties
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/resources/META-INF/cloudstack/vcs/spring-vcs-context.xml
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/AssociateMacToNetworkAnswer.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/AssociateMacToNetworkCommand.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/CreateNetworkAnswer.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/CreateNetworkCommand.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/DeleteNetworkAnswer.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/DeleteNetworkCommand.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/DisassociateMacFromNetworkAnswer.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/DisassociateMacFromNetworkCommand.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/StartupBrocadeVcsCommand.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/BrocadeVcsNetworkHostMappingVO.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/BrocadeVcsApi.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/BrocadeVcsApiException.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/Cache.java PRE-CREATION

>   plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/CacheManager.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/Constants.java PRE-CREATION

>   plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/Switch.java PRE-CREATION

>   plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsNetworkHostMappingDao.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsNetworkHostMappingDaoImpl.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/element/BrocadeVcsElement.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/guru/BrocadeVcsGuestNetworkGuru.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/resource/BrocadeVcsResource.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23314/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ritu  Sabharwal
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message