You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Ritu Sabharwal <rs...@brocade.com> on 2014/07/07 20:16:44 UTC

Review Request 23314: Plugin specific code for the Brocade Network Plugin

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

Review request for cloudstack.


Summary (updated)
-----------------

Plugin specific code for the Brocade Network Plugin


Bugs: CLOUDSTACK-6823
    https://issues.apache.org/jira/browse/CLOUDSTACK-6823


Repository: cloudstack-git


Description (updated)
-------

Plugin specific code.


Diffs (updated)
-----

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

Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin

Posted by Ritu Sabharwal <rs...@brocade.com>.

> On July 15, 2014, 11:21 a.m., Hugo Trippaers wrote:
> > This is looking good, when you addresses the comment on the system.js file in the other review i'll apply both patches. If the patches apply clean i'll run findbugs and cobertura to check for any issues and unit test coverage. 
> > 
> > Hugo

Hi Hugo,

I have fixed the system.js file. Also, removed the configuration reading from properties files. Now the plugin configurations are read as network service provider configurations from GUI or APIs. The unit tests are also updated for the change.

Please review it and do the needful so that we can make it by July 19.

Thanks & Regards,
Ritu S.


> On July 15, 2014, 11:21 a.m., Hugo Trippaers wrote:
> > plugins/network-elements/brocade-vcs/resources/BrocadeInterfaceSchema.xsd, line 5
> > <https://reviews.apache.org/r/23314/diff/2-3/?file=624832#file624832line5>
> >
> >     Did you check with the rat plugin if the license is accepted like this?
> >     
> >     I think the license is only properly detected with it is located in a comment like in the spring xml files.

The standard way to add comments to schema files is using annotation element. Reference: http://www.w3schools.com/schema/el_annotation.asp

The build throws parsing error if the comments are given like spring xml files. With annotation tag , the compilation goes fine and JAXB generation also works fine.

I did not find any xsd files in CloudStack codebase to get an example. What is the rat plugin?


- Ritu


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


On July 15, 2014, 9:52 p.m., Ritu  Sabharwal wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23314/
> -----------------------------------------------------------
> 
> (Updated July 15, 2014, 9:52 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/api/commands/AddBrocadeVcsDeviceCmd.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/api/commands/DeleteBrocadeVcsDeviceCmd.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/api/commands/ListBrocadeVcsDeviceNetworksCmd.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/api/commands/ListBrocadeVcsDevicesCmd.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/api/response/BrocadeVcsDeviceResponse.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/BrocadeVcsDeviceVO.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/BrocadeVcsNetworkVlanMappingVO.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/Constants.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsDao.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsDaoImpl.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsNetworkVlanMappingDao.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsNetworkVlanMappingDaoImpl.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/element/BrocadeVcsElementService.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
> 
>

Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin

Posted by Hugo Trippaers <ht...@schubergphilis.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23314/#review47769
-----------------------------------------------------------


This is looking good, when you addresses the comment on the system.js file in the other review i'll apply both patches. If the patches apply clean i'll run findbugs and cobertura to check for any issues and unit test coverage. 

Hugo


plugins/network-elements/brocade-vcs/resources/BrocadeInterfaceSchema.xsd
<https://reviews.apache.org/r/23314/#comment83972>

    Did you check with the rat plugin if the license is accepted like this?
    
    I think the license is only properly detected with it is located in a comment like in the spring xml files.


- Hugo Trippaers


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

Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin

Posted by Sebastien Goasguen <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23314/#review79041
-----------------------------------------------------------


Thank you for submitting your CloudStack contribution through review board. After discussion on the dev@cloudstack.apache.org the community decided to close down review board and start accepting contributiong through GitHub pull requests. We have been using GH PR for several months now and the process is better than review board.

We will keep Review Board open for another week to give you time to migrate your patch to a github PR if you wish. After that time, your patch will no longer be viewable (even though it will not be deleted).

Please consider submitting a pull request.

Great instructions are available at:
https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md

Thank you very much for your time and your contribution to Apache CloudStack, we hope that using this new process will encourage you to do more.

- Sebastien Goasguen


On July 15, 2014, 9:52 p.m., Ritu  Sabharwal wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23314/
> -----------------------------------------------------------
> 
> (Updated July 15, 2014, 9:52 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/api/commands/AddBrocadeVcsDeviceCmd.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/api/commands/DeleteBrocadeVcsDeviceCmd.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/api/commands/ListBrocadeVcsDeviceNetworksCmd.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/api/commands/ListBrocadeVcsDevicesCmd.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/api/response/BrocadeVcsDeviceResponse.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/BrocadeVcsDeviceVO.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/BrocadeVcsNetworkVlanMappingVO.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/Constants.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsDao.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsDaoImpl.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsNetworkVlanMappingDao.java PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsNetworkVlanMappingDaoImpl.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/element/BrocadeVcsElementService.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
> 
>

Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin

Posted by Ritu Sabharwal <rs...@brocade.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23314/
-----------------------------------------------------------

(Updated July 15, 2014, 9:52 p.m.)


Review request for cloudstack.


Changes
-------

Removed the configuration reading from properties files. Now the plugin configurations are read as network service provider configurations from GUI or APIs.


Bugs: CLOUDSTACK-6823
    https://issues.apache.org/jira/browse/CLOUDSTACK-6823


Repository: cloudstack-git


Description
-------

Plugin specific code.


Diffs (updated)
-----

  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/api/commands/AddBrocadeVcsDeviceCmd.java PRE-CREATION 
  plugins/network-elements/brocade-vcs/src/com/cloud/api/commands/DeleteBrocadeVcsDeviceCmd.java PRE-CREATION 
  plugins/network-elements/brocade-vcs/src/com/cloud/api/commands/ListBrocadeVcsDeviceNetworksCmd.java PRE-CREATION 
  plugins/network-elements/brocade-vcs/src/com/cloud/api/commands/ListBrocadeVcsDevicesCmd.java PRE-CREATION 
  plugins/network-elements/brocade-vcs/src/com/cloud/api/response/BrocadeVcsDeviceResponse.java PRE-CREATION 
  plugins/network-elements/brocade-vcs/src/com/cloud/network/BrocadeVcsDeviceVO.java PRE-CREATION 
  plugins/network-elements/brocade-vcs/src/com/cloud/network/BrocadeVcsNetworkVlanMappingVO.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/Constants.java PRE-CREATION 
  plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsDao.java PRE-CREATION 
  plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsDaoImpl.java PRE-CREATION 
  plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsNetworkVlanMappingDao.java PRE-CREATION 
  plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsNetworkVlanMappingDaoImpl.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/element/BrocadeVcsElementService.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

Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin

Posted by Ritu Sabharwal <rs...@brocade.com>.
-----------------------------------------------------------
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.


Changes
-------

Unit tests added.


Bugs: CLOUDSTACK-6823
    https://issues.apache.org/jira/browse/CLOUDSTACK-6823


Repository: cloudstack-git


Description
-------

Plugin specific code.


Diffs (updated)
-----

  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

Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin

Posted by Ritu Sabharwal <rs...@brocade.com>.
-----------------------------------------------------------
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.


Changes
-------

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.


Bugs: CLOUDSTACK-6823
    https://issues.apache.org/jira/browse/CLOUDSTACK-6823


Repository: cloudstack-git


Description
-------

Plugin specific code.


Diffs (updated)
-----

  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

Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin

Posted by Ritu Sabharwal <rs...@brocade.com>.

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

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


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

Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin

Posted by Ritu Sabharwal <rs...@brocade.com>.

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

Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin

Posted by Ritu Sabharwal <rs...@brocade.com>.

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

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


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

Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin

Posted by Hugo Trippaers <ht...@schubergphilis.com>.

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

Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin

Posted by Ritu Sabharwal <rs...@brocade.com>.

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

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.


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

Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin

Posted by Ritu Sabharwal <rs...@brocade.com>.

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

Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin

Posted by Hugo Trippaers <ht...@schubergphilis.com>.
-----------------------------------------------------------
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
> 
>

Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin

Posted by Ritu Sabharwal <rs...@brocade.com>.
-----------------------------------------------------------
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 (updated)
-----

  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