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-1278: Allow to configure virtual machine NICs in Azure ARM (#386)
Date Wed, 26 Apr 2017 21:10:50 GMT
andreaturli commented on this pull request.



> @@ -383,39 +391,94 @@ private OSProfile createOsProfile(String computerName, Template
template) {
       return builder.build();
    }
 
-   private NetworkInterfaceCard createNetworkInterfaceCard(String subnetId, String name,
String locationName,
-                                                           String azureGroup, TemplateOptions
options) {
-      final PublicIPAddressApi ipApi = api.getPublicIPAddressApi(azureGroup);
+   private List<NetworkInterfaceCard> createNICs(String nodeName, String location,
AzureTemplateOptions options) {

[minor] better `createNetworkInterfaceCards()` ?

>  
-      PublicIPAddressProperties properties = PublicIPAddressProperties.builder().publicIPAllocationMethod("Static")
-              .idleTimeoutInMinutes(4).build();
+      for (IpOptions ipConfig : publicIpsFirst(options.getIpOptions())) {

maybe this logic deserves to live in a private method? or a Function name `ipOptionsTo IpConfiguration`
maybe?

>        }
 
-      String networkInterfaceCardName = "jc-nic-" + name;
-      return api.getNetworkInterfaceCardApi(azureGroup).createOrUpdate(networkInterfaceCardName,
locationName,
-              networkInterfaceCardProperties.build(), ImmutableMap.of("jclouds", name));
+      return nics.build();
+   }
+   
+   private NetworkProfile createNetworkProfile(List<NetworkInterfaceCard> nics) {
+      List<NetworkInterface> nicAttachments = new ArrayList<NetworkInterface>(nics.size());
+      for (int i = 0; i < nics.size(); i++) {
+         nicAttachments.add(NetworkInterface.create(nics.get(i).id(), NetworkInterfaceProperties.create(i
== 0)));

maybe a javadoc to explain the method as it is not obvious, especially the `i == 0` part

>        }
 
-      String networkInterfaceCardName = "jc-nic-" + name;
-      return api.getNetworkInterfaceCardApi(azureGroup).createOrUpdate(networkInterfaceCardName,
locationName,
-              networkInterfaceCardProperties.build(), ImmutableMap.of("jclouds", name));
+      return nics.build();
+   }
+   
+   private NetworkProfile createNetworkProfile(List<NetworkInterfaceCard> nics) {
+      List<NetworkInterface> nicAttachments = new ArrayList<NetworkInterface>(nics.size());
+      for (int i = 0; i < nics.size(); i++) {
+         nicAttachments.add(NetworkInterface.create(nics.get(i).id(), NetworkInterfaceProperties.create(i
== 0)));
+      }
+      return NetworkProfile.create(nicAttachments);
+   }
+   
+   private static List<IpOptions> publicIpsFirst(List<IpOptions> ipOptions) {

javadoc maybe to explain the reason why you are sorting this list?

>        List<NetworkSecurityGroup> networkGroups = new ArrayList<NetworkSecurityGroup>();
 
-      for (IdReference networkInterfaceCardIdReference : networkInterfacesIdReferences) {
-         String nicName = networkInterfaceCardIdReference.name();
-         String nicResourceGroup = networkInterfaceCardIdReference.resourceGroup();
+      for (NetworkInterface networkInterfaceCardIdReference : networkInterfacesIdReferences)
{

maybe `networkInterfaceCardIdReference` is now `networkInterface` ?

> @@ -135,12 +137,12 @@ public boolean apply(SecurityGroup input) {
       if (vm == null) {
          throw new IllegalArgumentException("Node " + nodeId + " was not found");
       }
-      List<IdReference> networkInterfacesIdReferences = vm.properties().networkProfile().networkInterfaces();
+      List<NetworkInterface> networkInterfacesIdReferences = vm.properties().networkProfile().networkInterfaces();

[minor] is maybe `networkInterfaces` rather than `networkInterfacesIdReferences` ?

> @@ -136,9 +138,9 @@ public NodeMetadata apply(VirtualMachine virtualMachine) {
       return builder.build();
    }
 
-   private Iterable<String> getPrivateIpAddresses(List<IdReference> idReferences)
{
+   private Iterable<String> getPrivateIpAddresses(List<NetworkInterface> idReferences)
{

[minor] `networkInterfaces` instead of `idReferences` ?

>     private AvailabilitySet availabilitySet;
    private String availabilitySetName;
    private List<DataDisk> dataDisks = ImmutableList.of();
    private String resourceGroup;
-
-   /**
-    * Sets the virtual network name
-    */
-   public  AzureTemplateOptions virtualNetworkName(String virtualNetworkName) {

do we need to deprecate them first or because it is still in labs we can change the public
api this way?

> @@ -86,10 +89,10 @@ public boolean cleanupNode(final String id) {
    }
 
    public void cleanupVirtualMachineNICs(VirtualMachine virtualMachine) {
-      for (IdReference nicRef : virtualMachine.properties().networkProfile().networkInterfaces())
{
-         String nicResourceGroup = nicRef.resourceGroup();
-         String nicName = nicRef.name();
-         NetworkInterfaceCard nic = api.getNetworkInterfaceCardApi(nicRef.resourceGroup()).get(nicName);
+      for (NetworkInterface nicRef : virtualMachine.properties().networkProfile().networkInterfaces())
{

just `nic` instead of `nicRef` ?


> @@ -196,4 +197,35 @@ private void createResourceGroupIfNeeded(String group, String location,
AzureTem
                ImmutableMap.of("description", "jclouds default resource group"));
       }
    }
+   
+   private void normalizeNetworkOptions(AzureTemplateOptions options) {
+      if (!options.getNetworks().isEmpty() && !options.getIpOptions().isEmpty())
{
+         throw new IllegalArgumentException(
+               "The options.networks and options.ipOptions are exclusive. Just configure
the networking options once.");
+      }
+      
+      if (!options.getNetworks().isEmpty() && options.getIpOptions().isEmpty()) {
+         // The portable interface allows to configure network IDs (subnet IDs),

move this comment to javadoc maybe?

> @@ -196,4 +197,35 @@ private void createResourceGroupIfNeeded(String group, String location,
AzureTem
                ImmutableMap.of("description", "jclouds default resource group"));
       }
    }
+   
+   private void normalizeNetworkOptions(AzureTemplateOptions options) {
+      if (!options.getNetworks().isEmpty() && !options.getIpOptions().isEmpty())
{
+         throw new IllegalArgumentException(
+               "The options.networks and options.ipOptions are exclusive. Just configure
the networking options once.");
+      }
+      
+      if (!options.getNetworks().isEmpty() && options.getIpOptions().isEmpty()) {
+         // The portable interface allows to configure network IDs (subnet IDs),

we need a good testing coverage for this method :D

> @@ -29,6 +31,8 @@
 @AutoValue
 public abstract class Subnet {
 
+   private static final Pattern NETWORK_PATTERN = Pattern.compile("^.*/virtualNetworks/([^/]+)(/.*)?$");

maybe `VIRTUAL_NETWORK_PATTERN` ?

-- 
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/386#pullrequestreview-34960288
Mime
View raw message