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/21 00:12:57 UTC
[jclouds-labs] Couple OSType to Image and cleanup DeploymentParams
(#94)
DeploymentParams oddly had a name member, which made it senseless to use as a template ("knowing name is unique"). This sorts that out and also cleans up the input value type, retaining the Builder until we figure something better to do about that.
This also nests OSType into Image as looking at the docs, that's the canonical place for it.
The combination of these two changes leave the top-level namespace of the domain objects in pretty good shape. That said, a follow-up PR will clean up ImageParams as that requires some thinking.
You can merge this Pull Request by running:
git pull https://github.com/adriancole/jclouds-labs adrian.azurecompute-input-cleanup
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs/pull/94
-- Commit Summary --
* Cleanup Azure compute DeploymentParams input value type.
* Azure's OSType is reliable so needs no defensive UNRECOGNIZED enum value. Simplify use and couple to Image.
-- File Changes --
M azurecompute/src/main/java/org/jclouds/azurecompute/binders/BindOSImageParamsToXmlPayload.java (25)
R azurecompute/src/main/java/org/jclouds/azurecompute/binders/CreateDeploymentToXML.java (82)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/DeploymentParams.java (314)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/Disk.java (1)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/Image.java (3)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/ImageParams.java (9)
D azurecompute/src/main/java/org/jclouds/azurecompute/domain/InputEndpoint.java (137)
D azurecompute/src/main/java/org/jclouds/azurecompute/domain/OSType.java (45)
D azurecompute/src/main/java/org/jclouds/azurecompute/domain/Protocol.java (23)
M azurecompute/src/main/java/org/jclouds/azurecompute/features/DeploymentApi.java (10)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/DiskHandler.java (12)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/ImageHandler.java (7)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/DeploymentApiMockTest.java (19)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/DiskApiLiveTest.java (3)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/ImageApiLiveTest.java (2)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/ImageApiMockTest.java (2)
M azurecompute/src/test/java/org/jclouds/azurecompute/xml/ListDisksHandlerTest.java (2)
M azurecompute/src/test/java/org/jclouds/azurecompute/xml/ListImagesHandlerTest.java (5)
M azurecompute/src/test/resources/deploymentparams.xml (2)
M azurecompute/src/test/resources/images.xml (4)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/94.patch
https://github.com/jclouds/jclouds-labs/pull/94.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/94
Re: [jclouds-labs] Couple OSType to Image and cleanup
DeploymentParams (#94)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #326](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/326/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/94#issuecomment-59849422
Re: [jclouds-labs] Couple OSType to Image and cleanup
DeploymentParams (#94)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #327](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/327/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/94#issuecomment-59876214
Re: [jclouds-labs] Couple OSType to Image and cleanup
DeploymentParams (#94)
Posted by Adrian Cole <no...@github.com>.
Closed #94.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/94#event-181307666
Re: [jclouds-labs] Couple OSType to Image and cleanup
DeploymentParams (#94)
Posted by Andrew Phillips <no...@github.com>.
> XMLBuilder configBuilder = builder.e("ConfigurationSet"); // Windows
> configBuilder.e("ConfigurationSetType").t("WindowsProvisioningConfiguration").up()
> - .e("ComputerName").t(params.getUsername()).up()
> - .e("AdminPassword").t(params.getPassword()).up()
> + .e("ComputerName").t(params.username()).up()
For Unix below, `HostName` maps to `name` - `username` is correct for `ComputerName` here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/94/files#r19126412
Re: [jclouds-labs] Couple OSType to Image and cleanup
DeploymentParams (#94)
Posted by Adrian Cole <no...@github.com>.
merged to master. can do 1.8.x after 1.8.1
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/94#issuecomment-59877226
Re: [jclouds-labs] Couple OSType to Image and cleanup
DeploymentParams (#94)
Posted by Adrian Cole <no...@github.com>.
> @@ -64,7 +64,10 @@ private void resetState() {
>
> @Override public void endElement(String ignoredUri, String ignoredName, String qName) {
> if (qName.equals("OS")) {
> - os = OSType.fromValue(currentOrNull(currentText));
> + String osText = currentOrNull(currentText);
> + if (osText != null) {
heh.. I found that that NULL was me being too literal :) Image os is actually required per http://msdn.microsoft.com/en-us/library/jj157191.aspx
Good question, though!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/94/files#r19128448
Re: [jclouds-labs] Couple OSType to Image and cleanup
DeploymentParams (#94)
Posted by Adrian Cole <no...@github.com>.
> }
> }
>
> - private static String url(DeploymentParams params) {
> - return String.format("http://%s.blob.core.windows.net/disks/%s/%s", params.getStorageAccount(), params.getName(),
> - params.getSourceImageName());
> + private static String upperCamel(String input) {
good eye. It was used twice earlier, but no more. will inline!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/94/files#r19128372
Re: [jclouds-labs] Couple OSType to Image and cleanup
DeploymentParams (#94)
Posted by Adrian Cole <no...@github.com>.
@demobox @ccustine @abayer comin.. round.. the mountain
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/94#issuecomment-59848609
Re: [jclouds-labs] Couple OSType to Image and cleanup
DeploymentParams (#94)
Posted by Adrian Cole <no...@github.com>.
addressed stuff. thx dude! will merge to master after buildhive.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/94#issuecomment-59875759
Re: [jclouds-labs] Couple OSType to Image and cleanup
DeploymentParams (#94)
Posted by Andrew Phillips <no...@github.com>.
> }
> }
>
> - private static String url(DeploymentParams params) {
> - return String.format("http://%s.blob.core.windows.net/disks/%s/%s", params.getStorageAccount(), params.getName(),
> - params.getSourceImageName());
> + private static String upperCamel(String input) {
[minor] Only seems to be used once so far - inline in a later cleanup round, perhaps?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/94/files#r19126448
Re: [jclouds-labs] Couple OSType to Image and cleanup
DeploymentParams (#94)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1723](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1723/) 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/94#issuecomment-59850586
Re: [jclouds-labs] Couple OSType to Image and cleanup
DeploymentParams (#94)
Posted by Andrew Phillips <no...@github.com>.
> @@ -64,7 +64,10 @@ private void resetState() {
>
> @Override public void endElement(String ignoredUri, String ignoredName, String qName) {
> if (qName.equals("OS")) {
> - os = OSType.fromValue(currentOrNull(currentText));
> + String osText = currentOrNull(currentText);
> + if (osText != null) {
Above, there's also a `osText.toUpperCase().equals("NULL")` check - needed here too?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/94/files#r19126504
Re: [jclouds-labs] Couple OSType to Image and cleanup
DeploymentParams (#94)
Posted by Adrian Cole <no...@github.com>.
> XMLBuilder configBuilder = builder.e("ConfigurationSet"); // Windows
> configBuilder.e("ConfigurationSetType").t("WindowsProvisioningConfiguration").up()
> - .e("ComputerName").t(params.getUsername()).up()
> - .e("AdminPassword").t(params.getPassword()).up()
> + .e("ComputerName").t(params.username()).up()
Wow. there's a latent bug for ya! I'll add a test case for windows, since this is ridiculous.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/94/files#r19128362
Re: [jclouds-labs] Couple OSType to Image and cleanup
DeploymentParams (#94)
Posted by Andrew Phillips <no...@github.com>.
Codewise, +1 - looks good to me. Two questions about functionality that can be addressed during the merge; we don't need to go for another review round for those.
Thanks, @adriancole!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/94#issuecomment-59869380
Re: [jclouds-labs] Couple OSType to Image and cleanup
DeploymentParams (#94)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1725](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1725/) 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/94#issuecomment-59876693