jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrea Turli <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] JCLOUDS-1273: Support multiple resource groups in ARM (#385)
Date Mon, 24 Apr 2017 19:51:32 GMT
andreaturli commented on this pull request.

this is very useful, @nacx !

Thanks, some minor initial comments.

I'll post the result of tests asap as well

>  
-      getOrCreateVirtualNetworkWithSubnet(location, options, azureGroupName);
-      configureSecurityGroupForOptions(group, azureGroupName, template.getLocation(), options);
+      createResourceGroupIfNeeded(group, location, options);

as you are refactoring it, wouldn't be better to avoid function with side-effects (jclouds
doesn't have many of them) and set the `ResourceGroup` on `options` ? 

>  
-      getOrCreateVirtualNetworkWithSubnet(location, options, azureGroupName);
-      configureSecurityGroupForOptions(group, azureGroupName, template.getLocation(), options);
+      createResourceGroupIfNeeded(group, location, options);
+      getOrCreateVirtualNetworkWithSubnet(location, options);

same as above

>  
-      getOrCreateVirtualNetworkWithSubnet(location, options, azureGroupName);
-      configureSecurityGroupForOptions(group, azureGroupName, template.getLocation(), options);
+      createResourceGroupIfNeeded(group, location, options);
+      getOrCreateVirtualNetworkWithSubnet(location, options);
+      configureSecurityGroupForOptions(group, template.getLocation(), options);

same as above

>  
    public AzureComputeServiceLiveTest() {
       provider = "azurecompute-arm";
+      resourceGroupName = getClass().getSimpleName().toLowerCase();

+1

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/385#pullrequestreview-34398787
Mime
View raw message