> 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.
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. > On July 8, 2014, 9:29 a.m., Hugo Trippaers wrote: > > plugins/network-elements/brocade-vcs/src/com/cloud/network/guru/BrocadeVcsGuestNetworkGuru.java, > > line 303 > > <https://reviews.apache.org/r/23314/diff/2/?file=624856#file624856line303> > > > > This TODO is fixed right? This is fixed. Just the comment was there. > On July 8, 2014, 9:29 a.m., Hugo Trippaers wrote: > > plugins/network-elements/brocade-vcs/src/com/cloud/network/guru/BrocadeVcsGuestNetworkGuru.java, > > line 106 > > <https://reviews.apache.org/r/23314/diff/2/?file=624856#file624856line106> > > > > Please remove commented code. Removed the commented code. > On July 8, 2014, 9:29 a.m., Hugo Trippaers wrote: > > plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/CacheManager.java, > > line 50 > > <https://reviews.apache.org/r/23314/diff/2/?file=624850#file624850line50> > > > > You should get all properties from the CloudStack configuration > > mechanism. Either via the global config or via the settings on the network > > service provider configuration (preferred) The Brocade network gear details are read from config file for now because of the time constraint to meet the 4.5 timelines. I plan to enhance this and get these values from network service provider configuration for future versions. > On July 8, 2014, 9:29 a.m., Hugo Trippaers wrote: > > plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/CacheManager.java, > > line 86 > > <https://reviews.apache.org/r/23314/diff/2/?file=624850#file624850line86> > > > > This looks like something that should be persisted in the cloudstack > > database instead of stored outside cloudstack. The Brocade network gear mapping details are read from config file for now because of the time constraint to meet the 4.5 timelines. I plan to enhance this and get these values from network service provider configuration for future versions and persist it DB. - Ritu ----------------------------------------------------------- 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 > >
