You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Melan Nimesh Jayasingha <no...@github.com> on 2014/06/23 07:35:49 UTC
[jclouds] [JCLOUDS-609] Adding vCloud API 1.5 support (#418)
Please review the patch
You can merge this Pull Request by running:
git pull https://github.com/melanj/jclouds master
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/418
-- Commit Summary --
* [JCLOUDS-609] Adding vCloud API 1.5 support
-- File Changes --
M apis/vcloud/src/main/java/org/jclouds/vcloud/VCloudMediaType.java (9)
M apis/vcloud/src/main/java/org/jclouds/vcloud/VCloudVersionsApi.java (3)
M apis/vcloud/src/main/java/org/jclouds/vcloud/domain/ReferenceType.java (6)
M apis/vcloud/src/main/java/org/jclouds/vcloud/domain/internal/CatalogImpl.java (5)
M apis/vcloud/src/main/java/org/jclouds/vcloud/domain/internal/ReferenceTypeImpl.java (17)
M apis/vcloud/src/main/java/org/jclouds/vcloud/filters/AddVCloudAuthorizationAndCookieToRequest.java (22)
M apis/vcloud/src/main/java/org/jclouds/vcloud/functions/CatalogsInOrg.java (19)
A apis/vcloud/src/main/java/org/jclouds/vcloud/http/filters/VCloudBasicAuthentication.java (58)
A apis/vcloud/src/main/java/org/jclouds/vcloud/http/filters/VCloudSupportedVersions.java (35)
M apis/vcloud/src/main/java/org/jclouds/vcloud/internal/VCloudLoginApi.java (6)
M apis/vcloud/src/main/java/org/jclouds/vcloud/util/Utils.java (3)
M apis/vcloud/src/main/java/org/jclouds/vcloud/xml/OrgListHandler.java (2)
M apis/vcloud/src/test/java/org/jclouds/vcloud/VCloudVersionsApiTest.java (2)
M apis/vcloud/src/test/java/org/jclouds/vcloud/compute/BaseVCloudComputeServiceExpectTest.java (16)
M apis/vcloud/src/test/java/org/jclouds/vcloud/compute/strategy/InstantiateVAppTemplateWithGroupEncodedIntoNameThenCustomizeDeployAndPowerOnExpectTest.java (4)
M apis/vcloud/src/test/java/org/jclouds/vcloud/filters/AddVCloudAuthorizationAndCookieToRequestTest.java (4)
M apis/vcloud/src/test/java/org/jclouds/vcloud/internal/VCloudLoginApiTest.java (6)
M apis/vcloud/src/test/java/org/jclouds/vcloud/xml/ovf/VCloudVirtualHardwareSectionHandlerTest.java (4)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/418.patch
https://github.com/jclouds/jclouds/pull/418.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/418
Re: [jclouds] [JCLOUDS-609] Adding vCloud API 1.5 support (#418)
Posted by BuildHive <no...@github.com>.
[jclouds ยป jclouds #1268](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1268/) 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/pull/418#issuecomment-46809918
Re: [jclouds] [JCLOUDS-609] Adding vCloud API 1.5 support (#418)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -44,7 +48,20 @@
>
> @Override
> public Iterable<Catalog> apply(final Org org) {
> - return transform(org.getCatalogs().values(), new Function<ReferenceType, Catalog>() {
> +
> + Collection<ReferenceType> filtered = Collections2.filter(
> + org.getCatalogs().values(), new Predicate<ReferenceType>() {
> + @Override
> + public boolean apply(ReferenceType type) {
> + if(type == null){
> + return false;
> + }
> + return !ImmutableSet.of("add", "remove").contains(type.getRelationship());
Move this set to a variable outside the filter, to avoid creating it N times?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/418/files#r14185366
Re: [jclouds] [JCLOUDS-609] Adding vCloud API 1.5 support (#418)
Posted by Jeremy Daggett <no...@github.com>.
@melanj I am closing this PR since vcloud and vcloud-director have been removed from both the [jclouds](https://github.com/jclouds/jclouds/pull/604) and [jclouds-labs](https://github.com/jclouds/jclouds-labs/pull/111) repos. cc @demobox @nacx
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/418#issuecomment-70178078
Re: [jclouds] [JCLOUDS-609] Adding vCloud API 1.5 support (#418)
Posted by Adrian Cole <no...@github.com>.
> + this.creds = ((Supplier)Preconditions.checkNotNull(creds, "creds"));
> + this.apiVersion = apiVersion;
> + }
> +
> + public HttpRequest filter(HttpRequest request) throws HttpException
> + {
> + Credentials currentCreds = (Credentials)Preconditions.checkNotNull(this.creds.get(), "credential supplier returned null");
> + String acceptType = request.getFirstHeaderOrNull("Accept") == null ? "application/*+xml" : request.getFirstHeaderOrNull("Accept");
> +
> + String version = ";version=" + this.apiVersion;
> + String acceptHeader = acceptType + version;
> +
> + request = ((HttpRequest.Builder)request.toBuilder().replaceHeader("Accept", new String[] { acceptHeader })).build();
> +
> + return ((HttpRequest.Builder)request.toBuilder().replaceHeader("Authorization", new String[] { BasicAuthentication.basic(currentCreds.identity, currentCreds.credential) })).build();
> + }
this looks like conflating authentication with the accept header. maybe 2 filters is a better idea.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/418/files#r18427601
Re: [jclouds] [JCLOUDS-609] Adding vCloud API 1.5 support (#418)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1392](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1392/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/418#issuecomment-46810761
Re: [jclouds] [JCLOUDS-609] Adding vCloud API 1.5 support (#418)
Posted by Adrian Cole <no...@github.com>.
> +import org.jclouds.http.HttpRequest.Builder;
> +import org.jclouds.http.HttpRequestFilter;
> +import org.jclouds.http.filters.BasicAuthentication;
> +import org.jclouds.location.Provider;
> +import org.jclouds.rest.annotations.ApiVersion;
> +
> +@Singleton
> +public class VCloudBasicAuthentication implements HttpRequestFilter
> +{
> + private final Supplier<Credentials> creds;
> + private final String apiVersion;
> +
> + @Inject
> + public VCloudBasicAuthentication(@Provider Supplier<Credentials> creds, @ApiVersion String apiVersion)
> + {
> + this.creds = ((Supplier)Preconditions.checkNotNull(creds, "creds"));
why the cast?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/418/files#r18427591
Re: [jclouds] [JCLOUDS-609] Adding vCloud API 1.5 support (#418)
Posted by Ignasi Barrera <no...@github.com>.
> }
>
> @Override
> public HttpRequest filter(HttpRequest request) throws HttpException {
> - String token = vcloudTokenProvider.get();
> - return request
> - .toBuilder()
> - .replaceHeaders(
> - ImmutableMultimap.of("x-vcloud-authorization", token, HttpHeaders.COOKIE, "vcloud-token="
> - + token)).build();
> + String token = vcloudTokenProvider.get();
> + String acceptType = request.getFirstHeaderOrNull(HttpHeaders.ACCEPT) == null
What about:
```java
String acceptType = Objects.firstNotNull(request.getFirstHeaderOrNull(HttpHeaders.ACCEPT), "application/*+xml");
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/418/files#r14185260
Re: [jclouds] [JCLOUDS-609] Adding vCloud API 1.5 support (#418)
Posted by Jeremy Daggett <no...@github.com>.
Closed #418.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/418#event-219765488
Re: [jclouds] [JCLOUDS-609] Adding vCloud API 1.5 support (#418)
Posted by Melan Nimesh Jayasingha <no...@github.com>.
Thanks for the feedback @nacx. I will work on the suggestions/ improvements that you mentioned.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/418#issuecomment-47107377
Re: [jclouds] [JCLOUDS-609] Adding vCloud API 1.5 support (#418)
Posted by Adrian Cole <no...@github.com>.
heh nice to limp this along, though we really ought finish https://github.com/jclouds/jclouds-labs/tree/master/vcloud-director
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/418#issuecomment-57894734
Re: [jclouds] [JCLOUDS-609] Adding vCloud API 1.5 support (#418)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #921](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/921/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/418#issuecomment-46810424
Re: [jclouds] [JCLOUDS-609] Adding vCloud API 1.5 support (#418)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @melanj! Just a couple comments:
* [ ] Address the comments above.
* [ ] Use a 3 space indent and a 120 line length in Java classes.
* [ ] Run `mvn checkstyle:checkstyle` from the `vcloud` folder and address the violations (you'll find them in the `target/checkstyle-result.xml` file).
* [ ] Add the unis tests for the `VCloudBasicAuthentication` filter.
* [ ] Add the unis tests for the `VCloudSupportedVersions` filter.
Could you take a look at this checklist? Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/418#issuecomment-47102194
Re: [jclouds] [JCLOUDS-609] Adding vCloud API 1.5 support (#418)
Posted by Ignasi Barrera <no...@github.com>.
> + this.creds = ((Supplier)Preconditions.checkNotNull(creds, "creds"));
> + this.apiVersion = apiVersion;
> + }
> +
> + public HttpRequest filter(HttpRequest request) throws HttpException
> + {
> + Credentials currentCreds = (Credentials)Preconditions.checkNotNull(this.creds.get(), "credential supplier returned null");
> + String acceptType = request.getFirstHeaderOrNull("Accept") == null ? "application/*+xml" : request.getFirstHeaderOrNull("Accept");
> +
> + String version = ";version=" + this.apiVersion;
> + String acceptHeader = acceptType + version;
> +
> + request = ((HttpRequest.Builder)request.toBuilder().replaceHeader("Accept", new String[] { acceptHeader })).build();
> +
> + return ((HttpRequest.Builder)request.toBuilder().replaceHeader("Authorization", new String[] { BasicAuthentication.basic(currentCreds.identity, currentCreds.credential) })).build();
> + }
Could this class just extend the `BasicAuthentication` filter and just add the accept header after the parent has done its work?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/418/files#r14185496