You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Roman Coedo <no...@github.com> on 2014/05/09 00:00:27 UTC
[jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
You can merge this Pull Request by running:
git pull https://github.com/rcoedo/jclouds-labs-aws upstream
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-aws/pull/3
-- Commit Summary --
* Created the skeleton of the Glacier API.
-- File Changes --
A glacier/pom.xml (133)
A glacier/src/main/java/org/jclouds/glacier/GlacierApiMetadata.java (96)
A glacier/src/main/java/org/jclouds/glacier/GlacierAsyncClient.java (44)
A glacier/src/main/java/org/jclouds/glacier/GlacierClient.java (23)
A glacier/src/main/java/org/jclouds/glacier/config/GlacierRestClientModule.java (67)
A glacier/src/main/java/org/jclouds/glacier/filters/RequestAuthorizeSignature.java (65)
A glacier/src/main/java/org/jclouds/glacier/reference/GlacierHeaders.java (8)
A glacier/src/main/java/org/jclouds/glacier/util/AWSRequestSignerV4.java (195)
A glacier/src/main/resources/META-INF/services/org.jclouds.apis.ApiMetadata (1)
A glacier/src/test/java/org/jclouds/glacier/GlacierApiMetadataTest.java (33)
A glacier/src/test/java/org/jclouds/glacier/GlacierAsyncClientTest.java (35)
A glacier/src/test/java/org/jclouds/glacier/GlacierClientMockTest.java (62)
A glacier/src/test/java/org/jclouds/glacier/util/AWSRequestSignerV4Test.java (44)
A glacier/src/test/resources/META-INF/services/org.jclouds.apis.ApiMetadata (1)
M pom.xml (1)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-aws/pull/3.patch
https://github.com/jclouds/jclouds-labs-aws/pull/3.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Gaul <no...@github.com>.
> + buffer.append(dateWithoutTimeStamp).append("/");
> + buffer.append(REGION).append("/");
> + buffer.append(SERVICE).append("/");
> + buffer.append(TERMINATION_STRING);
> + return buffer.toString();
> + }
> +
> + private Multimap<String, String> buildCanonicalizedHeadersMap(HttpRequest request) {
> + Multimap<String, String> headers = request.getHeaders();
> + SortedSetMultimap<String, String> canonicalizedHeaders = TreeMultimap.create();
> + for (Entry<String, String> header : headers.entries()) {
> + if (header.getKey() == null)
> + continue;
> + String key = header.getKey().toString().toLowerCase(Locale.getDefault());
> + // Ignore any headers that are not particularly interesting.
> + if (key.equalsIgnoreCase(HttpHeaders.CONTENT_TYPE) || key.equalsIgnoreCase("Content-MD5")
Prefer ```HttpHeaders.CONTENT_MD5```.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12670149
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Gaul <no...@github.com>.
> + canonicalizedHeadersBuffer.append(key.toLowerCase()).append(':').append(header.getValue()).append('\n');
> + }
> + return canonicalizedHeadersBuffer.toString();
> + }
> +
> + private String buildSignedHeaders(Multimap<String, String> canonicalizedHeadersMap) {
> + return Joiner.on(';').join(Iterables.transform(canonicalizedHeadersMap.keySet(), new Function<String, String>() {
> +
> + @Override
> + public String apply(String input) {
> + return input.toLowerCase();
> + }
> + }));
> + }
> +
> + private String sha256(byte[] unhashedBytes) {
Make method static.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12670387
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public static final String AUTH_TAG = "AWS4";
> + public static final String HEADER_TAG = "x-amz-";
> + public static final String ALGORITHM = AUTH_TAG + "-HMAC-SHA256";
> + public static final String TERMINATION_STRING = "aws4_request";
> + public static final String REGION = "us-east-1";
> + public static final String SERVICE = "glacier";
> +
> + private final Crypto crypto;
> + private final String identity;
> + private final String credential;
> +
> + public AWSRequestSignerV4(String identity, String credential, Crypto crypto) {
> + this.crypto = crypto;
> + this.identity = identity;
> + this.credential = credential;
Same as above. Add `Preconditions.checkNotNull` checks to the mandatory fields.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12469130
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-aws #819](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-aws/819/) FAILURE
Looks like there's a problem with this pull request
[(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-aws/pull/3#issuecomment-42615183
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Gaul <no...@github.com>.
This mostly looks ready to go after you address some nits. Can you tag the relevant issue for all Glacier commits in the commit message, e.g.,
```JCLOUDS-457: Create skeleton of the Glacier API```
This will allow anyone to track development of the feature.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#issuecomment-42927480
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Ignasi Barrera <no...@github.com>.
> + .buildApi(GlacierClient.class);
> + }
> +
> + public void experiments() throws IOException, InterruptedException {
> + MockWebServer server = new MockWebServer();
> + server.enqueue(new MockResponse().addHeader(ETAG, "Testing"));
> + // hangs on Java 7 without this additional response ?!?
> + server.enqueue(new MockResponse().addHeader(ETAG, "Testing"));
> + server.play();
> +
> + GlacierClient client = getGlacierClient(server.getUrl("/"));
> + assertEquals(client.createVault("ConcreteVaultName"), true);
> + RecordedRequest request = server.takeRequest();
> + System.out.println(request);
> + System.out.println(request.getHeaders());
> +/* assertEquals(request.getRequestLine(), "PUT /bucket/object HTTP/1.1");
Try to avoid adding System.out outputs, remote this commented code, and properly check the recorded request fields match the expected ones for the request.
Also take care to shutdown the server in a `finally` block.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12469509
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-aws #827](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-aws/827/) 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-aws/pull/3#issuecomment-42783981
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Gaul <no...@github.com>.
> + * this work for additional information regarding copyright ownership.
> + * The ASF licenses this file to You under the Apache License, Version 2.0
> + * (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.glacier.reference;
> +
> +public interface GlacierHeaders {
Since this class only contains constants, can you make it a final class with a private constructor instead? Otherwise someone could instantiate or implement this class accidentally. Other Headers classes like ```AzureStorageHeaders``` follow this anti-pattern; can you fix those as well?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12570837
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Gaul <no...@github.com>.
Closed #3.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#event-121380589
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Roman Coedo <no...@github.com>.
That missing space is not in my code, should I change it? Thank you for your reviews!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#issuecomment-42841968
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-aws #821](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-aws/821/) 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-aws/pull/3#issuecomment-42616427
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Ignasi Barrera <no...@github.com>.
> +import java.io.IOException;
> +import java.net.URL;
> +import java.util.Properties;
> +import java.util.Set;
> +
> +import org.jclouds.ContextBuilder;
> +import org.jclouds.concurrent.config.ExecutorServiceModule;
> +import org.testng.annotations.Test;
> +
> +import com.google.common.collect.ImmutableSet;
> +import com.google.inject.Module;
> +import com.google.mockwebserver.MockResponse;
> +import com.google.mockwebserver.MockWebServer;
> +import com.google.mockwebserver.RecordedRequest;
> +
> +@Test(singleThreaded = true)
Does the test fail if the `singleThreaded` attribute is not set?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12469395
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-aws #835](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-aws/835/) 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-aws/pull/3#issuecomment-42940245
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #4](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/4/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#issuecomment-42616432
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Ignasi Barrera <no...@github.com>.
> + Properties overrides = new Properties();
> + // prevent expect-100 bug http://code.google.com/p/mockwebserver/issues/detail?id=6
> + overrides.setProperty(PROPERTY_SO_TIMEOUT, "0");
> + overrides.setProperty(PROPERTY_MAX_RETRIES, "1");
> + return ContextBuilder.newBuilder("glacier")
> + .credentials("accessKey", "secretKey")
> + .endpoint(server.toString())
> + .modules(modules)
> + .overrides(overrides)
> + .buildApi(GlacierClient.class);
> + }
> +
> + public void experiments() throws IOException, InterruptedException {
> + MockWebServer server = new MockWebServer();
> + server.enqueue(new MockResponse().addHeader(ETAG, "Testing"));
> + // hangs on Java 7 without this additional response ?!?
Does it only hang in Java 7? Usually when mockt ests hang is because a request is made and the code is waiting for the response. If mockwevserver does not have more responses configured, it just waits until the connection times out.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12469464
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-aws #818](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-aws/818/) FAILURE
Looks like there's a problem with this pull request
[(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-aws/pull/3#issuecomment-42612259
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Ignasi Barrera <no...@github.com>.
> + utils.logRequest(signatureLog, request, ">>");
> + request = this.signer.sign(request);
> + utils.logRequest(signatureLog, request, "<<");
> + return request;
> + }
> +
> + HttpRequest replaceDateHeader(HttpRequest request) {
> + request = request.toBuilder().removeHeader(HttpHeaders.DATE).build();
> + request = request.toBuilder().replaceHeader(GlacierHeaders.ALTERNATE_DATE, timeStampProvider.get()).build();
> + return request;
> + }
> +
> + HttpRequest replaceHostHeader(HttpRequest request) {
> + request = request.toBuilder().replaceHeader(HttpHeaders.HOST, request.getEndpoint().getHost()).build();
> + return request;
> + }
Is there any reason why these methods are not private?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12469109
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Gaul <no...@github.com>.
> + continue;
> + String key = header.getKey().toString().toLowerCase(Locale.getDefault());
> + // Ignore any headers that are not particularly interesting.
> + if (key.equalsIgnoreCase(HttpHeaders.CONTENT_TYPE) || key.equalsIgnoreCase("Content-MD5")
> + || key.equalsIgnoreCase(HttpHeaders.HOST) || key.startsWith(HEADER_TAG)) {
> + canonicalizedHeaders.put(key, header.getValue());
> + }
> + }
> + return canonicalizedHeaders;
> + }
> +
> + private String buildCanonicalizedHeadersString(Multimap<String, String> canonicalizedHeadersMap) {
> + StringBuilder canonicalizedHeadersBuffer = new StringBuilder();
> + for (Entry<String, String> header : canonicalizedHeadersMap.entries()) {
> + String key = header.getKey();
> + canonicalizedHeadersBuffer.append(String.format("%s:%s\n", key.toLowerCase(), header.getValue()));
Kind of an anti-pattern here, this compiles into another StringBuffer allocation. Instead:
```
canonicalizedHeadersBuffer.append(key.toLowerCase())
.append(':').append(header.getValue()).append('\n');
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12570927
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-aws #845](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-aws/845/) FAILURE
Looks like there's a problem with this pull request
[(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-aws/pull/3#issuecomment-43186858
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Ignasi Barrera <no...@github.com>.
> + <test.glacier.identity>${test.aws.identity}</test.glacier.identity>
> + <test.glacier.credential>${test.aws.credential}</test.glacier.credential>
> +
> + <jclouds.osgi.export>org.jclouds.glacier*;version="${project.version}"</jclouds.osgi.export>
> + <jclouds.osgi.import>
> + org.jclouds.labs*;version="${project.version}",
> + org.jclouds*;version="${project.version}",
> + *
> + </jclouds.osgi.import>
> + </properties>
> +
> + <dependencies>
> + <dependency>
> + <groupId>com.jamesmurty.utils</groupId>
> + <artifactId>java-xmlbuilder</artifactId>
> + <version>0.4</version>
Why is this dependency needed?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12469006
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #7](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/7/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#issuecomment-43187443
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #5](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/5/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#issuecomment-42784023
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Gaul <no...@github.com>.
> + this.utils = checkNotNull(utils);
> + }
> +
> + @Override
> + public HttpRequest filter(HttpRequest request) throws HttpException {
> + request = this.replaceDateHeader(request);
> + request = this.replaceHostHeader(request);
> + utils.logRequest(signatureLog, request, ">>");
> + request = this.signer.sign(request);
> + utils.logRequest(signatureLog, request, "<<");
> + return request;
> + }
> +
> + private HttpRequest replaceDateHeader(HttpRequest request) {
> + request = request.toBuilder().removeHeader(HttpHeaders.DATE).build();
> + request = request.toBuilder().replaceHeader(GlacierHeaders.ALTERNATE_DATE, timeStampProvider.get()).build();
Can you remove the duplicated toBuilder and build calls, e.g.,
```
request = request.toBuilder()
.removeHeader(...)
.replaceHeader(...)
.build()
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12570768
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Gaul <no...@github.com>.
BTW I am investigating using the V4 signer for aws-s3 which will future-proof jclouds for new Amazon regions since they will not support the older V2 signer.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#issuecomment-42927652
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Resource
> + @Named(Constants.LOGGER_SIGNATURE)
> + Logger signatureLog = Logger.NULL;
> +
> + private final Provider<String> timeStampProvider;
> + private final HttpUtils utils;
> +
> + @Inject
> + public RequestAuthorizeSignature(
> + @TimeStamp Provider<String> timeStampProvider,
> + @org.jclouds.location.Provider Supplier<Credentials> creds,
> + Crypto crypto, HttpUtils utils) {
> + this.signer = new AWSRequestSignerV4(creds.get().identity, creds.get().credential, crypto);
> + this.timeStampProvider = timeStampProvider;
> + this.utils = utils;
Add a `checkNotNull` to mandatory fields. Injection will already fail, but having the check will help when directly calling the constructor (for exampe in the tests).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12469091
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Gaul <no...@github.com>.
> +
> + public AWSRequestSignerV4(String identity, String credential, Crypto crypto) {
> + this.crypto = checkNotNull(crypto);
> + this.identity = checkNotNull(identity);
> + this.credential = checkNotNull(credential);
> + }
> +
> + private String buildHashedCanonicalRequest(String method, String endpoint, String hashedPayload,
> + String canonicalizedHeadersString, String signedHeaders) {
> + StringBuilder buffer = new StringBuilder();
> + buffer.append(method).append("\n");
> + buffer.append(endpoint).append("\n");
> + buffer.append("").append("\n");
> + buffer.append(canonicalizedHeadersString).append("\n");
> + buffer.append(signedHeaders).append("\n");
> + buffer.append(hashedPayload);
You can write this in a more compact form by reusing the return value of ```StringBuffer``` methods, e.g.,
```
String string = new StringBuffer()
.append(method).append("\n")
.append(endpoint).append("\n")
...
.build()
```
And actually this is equivalent to just using + for concatenation.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12570883
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #1](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/1/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#issuecomment-42612349
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-aws #820](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-aws/820/) FAILURE
Looks like there's a problem with this pull request
[(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-aws/pull/3#issuecomment-42615793
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Ignasi Barrera <no...@github.com>.
> + for (Entry<String, String> header : canonicalizedHeadersMap.entries()) {
> + String key = header.getKey();
> + canonicalizedHeadersBuffer.append(String.format("%s:%s\n", key.toLowerCase(), header.getValue()));
> + }
> + return canonicalizedHeadersBuffer.toString();
> + }
> +
> + private String buildSignedHeaders(Multimap<String, String> canonicalizedHeadersMap) {
> + StringBuilder signedHeadersBuffer = new StringBuilder();
> + for (Entry<String, String> header : canonicalizedHeadersMap.entries()) {
> + String key = header.getKey();
> + signedHeadersBuffer.append(key.toLowerCase()).append(";");
> + }
> + if(signedHeadersBuffer.length() > 0) {
> + signedHeadersBuffer.deleteCharAt(signedHeadersBuffer.length()-1);
> + }
Instead of manually building the string and removing the trailing ';', you could do something like:
```java
return Joiner.on(';').join(Iterables.transform(canonicalizedHeadersMap.keySet(), new Function<String, String>() {
@Override
public String apply(String input) {
return input.toLowerCase();
}
}));
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12469322
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #2](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/2/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#issuecomment-42615240
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Gaul <no...@github.com>.
> + properties.setProperty(PROPERTY_HEADER_TAG, GlacierHeaders.DEFAULT_AMAZON_HEADERTAG);
> + return properties;
> + }
> +
> + public static class Builder extends BaseRestApiMetadata.Builder<Builder> {
> +
> + @SuppressWarnings("deprecation")
> + protected Builder() {
> + super(GlacierClient.class, GlacierAsyncClient.class);
> + id("glacier")
> + .name("Amazon Glacier API")
> + .identityName("Access Key ID")
> + .credentialName("Secret Access Key")
> + .defaultEndpoint("https://glacier.us-east-1.amazonaws.com")
> + .documentation(URI.create("http://docs.aws.amazon.com/amazonglacier/latest/dev/amazon-glacier-api.html"))
> + .version("2012-06-01").defaultProperties(GlacierApiMetadata.defaultProperties()).context(CONTEXT_TOKEN)
Can you add a line break before the builder so that each call is on its own line?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12570730
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Phillips <no...@github.com>.
> That missing space is not in my code, should I change it?
Ah...whoops, indeed ;-) No, let's leave that for a separate PR, then. Thanks for clarifying ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#issuecomment-42913204
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Gaul <no...@github.com>.
> + buffer.append(credentialScope).append("\n");
> + buffer.append(hashedCanonicalRequest);
> + return buffer.toString();
> + }
> +
> + private String formatDateWithoutTimestamp(String date) {
> + return date.substring(0, 8);
> + }
> +
> + private String buildCredentialScope(String dateWithoutTimeStamp) {
> + StringBuilder buffer = new StringBuilder();
> + buffer.append(dateWithoutTimeStamp).append("/");
> + buffer.append(REGION).append("/");
> + buffer.append(SERVICE).append("/");
> + buffer.append(TERMINATION_STRING);
> + return buffer.toString();
Reuse result of ```StringBuffer.append``` instead of using ```buffer``` local variable.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12670375
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #3](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/3/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#issuecomment-42615875
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Gaul <no...@github.com>.
More nits, otherwise ready to commit. Any other comments @nacx and @demobox?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#issuecomment-43158659
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Ignasi Barrera <no...@github.com>.
It's good to see progress on this and that you're getting familiar with the jclouds internals. Don't get intimidated by the reviews!
Nice job @rcoedo!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#issuecomment-42649933
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-aws-pull-requests #6](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/6/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#issuecomment-42940443
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Phillips <no...@github.com>.
One minor [Checkstyle warning](https://jclouds.ci.cloudbees.com/job/jclouds-labs-aws-pull-requests/5/org.apache.jclouds.labs$aws-elb/violations/file/src/main/java/org/jclouds/aws/elb/AWSELBProviderMetadata.java/) (missing space after a comma, I think).
Thanks for the detailed reviews, @nacx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#issuecomment-42828569
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Ignasi Barrera <no...@github.com>.
> + } catch (IOException e) {
> + throw new HttpException("Error signing request", e);
> + }
> + return hashedPayload;
> + }
> +
> + private byte[] hmacSha256(byte[] key, String s) {
> + try {
> + Mac hmacSHA256 = crypto.hmacSHA256(key);
> + return hmacSHA256.doFinal(s.getBytes());
> + } catch (Exception e) {
> + throw new HttpException("Error signing request", e);
> + }
> + }
> +
> + @VisibleForTesting
The method is actually not visible for testing as it is private.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12469349
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Gaul <no...@github.com>.
> + public String apply(String input) {
> + return input.toLowerCase();
> + }
> + }));
> + }
> +
> + private String sha256(byte[] unhashedBytes) {
> + return Hashing.sha256().hashBytes(unhashedBytes).toString();
> + }
> +
> + private String buildHashedPayload(HttpRequest request) {
> + String hashedPayload = "";
> + try {
> + byte[] unhashedBytes = request.getPayload() == null ? "".getBytes() : ByteStreams.toByteArray(request
> + .getPayload().getInput());
> + hashedPayload = sha256(unhashedBytes);
Return the result of ```sha256``` instead of using the local variable.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12670350
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Gaul <no...@github.com>.
Tested locally, minor edits, and pushed to master. Good work @rcoedo and I look forward to the follow-on pull requests! :rocket:
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3#issuecomment-43188210
Re: [jclouds-labs-aws] Created the skeleton of the Glacier API. (#3)
Posted by Andrew Gaul <no...@github.com>.
> + private final Provider<String> timeStampProvider;
> + private final HttpUtils utils;
> +
> + @Inject
> + public RequestAuthorizeSignature(@TimeStamp Provider<String> timeStampProvider,
> + @org.jclouds.location.Provider Supplier<Credentials> creds, Crypto crypto, HttpUtils utils) {
> + checkNotNull(creds);
> + this.signer = new AWSRequestSignerV4(creds.get().identity, creds.get().credential, checkNotNull(crypto));
> + this.timeStampProvider = checkNotNull(timeStampProvider);
> + this.utils = checkNotNull(utils);
> + }
> +
> + @Override
> + public HttpRequest filter(HttpRequest request) throws HttpException {
> + request = this.replaceDateHeader(request);
> + request = this.replaceHostHeader(request);
As below, do you need to create multiple HttpRequests?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-aws/pull/3/files#r12570780