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-1231: Implement the SecurityGroupExtension in ARM (#347)
Date Thu, 26 Jan 2017 16:08:07 GMT
andreaturli commented on this pull request.

lgtm, some minor comments

> @@ -114,7 +103,7 @@ public static Properties defaultProperties() {
       properties.put(API_VERSION_PREFIX + PublicIPAddressApi.class.getSimpleName(), "2015-06-15");
       properties.put(API_VERSION_PREFIX + ResourceGroupApi.class.getSimpleName(), "2015-01-01");
       properties.put(API_VERSION_PREFIX + ResourceProviderApi.class.getSimpleName(), "2015-01-01");
-      properties.put(API_VERSION_PREFIX + StorageAccountApi.class.getSimpleName(), STORAGE_API_VERSION);
+      properties.put(API_VERSION_PREFIX + StorageAccountApi.class.getSimpleName(), "2015-06-15");

we should decide at some point if we want to use the latest version of all of them or just
use them randomly :D

>     }
 
-   public void deleteResourceGroupIfEmpty(String group) {
-      if (api.getVirtualMachineApi(group).list().isEmpty() 
-            && api.getStorageAccountApi(group).list().isEmpty()
+   public boolean deleteResourceGroupIfEmpty(String group) {
+      boolean deleted = false;
+      if (api.getVirtualMachineApi(group).list().isEmpty() && api.getStorageAccountApi(group).list().isEmpty()

those api calls are really expensive, I'll try to replace them in a subsequent PR

>        RegionAndId regionAndId = RegionAndId.fromSlashEncoded(id);
-      String group = locationToResourceGroupName.apply(regionAndId.region());
-      
+      ResourceGroup resourceGroup = resourceGroupMap.getUnchecked(regionAndId.region());
+      String group = resourceGroup.name();

[minor] could you name it `resourceGroupName`

>        final RegionAndId regionAndId = RegionAndId.fromSlashEncoded(cloneTemplate.getSourceNodeId());
-      final String group = locationToResourceGroupName.apply(regionAndId.region());
+      ResourceGroup resourceGroup = resourceGroupMap.getUnchecked(regionAndId.region());
+      final String group = resourceGroup.name();

[minor] maybe `resourceGroupName`?

> +   AzureComputeSecurityGroupExtension(AzureComputeApi api, @Memoized Supplier<Set<?
extends Location>> locations,
+         Function<NetworkSecurityGroup, SecurityGroup> groupConverter,
+         SecurityGroupAvailablePredicateFactory securityRuleAvailable,
+         @Named(TIMEOUT_RESOURCE_DELETED) Predicate<URI> resourceDeleted,
+         LoadingCache<String, ResourceGroup> resourceGroupMap) {
+      this.api = api;
+      this.locations = locations;
+      this.securityGroupConverter = groupConverter;
+      this.securityGroupAvailable = securityRuleAvailable;
+      this.resourceDeleted = resourceDeleted;
+      this.resourceGroupMap = resourceGroupMap;
+   }
+
+   @Override
+   public Set<SecurityGroup> listSecurityGroups() {
+      return ImmutableSet.copyOf(concat(transform(locations.get(), new Function<Location,
Set<SecurityGroup>>() {

why immutableSet?

> +      VirtualMachine vm = api.getVirtualMachineApi(resourceGroup.name()).get(regionAndId.id());
+      List<IdReference> networkInterfacesIdReferences = vm.properties().networkProfile().networkInterfaces();
+      List<NetworkSecurityGroup> networkGroups = new ArrayList<NetworkSecurityGroup>();
+
+      for (IdReference networkInterfaceCardIdReference : networkInterfacesIdReferences) {
+         String nicName = Iterables.getLast(Splitter.on("/").split(networkInterfaceCardIdReference.id()));
+         NetworkInterfaceCard card = api.getNetworkInterfaceCardApi(resourceGroup.name()).get(nicName);
+         if (card != null && card.properties().networkSecurityGroup() != null) {
+            String secGroupName = Iterables.getLast(Splitter.on("/").split(
+                  card.properties().networkSecurityGroup().id()));
+            NetworkSecurityGroup group = api.getNetworkSecurityGroupApi(resourceGroup.name()).get(secGroupName);
+            networkGroups.add(group);
+         }
+      }
+
+      return ImmutableSet.copyOf(transform(filter(networkGroups, notNull()), securityGroupConverter));

why immutableSet?

> +      final RegionAndId regionAndId = RegionAndId.fromSlashEncoded(group.getId());
+      ResourceGroup resourceGroup = resourceGroupMap.getUnchecked(regionAndId.region());
+
+      NetworkSecurityGroupApi groupApi = api.getNetworkSecurityGroupApi(resourceGroup.name());
+      NetworkSecurityGroup networkSecurityGroup = groupApi.get(regionAndId.id());
+
+      if (networkSecurityGroup == null) {
+         throw new IllegalArgumentException("Security group " + group.getName() + " was not
found");
+      }
+
+      NetworkSecurityRuleApi ruleApi = api.getNetworkSecurityRuleApi(resourceGroup.name(),
networkSecurityGroup.name());
+      int nextPriority = getRuleStartingPriority(networkSecurityGroup);
+
+      for (String ipRange : ipRanges) {
+         NetworkSecurityRuleProperties properties = NetworkSecurityRuleProperties.builder()
+               .protocol(Protocol.fromValue(protocol.name())) //

please remove `//`

-- 
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/347#pullrequestreview-18640274
Mime
View raw message