> 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 > > Hugo Trippaers wrote: > 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. > > Ritu Sabharwal wrote: > Hi Hugo, > > Thanks for reviewing the plugin. > > Few points: > > 1. The configuration using properties file was part of the Design > document > https://cwiki.apache.org/confluence/display/CLOUDSTACK/Brocade+Network+Plugin+to+Orchestrate+Brocade+VDX+Switches > and was not raised as a concern earlier and the implementation is based on > that. > > Changing the implementation right now would involve some work. Can this > be done as an enhancement as part of bug fix during testing? > > 2. For functional testcases, we are doing functional testing but how will > functional testcases be run by the community without the Brocade hardware? > > We are working on writing the unit and functional testcases. > > > Ritu Sabharwal wrote: > Hi Hugo, > > I have added the unit tests, working on the the marvin integration tests. > > Please reply back for the properties file change if that can be treated > an an enhancement bug fix during testing phase. > > Thanks for reviewing and supporting the plugin. > > Thanks & Regards, > Ritu S.
Hi Hugo, Just an update, I am working on removing the reading of configurations from the properties file and adding them as part of network provider configuration through APIs and UI. Also, working ont he marvin integration tests. Should be able to update code by tomorrow for review. Thanks & Regards, Ritu S. - Ritu ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23314/#review47435 ----------------------------------------------------------- On July 11, 2014, 11:44 p.m., Ritu Sabharwal wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23314/ > ----------------------------------------------------------- > > (Updated July 11, 2014, 11:44 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 > > plugins/network-elements/brocade-vcs/test/com/cloud/network/brocade/BrocadeVcsApiTest.java > PRE-CREATION > > plugins/network-elements/brocade-vcs/test/com/cloud/network/guru/BrocadeVcsGuestNetworkGuruTest.java > PRE-CREATION > > plugins/network-elements/brocade-vcs/test/com/cloud/network/resource/BrocadeVcsResourceTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/23314/diff/ > > > Testing > ------- > > > Thanks, > > Ritu Sabharwal > >
