You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Bhathiya <no...@github.com> on 2015/05/22 00:31:09 UTC
[jclouds-labs] JCLOUDS-853: Improve Create VirtualMachine Deployment
(#175)
Conflicts:
azurecompute/src/test/java/org/jclouds/azurecompute/features/VirtualMachineApiLiveTest.java
Fix Checkstyle Violations
Address live test failiures
Fix Checkstyle Violations
minor changes:
fix live and mocktests
minor changes
Code style changes
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/175
-- Commit Summary --
* JCLOUDS-853: Improve Create VirtualMachine Deployment
-- File Changes --
M azurecompute/pom.xml (4)
M azurecompute/src/main/java/org/jclouds/azurecompute/binders/DeploymentParamsToXML.java (242)
M azurecompute/src/main/java/org/jclouds/azurecompute/binders/RoleToXML.java (5)
M azurecompute/src/main/java/org/jclouds/azurecompute/compute/AzureComputeServiceAdapter.java (73)
A azurecompute/src/main/java/org/jclouds/azurecompute/domain/DataVirtualHardDiskParam.java (156)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/DeploymentParams.java (193)
A azurecompute/src/main/java/org/jclouds/azurecompute/domain/LinuxConfigurationSetParams.java (138)
A azurecompute/src/main/java/org/jclouds/azurecompute/domain/OSVirtualHardDiskParam.java (175)
A azurecompute/src/main/java/org/jclouds/azurecompute/domain/RoleParam.java (194)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/Rule.java (2)
A azurecompute/src/main/java/org/jclouds/azurecompute/domain/WindowsConfigurationSetParams.java (150)
M azurecompute/src/main/java/org/jclouds/azurecompute/util/ConflictManagementPredicate.java (2)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/RuleHandler.java (2)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/StorageServiceKeysHandler.java (2)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/DeploymentApiLiveTest.java (44)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/DeploymentApiMockTest.java (68)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/VMImageApiLiveTest.java (50)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/VirtualMachineApiLiveTest.java (102)
M azurecompute/src/test/java/org/jclouds/azurecompute/internal/AbstractAzureComputeApiLiveTest.java (2)
A azurecompute/src/test/resources/newdeploymentparams-linux.xml (1)
A azurecompute/src/test/resources/newdeploymentparams-windows.xml (1)
M azurecompute/src/test/resources/profiledefinitiontmparams.xml (6)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/175.patch
https://github.com/jclouds/jclouds-labs/pull/175.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/175
Re: [jclouds-labs] JCLOUDS-853: Improve Create VirtualMachine
Deployment (#175)
Posted by Ignasi Barrera <no...@github.com>.
> + .remoteSourceImageLink(in.remoteSourceImageLink()).resizedSizeInGB(in.resizedSizeInGB());
> + }
> +
> + public OSVirtualHardDiskParam build() {
> + return OSVirtualHardDiskParam
> + .create(hostCaching, diskLabel, diskName, mediaLink, sourceImageName, os, remoteSourceImageLink,
> + resizedSizeInGB);
> + }
> +
> + }
> +
> + public static OSVirtualHardDiskParam create(String hostCaching, String diskLabel, String diskName, URI mediaLink,
> + String sourceImageName, OSImage.Type os, URI remoteSourceImageLink, Integer resizedSizeInGB) {
> + return new AutoValue_OSVirtualHardDiskParam(hostCaching, diskLabel, diskName, mediaLink, sourceImageName, os,
> + remoteSourceImageLink, resizedSizeInGB);
> + }
Same comment as before. All nullables?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/175/files#r31916990
Re: [jclouds-labs] JCLOUDS-853: Improve Create VirtualMachine
Deployment (#175)
Posted by Ignasi Barrera <no...@github.com>.
> + public abstract String path();
> +
> + public static KeyPair create(String fingerPrint, String path) {
> + return new AutoValue_LinuxConfigurationSetParams_KeyPair(fingerPrint, path);
> + }
> + }
> +
> + public abstract String configurationSetType();
> +
> + @Nullable public abstract String hostName();
> +
> + public abstract String userName();
> +
> + public abstract String userPassword();
> +
> + public abstract Boolean disableSshPasswordAuthentication();
Use the primitive type for non-nullable values.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/175/files#r31916899
Re: [jclouds-labs] JCLOUDS-853: Improve Create VirtualMachine
Deployment (#175)
Posted by Ignasi Barrera <no...@github.com>.
> + /**
> + * If the disk that is being added is already registered in the subscription or the VHD for the disk does not exist
> + * in blob storage, this element is ignored. If the VHD file exists in blob storage, this element defines the path to
> + * the VHD and a disk is registered from it and attached to the virtual machine.
> + */
> + @Nullable public abstract URI sourceMediaLink();
> +
> + public Builder toBuilder() {
> + return builder().fromDataVirtualHardDiskParams(this);
> + }
> +
> + public static Builder builder() {
> + return new Builder();
> + }
> +
> + public static final class Builder {
The google auto version we use now supports builders. We should try to move to using them.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/175/files#r31916189
Re: [jclouds/jclouds-labs] JCLOUDS-853: Improve Create VirtualMachine
Deployment (#175)
Posted by Andrew Gaul <no...@github.com>.
Closed #175.
---
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/175#event-699641490
Re: [jclouds/jclouds-labs] JCLOUDS-853: Improve Create VirtualMachine
Deployment (#175)
Posted by Andrew Gaul <no...@github.com>.
Closing this given the work in #279.
---
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/175#issuecomment-227566506
Re: [jclouds-labs] JCLOUDS-853: Improve Create VirtualMachine
Deployment (#175)
Posted by Bhathiya <no...@github.com>.
@nacx
can you review this? . I am working on merging https://github.com/jclouds/jclouds-labs/pull/157 now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/175#issuecomment-104677773
Re: [jclouds-labs] JCLOUDS-853: Improve Create VirtualMachine
Deployment (#175)
Posted by Ignasi Barrera <no...@github.com>.
>
> - /**
> - * Optional. Specifies the name of a reserved IP address that is to be assigned to the deployment. You must run
> - * Create Reserved IP Address before you can assign the address to the deployment using this element.
> - *
> - * The ReservedIPName element is only available using version 2014-05-01 or higher.
> - *
> - * @return reserved IP.
> - */
> - @SinceApiVersion("2014-05-01")
> - @Nullable
> - public abstract String reservedIPName();
> -
> - public abstract List<String> subnetNames();
> + @Nullable public abstract String reservedIpName();
Why not keeping the `@SinceApiVersion` annotation?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/175/files#r31916520
Re: [jclouds-labs] JCLOUDS-853: Improve Create VirtualMachine
Deployment (#175)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public abstract String domain();
> +
> + public abstract String userName();
> +
> + public abstract String password();
> +
> + public static Credential create(String domain, String userName, String password) {
> + return new AutoValue_WindowsConfigurationSetParams_Credential(domain, userName, password);
> + }
> + }
> +
> + @AutoValue
> + public abstract static class DomainJoin {
> +
> + @Nullable public abstract List<Credential> credentials();
We should avoid returning null collections. Prefer returning empty ones.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/175/files#r31917100
Re: [jclouds-labs] JCLOUDS-853: Improve Create VirtualMachine
Deployment (#175)
Posted by Ignasi Barrera <no...@github.com>.
> + this.sourceMediaLink = sourceMediaLink;
> + return this;
> + }
> +
> + public Builder fromDataVirtualHardDiskParams(DataVirtualHardDiskParam in) {
> + return hostCaching(in.hostCaching()).diskLabel(in.diskLabel()).diskName(in.diskName()).LUN(in.LUN())
> + .logicalDiskSizeInGB(in.logicalDiskSizeInGB()).mediaLink(in.mediaLink())
> + .sourceMediaLink(in.sourceMediaLink());
> + }
> +
> + }
> +
> + public static DataVirtualHardDiskParam create(String hostCaching, String diskLabel, String diskName, Integer lun,
> + Integer logicalDiskSizeInGB, URI mediaLink, URI sourceMediaLink) {
> + return new AutoValue_DataVirtualHardDiskParam(hostCaching, diskLabel, diskName, lun, logicalDiskSizeInGB,
> + mediaLink, sourceMediaLink);
All values are nullable? When is it possible to have such an object with no values?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/175/files#r31916272
Re: [jclouds-labs] JCLOUDS-853: Improve Create VirtualMachine
Deployment (#175)
Posted by Ignasi Barrera <no...@github.com>.
> - * Specifies the associated password for the user name. Passwords are ASCII character strings 6 to 72 characters in
> - * length.
> - */
> - public abstract String password();
> -
> - /**
> - * {@link OSImage#name() name} of the user or platform image.
> - */
> - public abstract String sourceImageName();
> -
> - /**
> - * Indicates the {@link OSImage#mediaLink() location} when {@link #sourceImageName() source} is a platform image.
> - */
> - public abstract URI mediaLink();
> + // TODO move to RoleParams/NetworkConfigurationParams
> + @Nullable public abstract List<ExternalEndpoint> externalEndpoints();
This wasn't nullable before. Has this really changed?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/175/files#r31916428
Re: [jclouds-labs] JCLOUDS-853: Improve Create VirtualMachine
Deployment (#175)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + @AutoValue
> + public abstract static class PublicKey {
> +
> + public abstract String fingerPrint();
> +
> + public abstract String path();
> +
> + public static PublicKey create(String fingerPrint, String path) {
> + return new AutoValue_LinuxConfigurationSetParams_PublicKey(fingerPrint, path);
> + }
> + }
> +
> + @AutoValue
> + public abstract static class KeyPair {
Why these two different classes to hold the same information?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/175/files#r31916644
Re: [jclouds-labs] JCLOUDS-853: Improve Create VirtualMachine
Deployment (#175)
Posted by Ignasi Barrera <no...@github.com>.
> + public static DomainJoin create(List<Credential> credentials, String joinDomain) {
> + return new AutoValue_WindowsConfigurationSetParams_DomainJoin(credentials, joinDomain);
> + }
> + }
> +
> + public abstract String configurationSetType();
> +
> + @Nullable public abstract String computerName();
> +
> + @Nullable public abstract String adminPassword();
> +
> + @Nullable public abstract DomainJoin domainJoin();
> +
> + @Nullable public abstract String adminUserName();
> +
> + @Nullable public abstract List<CertificateSetting> storedCertificateSettings();
Same comment as before. Avoid null collections when possible.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/175/files#r31917120
Re: [jclouds-labs] JCLOUDS-853: Improve Create VirtualMachine
Deployment (#175)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + final RoleParam roleParam = RoleParam.builder()
> + .roleName(name)
> + .roleSize(RoleSize.Type.fromString(template.getHardware().getName()))
> + .osVirtualHardDiskParam(osParam)
> + .windowsConfigurationSet(winConfig)
> + .build();
> +
> + params = DeploymentParams.builder()
> + .name(name).roleParam(roleParam)
> + .virtualNetworkName(virtualNetworkName)
> + .externalEndpoints(externalEndpoints)
> + .subnetName(subnetName)
> + .reservedIpName(reservedIPAddress)
> + .build();
> + }
All code here is duplicated. Extract it all out of the conditionals and put only the configuration set in it. You're using builders, so there's no problem on doing that.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/175/files#r31916088