----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23314/#review47435 -----------------------------------------------------------
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. plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/CacheManager.java <https://reviews.apache.org/r/23314/#comment83255> 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) plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/CacheManager.java <https://reviews.apache.org/r/23314/#comment83256> This looks like something that should be persisted in the cloudstack database instead of stored outside cloudstack. plugins/network-elements/brocade-vcs/src/com/cloud/network/guru/BrocadeVcsGuestNetworkGuru.java <https://reviews.apache.org/r/23314/#comment83257> Please remove commented code. plugins/network-elements/brocade-vcs/src/com/cloud/network/guru/BrocadeVcsGuestNetworkGuru.java <https://reviews.apache.org/r/23314/#comment83258> This TODO is fixed right? - Hugo Trippaers On July 7, 2014, 7:08 p.m., Ritu Sabharwal wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23314/ > ----------------------------------------------------------- > > (Updated July 7, 2014, 7:08 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 > >
