You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Francesco Chicchiriccò <no...@github.com> on 2015/03/17 18:12:09 UTC
[jclouds-labs] [JCLOUDS-850] Providing AzureTemplateBuilderLiveTest
(#156)
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/156
-- Commit Summary --
* [JCLOUDS-850] Providing AzureTemplateBuilderLiveTest
-- File Changes --
M azurecompute/src/main/java/org/jclouds/azurecompute/AzureComputeProviderMetadata.java (10)
M azurecompute/src/main/java/org/jclouds/azurecompute/compute/AzureComputeServiceAdapter.java (2)
M azurecompute/src/main/java/org/jclouds/azurecompute/compute/functions/LocationToLocation.java (10)
M azurecompute/src/main/java/org/jclouds/azurecompute/compute/functions/OSImageToImage.java (53)
M azurecompute/src/main/java/org/jclouds/azurecompute/domain/OSImage.java (18)
A azurecompute/src/main/java/org/jclouds/azurecompute/domain/Region.java (90)
M azurecompute/src/main/java/org/jclouds/azurecompute/options/AzureComputeTemplateOptions.java (15)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/OSImageHandler.java (9)
M azurecompute/src/test/java/org/jclouds/azurecompute/AzureComputeProviderMetadataLive.java (24)
A azurecompute/src/test/java/org/jclouds/azurecompute/compute/AzureTemplateBuilderLiveTest.java (51)
M azurecompute/src/test/java/org/jclouds/azurecompute/compute/functions/OSImageToImageTest.java (10)
M azurecompute/src/test/java/org/jclouds/azurecompute/internal/BaseAzureComputeApiLiveTest.java (2)
M azurecompute/src/test/java/org/jclouds/azurecompute/xml/ListOSImagesHandlerTest.java (15)
M azurecompute/src/test/resources/images.xml (188)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/156.patch
https://github.com/jclouds/jclouds-labs/pull/156.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public AzureTemplateBuilderLiveTest() {
> + super();
> +
> + provider = "azurecompute";
> + }
> +
> + @Override
> + protected ProviderMetadata createProviderMetadata() {
> + synchronized (this) {
> + if (providerMeta == null) {
> + providerMeta = new AzureComputeProviderMetadata();
> + }
> + }
> + return providerMeta;
> + }
Fair enough. Is the synchronization needed?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156/files#r27566230
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Francesco Chicchiriccò <no...@github.com>.
@nacx all comments should now be addressed; still waiting for confirmation on [this one](https://github.com/jclouds/jclouds-labs/pull/156#discussion_r27569236).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156#issuecomment-88778434
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Ignasi Barrera <no...@github.com>.
When going to merge this I had a conflict in the "default template" property I wasn't sure how to properly resolve. Mind rebasing to the latest version (that has #158 and #153 merged)? Meanwhile I'll try to merge JCLOUDS-876 to have a cleaner PR. Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156#issuecomment-90242650
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Francesco Chicchiriccò <no...@github.com>.
@nacx PR rebased: all live tests (80, took approximately 2,5 hours) are working.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156#issuecomment-90506360
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Francesco Chicchiriccò <no...@github.com>.
> +
> + public AzureTemplateBuilderLiveTest() {
> + super();
> +
> + provider = "azurecompute";
> + }
> +
> + @Override
> + protected ProviderMetadata createProviderMetadata() {
> + synchronized (this) {
> + if (providerMeta == null) {
> + providerMeta = new AzureComputeProviderMetadata();
> + }
> + }
> + return providerMeta;
> + }
I could not find any way to fetch the provider metadata once created by base class method (and I needed it for `getIso3166Codes()`), so I thought the simpler way was to override and provide a local reference.
BTW This is what I've also seen from [AWSEC2](https://github.com/jclouds/jclouds/blob/acd06b30245c4df27e4e36176916c95df60e42ca/providers/aws-ec2/src/test/java/org/jclouds/aws/ec2/compute/AWSEC2TemplateBuilderLiveTest.java#L65)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156/files#r27556105
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public AzureTemplateBuilderLiveTest() {
> + super();
> +
> + provider = "azurecompute";
> + }
> +
> + @Override
> + protected ProviderMetadata createProviderMetadata() {
> + synchronized (this) {
> + if (providerMeta == null) {
> + providerMeta = new AzureComputeProviderMetadata();
> + }
> + }
> + return providerMeta;
> + }
Is this really necessary? Isn't the base class method working?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156/files#r27519615
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -89,6 +89,14 @@ public void copyTo(final TemplateOptions to) {
> if (storageAccountType.isPresent()) {
> eTo.storageAccountType(storageAccountType.get());
> }
> +
> + final LoginCredentials credentials = new LoginCredentials.Builder().
> + user(loginUser).
> + password(loginPassword == null || !loginPassword.isPresent() ? null : loginPassword.get()).
> + privateKey(loginPrivateKey == null || !loginPrivateKey.isPresent() ? null : loginPrivateKey.get()).
> + authenticateSudo(authenticateSudo == null ? false : authenticateSudo).
> + build();
> + eTo.overrideLoginCredentials(credentials);
Worth fixing the class that does not properly copy that then?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156/files#r27566165
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Francesco Chicchiriccò <no...@github.com>.
> @@ -89,6 +89,14 @@ public void copyTo(final TemplateOptions to) {
> if (storageAccountType.isPresent()) {
> eTo.storageAccountType(storageAccountType.get());
> }
> +
> + final LoginCredentials credentials = new LoginCredentials.Builder().
> + user(loginUser).
> + password(loginPassword == null || !loginPassword.isPresent() ? null : loginPassword.get()).
> + privateKey(loginPrivateKey == null || !loginPrivateKey.isPresent() ? null : loginPrivateKey.get()).
> + authenticateSudo(authenticateSudo == null ? false : authenticateSudo).
> + build();
> + eTo.overrideLoginCredentials(credentials);
As you can see, I have opened [JCLOUDS-876](https://issues.apache.org/jira/browse/JCLOUDS-876) and provided a comment there; for the moment I have also reduced the temporary fix for this issue with last commit 00058df98c7d2e1c9a60ee006885d7610cb7bd3f
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156/files#r27660692
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Francesco Chicchiriccò <no...@github.com>.
> +
> + public AzureTemplateBuilderLiveTest() {
> + super();
> +
> + provider = "azurecompute";
> + }
> +
> + @Override
> + protected ProviderMetadata createProviderMetadata() {
> + synchronized (this) {
> + if (providerMeta == null) {
> + providerMeta = new AzureComputeProviderMetadata();
> + }
> + }
> + return providerMeta;
> + }
Probably not, I'll re-run all tests without it for further check.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156/files#r27566957
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Ignasi Barrera <no...@github.com>.
Squashed and pushed to master and 1.9.x. Many thanks @ilgrosso!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156#issuecomment-90936758
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -81,34 +83,44 @@ private Location createLocation(final String input) {
> if (input == null) {
> return null;
> }
> - return new LocationBuilder().
> - id(input).
> - scope(LocationScope.REGION).
> - description(input).
> - parent(Iterables.getOnlyElement(provider.get())).
> - metadata(ImmutableMap.<String, Object>of("name", input)).
> - build();
> +
> + final LocationBuilder builder = new LocationBuilder();
Instead of manually creating a location here, you'd better inject the location supplier into this class and pick one of the existing locations.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156/files#r27518879
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Francesco Chicchiriccò <no...@github.com>.
> @@ -89,6 +89,14 @@ public void copyTo(final TemplateOptions to) {
> if (storageAccountType.isPresent()) {
> eTo.storageAccountType(storageAccountType.get());
> }
> +
> + final LoginCredentials credentials = new LoginCredentials.Builder().
> + user(loginUser).
> + password(loginPassword == null || !loginPassword.isPresent() ? null : loginPassword.get()).
> + privateKey(loginPrivateKey == null || !loginPrivateKey.isPresent() ? null : loginPrivateKey.get()).
> + authenticateSudo(authenticateSudo == null ? false : authenticateSudo).
> + build();
> + eTo.overrideLoginCredentials(credentials);
After some more investigation, it seems that without the snippet above, the original object shows
```
loginPassword=Optional.absent()
loginPrivateKey=Optional.absent()
```
while `eTo`
```
loginPassword=<null>
loginPrivateKey=<null>
```
I believe the problem is located in [TemplateOptions around lines 89-96](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/options/TemplateOptions.java#L89-L96) which should be better replaced by the snippet above. Alternatively, `loginPassword` and `loginPrivateKey` should be initialized to `Optional.absent()`.
WDYT? Shall I open an issue / PR about this and leave the fix temporarily here (in order to keep the test case above work)?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156/files#r27569236
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Francesco Chicchiriccò <no...@github.com>.
@nacx Sure! I'll try to rebase ASAP, thx for all merges.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156#issuecomment-90405749
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Ignasi Barrera <no...@github.com>.
See https://github.com/jclouds/jclouds/pull/724 for a fix for the copy options issue. Can you try this on top of that commit, without the workaround?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156#issuecomment-90731625
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -89,6 +89,14 @@ public void copyTo(final TemplateOptions to) {
> if (storageAccountType.isPresent()) {
> eTo.storageAccountType(storageAccountType.get());
> }
> +
> + final LoginCredentials credentials = new LoginCredentials.Builder().
> + user(loginUser).
> + password(loginPassword == null || !loginPassword.isPresent() ? null : loginPassword.get()).
> + privateKey(loginPrivateKey == null || !loginPrivateKey.isPresent() ? null : loginPrivateKey.get()).
> + authenticateSudo(authenticateSudo == null ? false : authenticateSudo).
> + build();
> + eTo.overrideLoginCredentials(credentials);
>WDYT? Shall I open an issue / PR about this and leave the fix temporarily here (in order to keep the test case above working)?
If you can do that, that would be much appreciated. Let's fix jclouds as soon as we find some issues instead of silo-ing fixes in providers.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156/files#r27641008
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Francesco Chicchiriccò <no...@github.com>.
> @@ -89,6 +89,14 @@ public void copyTo(final TemplateOptions to) {
> if (storageAccountType.isPresent()) {
> eTo.storageAccountType(storageAccountType.get());
> }
> +
> + final LoginCredentials credentials = new LoginCredentials.Builder().
> + user(loginUser).
> + password(loginPassword == null || !loginPassword.isPresent() ? null : loginPassword.get()).
> + privateKey(loginPrivateKey == null || !loginPrivateKey.isPresent() ? null : loginPrivateKey.get()).
> + authenticateSudo(authenticateSudo == null ? false : authenticateSudo).
> + build();
> + eTo.overrideLoginCredentials(credentials);
Without this, I get failure in [BaseTemplateBuilderLiveTest#testFromTemplate](https://github.com/jclouds/jclouds/blob/master/compute/src/test/java/org/jclouds/compute/internal/BaseTemplateBuilderLiveTest.java#L65) because `loginPasswordPresent=true, loginPrivateKeyPresent=true` is not correctly copied.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156/files#r27565869
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -89,6 +89,14 @@ public void copyTo(final TemplateOptions to) {
> if (storageAccountType.isPresent()) {
> eTo.storageAccountType(storageAccountType.get());
> }
> +
> + final LoginCredentials credentials = new LoginCredentials.Builder().
> + user(loginUser).
> + password(loginPassword == null || !loginPassword.isPresent() ? null : loginPassword.get()).
> + privateKey(loginPrivateKey == null || !loginPrivateKey.isPresent() ? null : loginPrivateKey.get()).
> + authenticateSudo(authenticateSudo == null ? false : authenticateSudo).
> + build();
> + eTo.overrideLoginCredentials(credentials);
Is this really needed? Doesn't the superclass already copy this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156/files#r27519191
Re: [jclouds-labs] [JCLOUDS-850] Providing
AzureTemplateBuilderLiveTest (#156)
Posted by Francesco Chicchiriccò <no...@github.com>.
@nacx I've applied the patch from jclouds/jclouds#724, rebuilt there then removed the workaround from here: everything is working, cool!
Shall I wait for that PR to be merged before removing the workaround here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/156#issuecomment-90821837