> 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 > >
