Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

myniva/gradle-s3-build-cache#5: Adds option to use reduced redundancy… #6

Merged
merged 3 commits into from
Sep 26, 2017

Conversation

antillean
Copy link
Contributor

Adds option to use reduced redundancy storage, and sets it as the default. Adds tests for same. Removes failing test 'AwsS3BuildCacheServiceFactoryTest.testIllegalConfigWithInvalidRegion'.

…as the default. Adds tests for same. Removes failing test 'AwsS3BuildCacheServiceFactoryTest.testIllegalConfigWithInvalidRegion'.
Copy link
Owner

@myniva myniva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your pull request! It would be great if you could resolve the mentioned issues (even though they are all minor). I will merge and release a new version as soon as possible.

README.md Outdated
@@ -46,6 +46,7 @@ The AWS S3 build cache implementation requires two configuration options:
| ----------------- | ----------- |
| region | The AWS region the S3 bucket is located in. |
| bucket | The name of the AWS S3 bucket where cache objects should be stored. |
| reducedRedundancy | Whether or not to use [reduced redundancy](https://aws.amazon.com/s3/reduced-redundancy/). Enabled by default. |
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AWS S3 build cache implementation requires two configuration options (line 43)

This is no longer valid. What do you think about adding a new column "Default Value" and mentioning that all parameters which do not have a default are mandatory?

import static org.mockito.Mockito.*;

public class AwsS3BuildCacheServiceTest {
@Mock
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use 2 spaces for indentation like in the other classes.

import org.junit.Test;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this empty line.

import java.io.IOException;
import java.io.InputStream;

import static org.mockito.ArgumentMatchers.any;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move before non static imports.

@@ -0,0 +1,73 @@
package ch.myniva.gradle.caching.s3.internal;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add missing license header:

/*
 * Copyright 2017 the original author or authors.
 *
 * Licensed 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.
 */

@antillean
Copy link
Contributor Author

Sorry for being so late in making these changes! I must've missed the email notifications, and then forgot about this. >_>

Let me know if you'd like me to make other changes.

Also, it's worth mentioning: I've been using your current version (0.3.0) of this plugin -- so a version without this change -- and all the cache artefacts uploaded to the s3 bucket use the Reduced Redundancy storage class and I have no idea why. :/

@myniva myniva merged commit 9632a65 into myniva:develop Sep 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants