You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Adrian Cole <no...@github.com> on 2014/10/15 17:14:59 UTC
[jclouds-labs] Cleanup round 1 of azurecompute Deployment class and
imports. (#92)
did a global import order fix so that all following changes don't have this.
Main change was to consolidate the Deployment value type. A follow-up PR will take out the private fields, equals, etc. with auto-value.
You can merge this Pull Request by running:
git pull https://github.com/adriancole/jclouds-labs adrian.azurecompute-Deployment-cleanup1
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs/pull/92
-- Commit Summary --
* Cleanup round 1 of azurecompute Deployment class and imports.
-- File Changes --
M azurecompute/src/main/java/org/jclouds/azurecompute/AzureComputeApi.java (2)
M azurecompute/src/main/java/org/jclouds/azurecompute/AzureComputeProviderMetadata.java (5)
M azurecompute/src/main/java/org/jclouds/azurecompute/AzureManagementApiMetadata.java (8)
M azurecompute/src/main/java/org/jclouds/azurecompute/binders/BindCreateHostedServiceToXmlPayload.java (15)
M azurecompute/src/main/java/org/jclouds/azurecompute/binders/BindDeploymentParamsToXmlPayload.java (6)
M azurecompute/src/main/java/org/jclouds/azurecompute/binders/BindOSImageParamsToXmlPayload.java (6)
M azurecompute/src/main/java/org/jclouds/azurecompute/compute/AzureComputeServiceAdapter.java (1)
M azurecompute/src/main/java/org/jclouds/azurecompute/compute/config/AzureComputeServiceContextModule.java (5)
M azurecompute/src/main/java/org/jclouds/azurecompute/compute/functions/AzureImageToImage.java (3)
M azurecompute/src/main/java/org/jclouds/azurecompute/compute/functions/DeploymentToNodeMetadata.java (3)
M azurecompute/src/main/java/org/jclouds/azurecompute/compute/functions/RoleSizeToHardware.java (3)
M azurecompute/src/main/java/org/jclouds/azurecompute/config/AzureComputeHttpApiModule.java (7)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/Deployment.java (363)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/DeploymentParams.java (3)
D azurecompute/src/main/java/org/jclouds/azurecompute/domain/DeploymentSlot.java (42)
D azurecompute/src/main/java/org/jclouds/azurecompute/domain/DeploymentStatus.java (43)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/DetailedHostedServiceProperties.java (10)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/Disk.java (7)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/Error.java (4)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/HostedService.java (7)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/HostedServiceProperties.java (4)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/Image.java (9)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/ImageParams.java (7)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/InstanceStatus.java (4)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/Location.java (7)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/OSType.java (4)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/Operation.java (4)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/RoleSize.java (4)
M azurecompute/src/main/java/org/jclouds/azurecompute/features/DeploymentApi.java (5)
M azurecompute/src/main/java/org/jclouds/azurecompute/features/DiskApi.java (8)
M azurecompute/src/main/java/org/jclouds/azurecompute/features/HostedServiceApi.java (8)
M azurecompute/src/main/java/org/jclouds/azurecompute/features/ImageApi.java (8)
M azurecompute/src/main/java/org/jclouds/azurecompute/features/LocationApi.java (6)
M azurecompute/src/main/java/org/jclouds/azurecompute/features/OperationApi.java (5)
M azurecompute/src/main/java/org/jclouds/azurecompute/features/VirtualMachineApi.java (1)
M azurecompute/src/main/java/org/jclouds/azurecompute/functions/ImageParamsName.java (8)
M azurecompute/src/main/java/org/jclouds/azurecompute/functions/ParseRequestIdHeader.java (6)
M azurecompute/src/main/java/org/jclouds/azurecompute/options/CreateHostedServiceOptions.java (3)
M azurecompute/src/main/java/org/jclouds/azurecompute/suppliers/KeyStoreSupplier.java (13)
M azurecompute/src/main/java/org/jclouds/azurecompute/suppliers/SSLContextWithKeysSupplier.java (9)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/DeploymentHandler.java (24)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/DetailedHostedServicePropertiesHandler.java (7)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/DiskHandler.java (8)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/HostedServiceHandler.java (8)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/HostedServicePropertiesHandler.java (8)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/HostedServiceWithDetailedPropertiesHandler.java (1)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/ImageHandler.java (10)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/ListDisksHandler.java (8)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/ListHostedServicesHandler.java (8)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/ListImagesHandler.java (8)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/ListLocationsHandler.java (8)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/OperationHandler.java (7)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/DeploymentApiMockTest.java (7)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/DiskApiLiveTest.java (14)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/DiskApiMockTest.java (7)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/HostedServiceApiLiveTest.java (20)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/HostedServiceApiMockTest.java (11)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/ImageApiLiveTest.java (16)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/ImageApiMockTest.java (8)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/LocationApiLiveTest.java (12)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/LocationApiMockTest.java (7)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/OperationApiMockTest.java (7)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/VirtualMachineApiMockTest.java (5)
M azurecompute/src/test/java/org/jclouds/azurecompute/internal/BaseAzureComputeApiLiveTest.java (5)
M azurecompute/src/test/java/org/jclouds/azurecompute/internal/BaseAzureComputeApiMockTest.java (20)
M azurecompute/src/test/java/org/jclouds/azurecompute/parse/ErrorTest.java (5)
M azurecompute/src/test/java/org/jclouds/azurecompute/parse/GetDeploymentTest.java (17)
M azurecompute/src/test/java/org/jclouds/azurecompute/parse/GetHostedServiceDetailsTest.java (5)
M azurecompute/src/test/java/org/jclouds/azurecompute/parse/GetHostedServiceTest.java (5)
M azurecompute/src/test/java/org/jclouds/azurecompute/parse/GetOperationTest.java (5)
M azurecompute/src/test/java/org/jclouds/azurecompute/parse/ListDisksTest.java (6)
M azurecompute/src/test/java/org/jclouds/azurecompute/parse/ListHostedServicesTest.java (6)
M azurecompute/src/test/java/org/jclouds/azurecompute/parse/ListImagesTest.java (6)
M azurecompute/src/test/java/org/jclouds/azurecompute/parse/ListLocationsTest.java (6)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/92.patch
https://github.com/jclouds/jclouds-labs/pull/92.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
thanks for the review, @demobox! I'll ping you when done the last cleanup round.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59648657
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #325](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/325/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59805936
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1699](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1699/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59387964
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
> }
>
> @Override
> public String toString() {
> - return MoreObjects.toStringHelper(this).omitNullValues().add("code", rawCode).add("message", message).toString();
> + return MoreObjects.toStringHelper(this).omitNullValues().add("code", code).add("message", message).toString();
unintentional and will remove. I think this one was just copied over.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19059954
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #320](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/320/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59467501
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Andrew Phillips <no...@github.com>.
>
> - /**
> - * Specifies the name for the virtual machine. The name must be unique
> - * within Windows Azure.
> - */
> - private final String roleName;
> + /** Specifies the name for the virtual machine. The name must be unique within Windows Azure. */
Wow, that's quite some restriction! ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19055526
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
@demobox wrt builder for output-only.. I think it was probably my fault. I'm sure I suggested that we use builders, but didn't think through that they are only relevant for input objects.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59764125
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1718](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1718/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59766684
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Andrew Phillips <no...@github.com>.
Thanks for another big cleanup, @adriancole! +1 - looks good to me.
A couple of minor questions, but they can be dealt with, if at all, in later stages of the cleanup.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59625836
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Adrian Cole <no...@github.com>.
>
> - /**
> - * Specifies the name for the virtual machine. The name must be unique
> - * within Windows Azure.
> - */
> - private final String roleName;
> + /** Specifies the name for the virtual machine. The name must be unique within Windows Azure. */
Thanks! There are a lot of copy paste problems along with the bitrot. I
have one more structural change then hope to update the api to latest and
fine toothed comb. If you see any more things like this either send a pull
request or make a comment.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19151524
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1696](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1696/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59225000
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Andrew Phillips <no...@github.com>.
> - CaseFormat.UPPER_UNDERSCORE, checkNotNull(type, "type")));
> - } catch (IllegalArgumentException e) {
> - return ROLE_STATE_UNKNOWN;
> - }
> - }
> + /**
> + * The role state is currently unknown. The state should automatically be
> + * resolved once the role state is detected, so no action is required.
> + */
> + ROLE_STATE_UNKNOWN,
> +
> + /**
> + * The host agent is currently creating resources for the Virtual Machine
> + * (VM).
> + */
> + CREATING_V_M,
Out of curiosity (not something to be addressed in this PR, I think): are these copied from Azure somehow? `V_M` looks a bit strange... ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19055594
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Andrew Phillips <no...@github.com>.
> }
>
> - @Override
> - public int hashCode() {
> - return Objects.hashCode(name);
> + // TODO: Remove from here down with @AutoValue.
> + private Disk(String name, String location, String affinityGroup, String description, OSType os, URI mediaLink,
> + Integer logicalSizeInGB, Attachment attachedTo, String sourceImage) {
> + this.name = checkNotNull(name, "name");
> + this.location = location;
> + this.affinityGroup = affinityGroup;
> + this.description = description;
> + this.os = os;
> + this.mediaLink = mediaLink;
> + this.logicalSizeInGB = checkNotNull(logicalSizeInGB, "logicalSizeInGB of %s", name);
This is listed above as `@Nullable`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19098188
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
> @@ -55,4 +55,12 @@ public void endElement(String uri, String name, String qName) throws SAXExceptio
> public void characters(char ch[], int start, int length) {
> currentText.append(ch, start, length);
> }
> +
> + public static Code code(String code) {
good catch. unintentionally public.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19059958
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
> - CaseFormat.UPPER_UNDERSCORE, checkNotNull(type, "type")));
> - } catch (IllegalArgumentException e) {
> - return ROLE_STATE_UNKNOWN;
> - }
> - }
> + /**
> + * The role state is currently unknown. The state should automatically be
> + * resolved once the role state is detected, so no action is required.
> + */
> + ROLE_STATE_UNKNOWN,
> +
> + /**
> + * The host agent is currently creating resources for the Virtual Machine
> + * (VM).
> + */
> + CREATING_V_M,
this looks like a latent bug. will review these after cleanup is done when I update the version.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19059957
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
> @@ -20,4 +20,4 @@ Once you do this, you will set the following to run the live tests.
> mvn -Plive -Dtest.jclouds.azurecompute.subscription-id=12345678-abcd-dcba-abdc-ba0987654321
> -Dtest.azurecompute.credential=P12_EXPORT_PASSWORD
> -Dtest.azurecompute.identity=$HOME/.jclouds/azure.p12
nope! actually this should be p12 for now anyway. I haven't refactored the authentication bit, yet, and was surprised myself at this!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19059948
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
ps so far I'm absolutely convinced we should end the practice of builders on output-only value types.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59651798
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #315](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/315/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59223175
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1710](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1710/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59616784
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
For the passer-by, I'm doing slow-burn cleanup, and also playing with ideas wrt where we actually need builders or defensive enums. Current thinking is that we don't unless it is an in-out or input value object. Once I'm done cleanup here, I can present a coherent idea back to the group. That may not happen until a week from now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59402036
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Andrew Phillips <no...@github.com>.
> }
> - } else if (equalsOrSuffix(qName, "Url")) {
> - final String url = currentOrNull(currentText);
> - if (url != null) {
> - builder.deploymentURL(URI.create(url));
> + } else if (qName.equals("RoleName")) {
> + virtualMachineName = currentOrNull(currentText);
Just checking that `RoleName` is indeed the VM name here..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19098505
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Adrian Cole <no...@github.com>.
merged into master. will do 1.8.x once 1.8.1 is cut.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59829753
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1700](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1700/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59390620
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1719](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1719/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59807564
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Andrew Phillips <no...@github.com>.
> }
>
> @Override
> public String toString() {
> - return MoreObjects.toStringHelper(this).omitNullValues().add("code", rawCode).add("message", message).toString();
> + return MoreObjects.toStringHelper(this).omitNullValues().add("code", code).add("message", message).toString();
[minor] The new implementations elsewhere seem to remove `omitNullValues`. Intentionally left here? What does `@AutoValue` do?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19055557
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Daniel Estévez <no...@github.com>.
>
> - /**
> - * Specifies the name for the virtual machine. The name must be unique
> - * within Windows Azure.
> - */
> - private final String roleName;
> + /** Specifies the name for the virtual machine. The name must be unique within Windows Azure. */
Actually this is not true. Hosted Services name are unique in Azure (they're used for DNS) but VM role names are only unique inside the hosted/cloud service they're deployed in.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19132594
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #324](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/324/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59765078
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Andrew Phillips <no...@github.com>.
> @@ -20,4 +20,4 @@ Once you do this, you will set the following to run the live tests.
> mvn -Plive -Dtest.jclouds.azurecompute.subscription-id=12345678-abcd-dcba-abdc-ba0987654321
> -Dtest.azurecompute.credential=P12_EXPORT_PASSWORD
> -Dtest.azurecompute.identity=$HOME/.jclouds/azure.p12
[minor] Should this also be `.pem`? If so, happy to fix this during merge...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19055517
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
> }
>
> - public String getPrivateIpAddress() {
> - return privateIpAddress;
> + private Deployment(Builder builder) {
Using constructor pattern like this leads to far less chance of mistake when builders are used.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r18899994
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #319](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/319/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59387024
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #322](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/322/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59614997
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #318](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/318/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59385116
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
added disk, the last biggie. I still have some consistency cleanups to do, and the input objects.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59651711
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Andrew Phillips <no...@github.com>.
>
> /**
> * @see <a href="http://msdn.microsoft.com/en-us/library/gg441293" >api</a>
> */
> -public class LocationHandler extends ParseSax.HandlerForGeneratedRequestWithResult<Location> {
> +final class LocationHandler extends ParseSax.HandlerForGeneratedRequestWithResult<Location> {
> + private String name;
> + private String displayName;
> + private final List<String> availableServices = Lists.newArrayList();
[minor] Same question as above about array lists vs. `ImmutableList.Builder` here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19098795
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Chris Custine <no...@github.com>.
+1 btw, what are we waiting on before using autovalue? A specific issue or feature?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59810657
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Andrew Phillips <no...@github.com>.
> we should end the practice of builders on output-only value types.
Do you recall where this came from in the first place..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59666304
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Adrian Cole <no...@github.com>.
> - } else if (equalsOrSuffix(qName, "ServiceName")) {
> - builder.name(currentOrNull(currentText));
> + if (qName.equals("DateCreated")) {
> + created = dateService.iso8601SecondsDateParse(currentOrNull(currentText));
> + } else if (qName.equals("DateLastModified")) {
> + lastModified = dateService.iso8601SecondsDateParse(currentOrNull(currentText));
> + } else if (qName.equals("Status")) {
> + String statusText = currentOrNull(currentText);
> + if (statusText != null) {
> + status = status(statusText);
> + }
> + } else if (qName.equals("Name")) {
> + propertyName = currentOrNull(currentText);
> + } else if (qName.equals("Value")) {
> + extendedProperties.put(propertyName, currentOrNull(currentText));
> + propertyName = null;
propertyValue comes after propertyName, and propertyName is needed to put that into the map.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19098953
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Andrew Phillips <no...@github.com>.
Only one comment that might need to be looked at now (a field marked `@Nullable` but with a `checkNotNull`) - the rest can all be ignored and/or handled later, if desired.
+1 - looks good to me! Thanks for more cleanup, @adriancole!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59804663
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
OK. all output objects are clean. I'd expect another 1K or so less lines when auto-value cuts another release.
Note: I decided to not use MoreObjects.toStringHelper for a few reasons.
1. Objects.toStringHelper will be around until June 2016, so a little early to worry about, esp in labs
2. Using MoreObjects.toStringHelper makes backporting a royal pita
3. I won't need any toStringHelper once auto-value is hooked up, thus avoiding the entire thrash problem for this class of guava usage.
@demobox ready for your review! After this, I'll work on the input objects, then update the api, then move on to compute service.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59764644
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Andrew Phillips <no...@github.com>.
>
> /**
> * @see <a href="http://msdn.microsoft.com/en-us/library/jj157191" >api</a>
> */
> -public class ImageHandler extends ParseSax.HandlerForGeneratedRequestWithResult<Image> {
> +final class ImageHandler extends ParseSax.HandlerForGeneratedRequestWithResult<Image> {
> + private String name;
> + private String location;
> + private String affinityGroup;
> + private String label;
> + private String category;
> + private String description;
> + private OSType os;
> + private URI mediaLink;
> + private Integer logicalSizeInGB;
> + private final List<String> eula = Lists.newArrayList();
[minor] I noticed you're using `ImmutableList.Builder` elsewhere for this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19098733
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Andrew Phillips <no...@github.com>.
> @@ -55,4 +55,12 @@ public void endElement(String uri, String name, String qName) throws SAXExceptio
> public void characters(char ch[], int start, int length) {
> currentText.append(ch, start, length);
> }
> +
> + public static Code code(String code) {
[minor] Is this called from elsewhere? Similar parsing methods (e.g. in `DeploymentHandler`) seem to be `private`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19055625
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Adrian Cole <no...@github.com>.
> }
>
> - @Override
> - public int hashCode() {
> - return Objects.hashCode(name);
> + // TODO: Remove from here down with @AutoValue.
> + private Disk(String name, String location, String affinityGroup, String description, OSType os, URI mediaLink,
> + Integer logicalSizeInGB, Attachment attachedTo, String sourceImage) {
> + this.name = checkNotNull(name, "name");
> + this.location = location;
> + this.affinityGroup = affinityGroup;
> + this.description = description;
> + this.os = os;
> + this.mediaLink = mediaLink;
> + this.logicalSizeInGB = checkNotNull(logicalSizeInGB, "logicalSizeInGB of %s", name);
nullable was wrong. fixed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19099082
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1715](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1715/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59652293
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
> + instanceStatus + ", instanceStateDetails=" + instanceStateDetails + ", instanceErrorCode="
> + instanceErrorCode + ", instanceSize=" + instanceSize + ", privateIpAddress=" + privateIpAddress
> + ", publicIpAddress=" + publicIpAddress + "]";
> }
> +
> + public static Builder builder() {
> + return new Builder();
> + }
> +
> + public static class Builder {
Likely going to kill this or move it to the XML parser. There's no reason for a user to create this object.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r18900072
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
Live tests still pass. Can't migrate to google auto value yet as we use a custom `@Nullable` annotation. The fix for that was merged in Feb, but there's not been a new release, yet. [Pinged them](https://github.com/google/auto/issues/144).
Also note. I'm only about 1/2 done cleaning objects anyway.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59614675
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Andrew Phillips <no...@github.com>.
> - } else if (equalsOrSuffix(qName, "ServiceName")) {
> - builder.name(currentOrNull(currentText));
> + if (qName.equals("DateCreated")) {
> + created = dateService.iso8601SecondsDateParse(currentOrNull(currentText));
> + } else if (qName.equals("DateLastModified")) {
> + lastModified = dateService.iso8601SecondsDateParse(currentOrNull(currentText));
> + } else if (qName.equals("Status")) {
> + String statusText = currentOrNull(currentText);
> + if (statusText != null) {
> + status = status(statusText);
> + }
> + } else if (qName.equals("Name")) {
> + propertyName = currentOrNull(currentText);
> + } else if (qName.equals("Value")) {
> + extendedProperties.put(propertyName, currentOrNull(currentText));
> + propertyName = null;
[minor] Curious why this is here but not for `Name` just above?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19098644
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Adrian Cole <no...@github.com>.
Closed #92.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#event-181125888
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
cc @abayer
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59222099
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
> - CaseFormat.UPPER_UNDERSCORE, checkNotNull(type, "type")));
> - } catch (IllegalArgumentException e) {
> - return ROLE_STATE_UNKNOWN;
> - }
> - }
> + /**
> + * The role state is currently unknown. The state should automatically be
> + * resolved once the role state is detected, so no action is required.
> + */
> + ROLE_STATE_UNKNOWN,
> +
> + /**
> + * The host agent is currently creating resources for the Virtual Machine
> + * (VM).
> + */
> + CREATING_V_M,
hah looks like it might be a hack around casing! http://msdn.microsoft.com/en-us/library/azure/ee460804.aspx#RoleInstanceList
Since this is output-only, I suspect such a hack isn't helpful. will add a test case.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19060027
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Adrian Cole <no...@github.com>.
> }
>
> - public String getPrivateIpAddress() {
> - return privateIpAddress;
> + private Deployment(Builder builder) {
Also, intentionally didn't do a nullable sweep. This is just reorganizing the fields however good or bad they are.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r18900034
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Adrian Cole <no...@github.com>.
> }
> - } else if (equalsOrSuffix(qName, "Url")) {
> - final String url = currentOrNull(currentText);
> - if (url != null) {
> - builder.deploymentURL(URI.create(url));
> + } else if (qName.equals("RoleName")) {
> + virtualMachineName = currentOrNull(currentText);
comments and documentation eluded to that.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19098682
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Adrian Cole <no...@github.com>.
>
> /**
> * @see <a href="http://msdn.microsoft.com/en-us/library/jj157191" >api</a>
> */
> -public class ImageHandler extends ParseSax.HandlerForGeneratedRequestWithResult<Image> {
> +final class ImageHandler extends ParseSax.HandlerForGeneratedRequestWithResult<Image> {
> + private String name;
> + private String location;
> + private String affinityGroup;
> + private String label;
> + private String category;
> + private String description;
> + private OSType os;
> + private URI mediaLink;
> + private Integer logicalSizeInGB;
> + private final List<String> eula = Lists.newArrayList();
using immutable for throw-away handlers. this one is used in a loop.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19098816
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #323](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/323/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59651953
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by Andrew Phillips <no...@github.com>.
> + } catch (IllegalArgumentException e) {
> + return Status.UNRECOGNIZED;
> + }
> + }
> +
> + private static Slot parseSlot(String slot) {
> + try {
> + return Slot.valueOf(UPPER_CAMEL.to(UPPER_UNDERSCORE, slot));
> + } catch (IllegalArgumentException e) {
> + return Slot.UNRECOGNIZED;
> + }
> + }
> +
> + @VisibleForTesting static InstanceStatus parseInstanceStatus(String instanceStatus) {
> + try {
> + // Azure isn't exactly upper-camel, as some states end in VM, not Vm.
Ahhh...**this** is what this was for!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92/files#r19063624
Re: [jclouds-labs] Cleanup round 1 of azurecompute: Output value
types. (#92)
Posted by Adrian Cole <no...@github.com>.
@ccustine there's a snapshot published to sonatype, but no stable version. Since we use our own Nullable annotation, auto-value 1.0-rc1 can't work, but anything after Feb will :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59828480
Re: [jclouds-labs] Cleanup round 1 of azurecompute Deployment class
and imports. (#92)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1701](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1701/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/92#issuecomment-59468146