nacx requested changes on this pull request. Thanks, @code4clouds!
Just some general considerations apart from the existing comments: * jclouds uses a 3 space indent. Please, format the code using our [coding standards](https://cwiki.apache.org/confluence/display/JCLOUDS/Coding+Standards). * Use consistently the builder pattern in all objects. Add builders to all new model objects (I know not all objects have them, but they should; let's add them for all new domain classes). * Review all the javadocs. * Enforce immutability in all collections (list, maps, etc). * It looks like there are several classes that duplicate existing ones. Take a look at the existing ones and see if they can be reused. > +import org.jclouds.json.SerializedNames; + +import java.util.List; + +@AutoValue +public abstract class ExtensionProfile { + + /** + * The Extensions of the extension profile + */ + public abstract List<Extension> extensions(); + + + @SerializedNames({"extensions"}) + public static ExtensionProfile create(final List<Extension> extensions) { + return new AutoValue_ExtensionProfile(extensions); Enforce immutability. Also, as the list not declared nullable change to something like: ```java return new AutoValue_ExtensionProfile(extensions == null ? ImmutableList.of() : ImmutableList.copyOf(extensions)); ``` Also review the entire change set to enforce immutability in all collection properties when present. > + * 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; + +@AutoValue +public abstract class Extension { + + /** + * The autoUpgradeMinorVersion reference of the extension profile Is this javadoc accurate? > +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 storage profile + */ + @Nullable + public abstract List<String> fileUris(); + + /** + * The image reference of the storage profile Wrong javadoc? > + * 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; + +import java.util.Map; + +@AutoValue +public abstract class ExtensionProperties { + + /** + * The autoUpgradeMinorVersion reference of the extension profile Review all javadocs in this class. > + * + * 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; + +@AutoValue +public abstract class VirtualMachineScaleSetIpConfiguration { Are this class and the `VirtualMachineScaleSetIpConfigurationProperties` one different from the existing `IpConfiguration` and `IpConfigurationProperties`? > + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (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. + */ +package org.jclouds.azurecompute.arm.domain; + + import com.google.auto.value.AutoValue; [minor] Indent. > +import com.google.auto.value.AutoValue; +import org.jclouds.javax.annotation.Nullable; +import org.jclouds.json.SerializedNames; + +/** + * VirtualMachineScaleSetNetworkSecurityGroup + */ +@AutoValue +public abstract class VirtualMachineScaleSetNetworkSecurityGroup { + /** + * The name of the NetworkInterfaceConfiguration + */ + @Nullable + public abstract String id(); + + @SerializedNames({"dnsSettings"}) Wrong json mapping? > + * 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; + +/** + * VirtualMachineScaleSetNetworkSecurityGroup + */ +@AutoValue +public abstract class VirtualMachineScaleSetNetworkSecurityGroup { Can't we reuse the existing `NetworkSecurityGroup` class? > + 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); + + public abstract VirtualMachineScaleSetOSProfile build(); + } +} is this class really different from the existing `OSProfile`? If it really is, extract the common classes from the existing one so they can be reused. > + @Nullable + public abstract String name(); + + /** + * The publisher of the VirtualMachineScaleSetPlan + */ + @Nullable + public abstract String publisher(); + + /** + * The product of the VirtualMachineScaleSetPlan + */ + @Nullable + public abstract String product(); + + @SerializedNames({"name", "tier", "capacity"}) Wrong json mapping? > + * 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; + +@AutoValue +public abstract class VirtualMachineScaleSetPublicIPAddressConfiguration { Same here. Can we use the existing `PublicIpaddress` and its properties class? > + * + * 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`; > + +import javax.inject.Named; +import javax.ws.rs.Consumes; +import javax.ws.rs.GET; +import javax.ws.rs.PUT; +import javax.ws.rs.DELETE; +import javax.ws.rs.Path; +import javax.ws.rs.PathParam; +import javax.ws.rs.core.MediaType; +import java.io.Closeable; +import java.net.URI; +import java.util.List; +import java.util.Map; + +/** + * The Virtual Machine API includes operations for managing the virtual machines in your subscription. The "Virtual Machine Scale Sets API" > +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.HashMap; +import java.util.Arrays; + +import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.assertTrue; + +@Test(groups = "live", testName = "VirtualMachineScaleSetApiLiveTest") +public class VirtualMachineScaleSetApiLiveTest extends BaseAzureComputeApiLiveTest { + + private String subscriptionid; + private String vmssName; + private VirtualMachineScaleSetSKU SKU; Use lowercase for the variable name. > + VirtualMachineScaleSetDNSSettings dnsSettings = > VirtualMachineScaleSetDNSSettings.create(dnsList); + + NetworkInterfaceConfigurationProperties networkInterfaceConfigurationProperties = + NetworkInterfaceConfigurationProperties.create(true, false, networkSecurityGroup, dnsSettings, virtualMachineScaleSetIpConfigurations); + NetworkInterfaceConfiguration networkInterfaceConfiguration = NetworkInterfaceConfiguration.create("nicconfig1", networkInterfaceConfigurationProperties); + networkInterfaceConfigurations.add(networkInterfaceConfiguration); + + return VirtualMachineScaleSetNetworkProfile.create(networkInterfaceConfigurations); + } + + + private ExtensionProfile getExtensionProfile() { + List<Extension> extensions = new ArrayList<Extension>(); + + List<String> uris = new ArrayList<String>(); + uris.add("https://mystorage1.blob.core.windows.net/winvmextekfacnt/SampleCmd_1.cmd";); Is this something tied to a particular account? Will this prevent the tests from succeeding for any user? > +import org.jclouds.azurecompute.arm.domain.IdReference; +import org.jclouds.azurecompute.arm.domain.ImageReference; +import org.jclouds.azurecompute.arm.domain.IpConfiguration; +import org.jclouds.azurecompute.arm.domain.IpConfigurationProperties; +import org.jclouds.azurecompute.arm.domain.ManagedDiskParameters; +import org.jclouds.azurecompute.arm.domain.NetworkInterfaceCard; +import org.jclouds.azurecompute.arm.domain.NetworkInterfaceCardProperties; +import org.jclouds.azurecompute.arm.domain.NetworkInterfaceConfiguration; +import org.jclouds.azurecompute.arm.domain.NetworkInterfaceConfigurationProperties; +import org.jclouds.azurecompute.arm.domain.NetworkProfile; +//import org.jclouds.azurecompute.arm.domain.NetworkProfile.NetworkInterface; +import org.jclouds.azurecompute.arm.domain.OSDisk; +//import org.jclouds.azurecompute.arm.domain.OSProfile.LinuxConfiguration; +//import org.jclouds.azurecompute.arm.domain.OSProfile.WindowsConfiguration.AdditionalUnattendContent; +//import org.jclouds.azurecompute.arm.domain.OSProfile.WindowsConfiguration.WinRM.Protocol; +//import org.jclouds.azurecompute.arm.domain.Secrets.SourceVault; Remove commented code. > +//import java.net.URI; +//import java.text.DateFormat; +//import java.text.SimpleDateFormat; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +//import static com.google.common.collect.Iterables.isEmpty; +//import static org.jclouds.util.Predicates2.retry; +//import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertNotNull; + + +@Test(groups = "unit", testName = "VirtualMachineApiMockTest", singleThreaded = true) Fix test name. > + private final String resourcegroup = "myresourcegroup"; + private final String virtualNetwork = "myvirtualnetwork"; + private final String subnetName = "mysubnet"; + private final String apiVersion = "2016-04-30-preview"; + + + private VirtualMachineScaleSet getVMSS(VirtualMachineScaleSetPlan plan) { + VirtualMachineScaleSet vmss = VirtualMachineScaleSet.create("/subscriptions/SUBSCRIPTIONID/" + "" + + "resourceGroups/groupname/providers/Microsoft.Compute/VirtualMachineScaleSets/virtualmachinescaleset", "windowsmachine", + "eastus", getSKU(), null, plan, getVirtualMachineScaleSetProperties()); + return vmss; + } + + + + public VirtualMachineScaleSetSKU getSKU() { All these methods are duplicated. Make them visible in the other class and just reuse them. Also, this class has no tests. Add the corresponding mock tests. -- 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-67277176
