> On May 30, 2013, 8:56 a.m., Hugo Trippaers wrote: > > Daan, > > > > I noticed quite a few tabs in your patch. The CloudStack standard is to use > > spaces instead of tabs. Can you change the patch to fix this? > > > > Other comments below. > > > > Can you explain what tests you executed to test the existing vlan > > functionality (besides the unittests)? > >
tabs where removed (not rigorously) I will submit a new patch for this and the pvlan configuration issue below. aside from unit tests, manual testing of private gateway functionality (crud) has been performed for vlan flavor. > On May 30, 2013, 8:56 a.m., Hugo Trippaers wrote: > > api/src/com/cloud/network/Networks.java, line 94 > > <https://reviews.apache.org/r/10970/diff/4/?file=297128#file297128line94> > > > > Why not use value.getScheme() != null ? a value can be a not=null string containing no ':', but for instance only a uuid/name/number. In these cases we want to load the scheme and the value into one uri > On May 30, 2013, 8:56 a.m., Hugo Trippaers wrote: > > core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java, > > line 639 > > <https://reviews.apache.org/r/10970/diff/4/?file=297135#file297135line639> > > > > Dead code can be removed. will do > On May 30, 2013, 8:56 a.m., Hugo Trippaers wrote: > > server/src/com/cloud/api/ApiResponseHelper.java, line 1292 > > <https://reviews.apache.org/r/10970/diff/4/?file=297149#file297149line1292> > > > > Does this have anything to do anything with your changes? unused but not my code -> reapplied > On May 30, 2013, 8:56 a.m., Hugo Trippaers wrote: > > server/src/com/cloud/api/ApiResponseHelper.java, line 2132 > > <https://reviews.apache.org/r/10970/diff/4/?file=297149#file297149line2132> > > > > Why did this need to go? unused but not my code -> reapplied > On May 30, 2013, 8:56 a.m., Hugo Trippaers wrote: > > server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java, > > line 479 > > <https://reviews.apache.org/r/10970/diff/4/?file=297160#file297160line479> > > > > Did you answer this question? Why it it here? removed comment > On May 30, 2013, 8:56 a.m., Hugo Trippaers wrote: > > server/src/com/cloud/configuration/ConfigurationManagerImpl.java, line 2711 > > <https://reviews.apache.org/r/10970/diff/4/?file=297150#file297150line2711> > > > > Is this split covered in your code? created conditional on uri.scheme to allow for this split. - daan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10970/#review21191 ----------------------------------------------------------- On May 28, 2013, 9:43 a.m., daan Hoogland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10970/ > ----------------------------------------------------------- > > (Updated May 28, 2013, 9:43 a.m.) > > > Review request for cloudstack, Murali Reddy, Hugo Trippaers, and Chiradeep > Vittal. > > > Description > ------- > > converting vlan id to uri to support a broader range of networks in for > instance vpc gateway connections > > > Diffs > ----- > > api/src/com/cloud/agent/api/to/IpAddressTO.java 82c7d99 > api/src/com/cloud/agent/api/to/NetworkTO.java 3edd4c0 > api/src/com/cloud/network/NetworkService.java 59702a2 > api/src/com/cloud/network/Networks.java 5aede05 > api/src/com/cloud/network/vpc/PrivateIp.java eb68433 > api/src/com/cloud/network/vpc/StaticRouteProfile.java 54aa6e4 > api/src/com/cloud/network/vpc/VpcGateway.java 5d278e9 > api/src/com/cloud/network/vpc/VpcService.java 7a444c0 > > api/src/org/apache/cloudstack/api/command/admin/vpc/CreatePrivateGatewayCmd.java > 22dfb9e > api/src/org/apache/cloudstack/api/response/PrivateGatewayResponse.java > c5c7df5 > > core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java > 8b996d1 > engine/schema/src/com/cloud/network/vpc/VpcGatewayVO.java 7df2dfd > > plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BaremetaNetworkGuru.java > 6d14e3f > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java > b897df2 > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java > f979cfe > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/OvsVifDriver.java > eac3248 > plugins/hypervisors/ovm/src/com/cloud/ovm/hypervisor/OvmResourceBase.java > a626e31 > > plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java > 630d1b4 > > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java > 4680fde > > plugins/network-elements/cisco-vnmc/test/com/cloud/network/element/CiscoVnmcElementTest.java > a16733b > > plugins/network-elements/f5/src/com/cloud/network/resource/F5BigIpResource.java > 1733712 > > plugins/network-elements/juniper-srx/src/com/cloud/network/resource/JuniperSrxResource.java > fd065d5 > > plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java > c0d4599 > > plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsTunnelManagerImpl.java > b1ecaac > server/src/com/cloud/api/ApiResponseHelper.java 89739cf > server/src/com/cloud/configuration/ConfigurationManagerImpl.java 214e292 > server/src/com/cloud/network/ExternalFirewallDeviceManagerImpl.java 9d24e47 > server/src/com/cloud/network/ExternalLoadBalancerDeviceManagerImpl.java > f93bf7a > server/src/com/cloud/network/ExternalLoadBalancerUsageManagerImpl.java > 2c8031c > server/src/com/cloud/network/NetworkManager.java 05bc26e > server/src/com/cloud/network/NetworkManagerImpl.java 254510b > server/src/com/cloud/network/NetworkServiceImpl.java 1533ca9 > server/src/com/cloud/network/guru/DirectPodBasedNetworkGuru.java cf27986 > server/src/com/cloud/network/guru/PrivateNetworkGuru.java 2e266e7 > server/src/com/cloud/network/guru/PublicNetworkGuru.java a83cdb3 > > server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java > 9992b7c > server/src/com/cloud/network/vpc/PrivateGatewayProfile.java d6480cd > server/src/com/cloud/network/vpc/PrivateIpAddress.java 2f3cf53 > server/src/com/cloud/network/vpc/VpcManagerImpl.java 1aab732 > server/test/com/cloud/network/CreatePrivateNetworkTest.java PRE-CREATION > server/test/com/cloud/network/MockNetworkManagerImpl.java e5d34fb > server/test/com/cloud/vpc/MockNetworkManagerImpl.java 7129273 > server/test/com/cloud/vpc/MockVpcManagerImpl.java 921321f > setup/db/db/schema-410to420.sql 1c9a8c1 > > Diff: https://reviews.apache.org/r/10970/diff/ > > > Testing > ------- > > the NetworkTO is tested to accept uris with several initial states. > createPrivateNetwork in NetworkServiceImpl is tested (to accept only vlan or > lswitch based networks for now) > test code that used to use 'vlan://#' now uses 'vlan:#' > > > Thanks, > > daan Hoogland > >
