nacx requested changes on this pull request.

Thanks for the fixes @code4clouds!
Still some comments to address.
Also, in general, remove the `@Nullable` annotation from all the collection 
fields where we enforce presence and immutability. The annotation should only 
be present where users can expect a null value.

> +#    (the "License"); you may not use this file except in compliance with
+#    the License.  You may obtain a copy of the License at
+#
+#        http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS,
+#    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#    See the License for the specific language governing permissions and
+#    limitations under the License.
+#
+#-->
+[*.java]
+indent_style = space
+indent_size = 3
+max_line_length = 120

I didn't know about editorconfig, but looks like a convenient idea. However, we 
should discuss the right location for this file (the root of the repo, 
somewhere in the main jclouds repo, etc) so we don't duplicate it in every 
project.
It would be better to move it to a separate PR (a PR to the main jclouds repo) 
and discuss its location and contents in the context of 
[JCLOUDS-1346](https://issues.apache.org/jira/browse/JCLOUDS-1346). If you 
agree, could you remove the file from this PR?

> +    /**
+     * The name reference of the extension profile
+     */
+    public abstract String name();
+
+    /**
+     * The properties reference of the extension profile
+     */
+    public abstract ExtensionProperties properties();
+
+
+    @SerializedNames({"name", "properties"})
+    public static Extension create(final String name, final 
ExtensionProperties properties) {
+        return new AutoValue_Extension(name, properties);
+    }
+}

Use 3 space indent.

> +import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import java.util.List;
+
+@AutoValue
+public abstract class ExtensionProfileSettings {
+
+   /**
+    * The fileUris reference of the extension profile settings
+    */
+   @Nullable
+   public abstract List<String> fileUris();
+
+   /**
+    * The commandToExecute of the storage profile settings

"extension profile"

> + *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.azurecompute.arm.domain;
+
+import com.google.auto.value.AutoValue;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+/**
+ * SKU

Fix or remove this javadoc.

> + * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.azurecompute.arm.domain;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import java.util.List;
+
+/**
+ * NetworkInterfaceConfigurationProperty

Remove this javadoc.

> +   public abstract Boolean primary();
+
+   /**
+    * The enableAcceleratedNetworking of the 
NetworkInterfaceConfigurationProperties
+    */
+   @Nullable
+   public abstract Boolean enableAcceleratedNetworking();
+
+   /**
+    * The networkSecurityGroup of the NetworkInterfaceConfigurationProperties
+    */
+   @Nullable
+   public abstract VirtualMachineScaleSetNetworkSecurityGroup 
networkSecurityGroup();
+
+   /**
+    * The dnsSettings of the VirtualMachineScaleSetDNSSettings

Of the "NetworkInterfaceConfigurationProperties"?

> @@ -51,6 +51,7 @@ public static NetworkInterfaceCard create(final String name,
                                              final String location,
                                              final 
NetworkInterfaceCardProperties properties,
                                              final Map<String, String> tags) {
-      return new AutoValue_NetworkInterfaceCard(name, id, etag, location, 
properties, tags == null ? null : ImmutableMap.copyOf(tags));
+      return new AutoValue_NetworkInterfaceCard(name, id, etag, location, 
properties,
+         tags == null ? ImmutableMap.<String, String>of() : 
ImmutableMap.copyOf(tags));

I think we shouldn't change how the `tags` fields are configured. It is OK to 
have them `null`(although it is not the default practice). See: 
https://github.com/jclouds/jclouds-labs/pull/262#r60811082
Could you please revert the changes made to the already existing `tags`fields?

> +import com.google.auto.value.AutoValue;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+/**
+ * Virtual Machine Scale Set Network Security Group
+ */
+@AutoValue
+public abstract class VirtualMachineScaleSetNetworkSecurityGroup {
+   /**
+    * The id of the Virtual Machine Scale Set Network Security Group
+    */
+   @Nullable
+   public abstract String id();
+
+   @SerializedNames({"dnsSettings"})

Is this correct? Is `dnsSettings` the name of the field the war json that maps 
to the `id` property?

> +      public abstract List<AdditionalUnattendContent> 
> additionalUnattendContent();
+
+      /**
+       * is automatic updates enabled
+       */
+      public abstract boolean enableAutomaticUpdates();
+
+      @SerializedNames({"provisionVMAgent", "winRM", 
"additionalUnattendContent", "enableAutomaticUpdates"})
+      public static WindowsConfiguration create(final boolean 
provisionVMAgent, final WinRM winRM,
+                                                final 
List<AdditionalUnattendContent> additionalUnattendContent,
+                                                final boolean 
enableAutomaticUpdates) {
+
+         return new 
AutoValue_VirtualMachineScaleSetOSProfile_WindowsConfiguration(provisionVMAgent,
 winRM,
+            additionalUnattendContent, enableAutomaticUpdates);
+      }
+   }

Can't these configuration classes be extracted and reused? They look quite 
similar to the existing ones.

> +         public static AdditionalUnattendContent create(final String pass, 
> final String component,
+                                                        final String 
settingName,
+                                                        final String content) {
+
+            return new 
AutoValue_VirtualMachineScaleSetOSProfile_WindowsConfiguration_AdditionalUnattendContent(
+               pass, component, settingName, content);
+         }
+      }
+
+      /**
+       * The provision VM Agent true of false.
+       */
+      public abstract boolean provisionVMAgent();
+
+      /**
+       * winR

[typo] winRM

> +      return new AutoValue_VirtualMachineScaleSetOSProfile.Builder();
+   }
+
+   @AutoValue.Builder
+   public abstract static class Builder {
+      public abstract Builder computerNamePrefix(String computerNamePrefix);
+
+      public abstract Builder adminUsername(String adminUsername);
+
+      public abstract Builder adminPassword(String adminPassword);
+
+      public abstract Builder linuxConfiguration(LinuxConfiguration 
linuxConfiguration);
+
+      public abstract Builder windowsConfiguration(WindowsConfiguration 
windowsConfiguration);
+
+      public abstract Builder secrets(List<Secrets> secrets);

Enforce an immutable collection

> +   @Nullable
+   public abstract String name();
+
+   /**
+    * The publisher of the Virtual Machine Scale Set Plan
+    */
+   @Nullable
+   public abstract String publisher();
+
+   /**
+    * The product of the Virtual Machine Scale Set Plan
+    */
+   @Nullable
+   public abstract String product();
+
+   @SerializedNames({"name", "tier", "capacity"})

Wrong json mapping?

> +
+   @AutoValue.Builder
+   public abstract static class Builder {
+      public abstract Builder id(String id);
+      public abstract Builder name(String name);
+      public abstract Builder location(String location);
+      public abstract Builder sku(VirtualMachineScaleSetSKU sku);
+      public abstract Builder tags(Map<String, String> tags);
+      public abstract Builder plan(VirtualMachineScaleSetPlan plan);
+      public abstract Builder properties(VirtualMachineScaleSetProperties 
properties);
+
+      abstract Map<String, String> tags();
+      abstract VirtualMachineScaleSet autoBuild();
+
+      public VirtualMachineScaleSet build() {
+         tags(tags() == null ? ImmutableMap.<String, String>of() : 
ImmutableMap.copyOf(tags()));

According to my previous comment, keep tags null if null, otherwise, make them 
immutable.

> + *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.azurecompute.arm.domain;
+
+import com.google.auto.value.AutoValue;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+/**
+ * SKU

Remove

> + *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.azurecompute.arm.domain;
+
+import com.google.auto.value.AutoValue;
+import org.jclouds.json.SerializedNames;
+
+/**
+ * SKU

Remove

> + *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.azurecompute.arm.domain;
+
+import com.google.auto.value.AutoValue;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+/**
+ * SKU

Remove

> + *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.azurecompute.arm.domain;
+
+import org.jclouds.azurecompute.arm.util.GetEnumValue;
+
+public class VirtualMachineScaleSetUpgradeMode {
+   public enum Status {
+      Manual,
+      Automatic,
+      UNRECOGNIZED;

Be consistent with style and use `Unrecognized`;

> + * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.azurecompute.arm.domain;
+
+import com.google.auto.value.AutoValue;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+/**
+ * SKU
+ */
+@AutoValue
+public abstract class VirtualMachineScaleSetUpgradePolicy {
+   /**
+    * The name of the Virtual Machine Scale Set Upgrade Policy

"mode"

> +
+   private Subnet createDefaultSubnet()  {
+      server.enqueue(jsonResponse("/getonesubnet.json").setResponseCode(200));
+      final SubnetApi subnetApi = api.getSubnetApi("azurearmtesting", 
"myvirtualnetwork");
+      Subnet subnet = subnetApi.get("mysubnet");
+      String path = 
String.format("/subscriptions/%s/resourcegroups/%s/providers/Microsoft.Network/virtualNetworks/%s/subnets/%s?%s",
 subscriptionid, resourcegroup, virtualNetwork, subnetName, apiVersion);
+      try {
+         assertSent(server, "GET", path);
+      } catch (InterruptedException e) {
+         e.printStackTrace();
+      }
+      return subnet;
+   }
+
+
+}

Tests still missing here.

-- 
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/412#pullrequestreview-67919642

Reply via email to