Skip to content

Adding support for S3 Control #1449

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 3, 2019
Merged

Adding support for S3 Control #1449

merged 2 commits into from
Oct 3, 2019

Conversation

spfink
Copy link
Contributor

@spfink spfink commented Oct 1, 2019 •
edited
Loading

Adds support for S3 Control

Testing

Unit and integration tests added.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed
  • A short description of the change has been added to the CHANGELOG
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@spfink spfink changed the title Finks/s3 control Adding support for S3 Control Oct 1, 2019
millems
millems reviewed Oct 1, 2019
View reviewed changes
bom/pom.xml Outdated
@@ -820,6 +820,11 @@
<artifactId>s3</artifactId>
<version>${awsjavasdk.version}</version>
</dependency>
<dependency>
<groupId>software.amazon.awssdk</groupId>
<artifactId>s3-control</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we verify this matches the one generated by the release automation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, will verify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appears that we remove spaces rather than replace with a dash looking at other services. Updated.

@codecov-io
Copy link

codecov-io commented Oct 2, 2019 •
edited
Loading

Codecov Report

Merging #1449 into master will increase coverage by 0.09%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1449      +/-   ##
============================================
+ Coverage      73.7%   73.79%   +0.09%     
- Complexity      660      684      +24     
============================================
  Files           844      847       +3     
  Lines         25886    25990     +104     
  Branches       1970     1992      +22     
============================================
+ Hits          19078    19179     +101     
- Misses         5947     5951       +4     
+ Partials        861      860       -1
Flag Coverage Δ Complexity Δ
#unittests 73.79% <80%> (+0.09%) 684 <24> (+24) ⬆️
Impacted Files Coverage Δ Complexity Δ
...k/codegen/poet/builder/BaseClientBuilderClass.java 79.25% <ø> (ø) 0 <0> (ø) ⬇️
.../amazon/awssdk/codegen/model/service/AuthType.java 100% <100%> (ø) 0 <0> (ø) ⬇️
...re/internal/http/pipeline/stages/SigningStage.java 86.36% <100%> (+0.64%) 0 <0> (ø) ⬇️
...sdk/services/s3control/S3ControlConfiguration.java 65% <65%> (ø) 6 <6> (?)
...ternal/interceptors/PayloadSigningInterceptor.java 80% <80%> (ø) 3 <3> (?)
...ernal/interceptors/EndpointAddressInterceptor.java 88.88% <88.88%> (ø) 15 <15> (?)
...tomization/CodegenCustomizationProcessorChain.java 81.81% <0%> (-8.19%) 0% <0%> (ø)
...on/awssdk/protocols/ion/AwsIonProtocolFactory.java 72.72% <0%> (-5.06%) 0% <0%> (ø)
.../software/amazon/awssdk/core/sync/RequestBody.java 89.28% <0%> (-3.31%) 0% <0%> (ø)
...ftware/amazon/awssdk/codegen/internal/Jackson.java 59.09% <0%> (-2.82%) 0% <0%> (ø)
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9bcf5b...b54f9e3. Read the comment docs.

bmaizels
bmaizels approved these changes Oct 2, 2019
View reviewed changes
Copy link
Contributor

@bmaizels bmaizels left a comment

Choose a reason for hiding this comment

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

Minor comments.

...3control/src/it/java/software.amazon.awssdk.services.s3control/S3ControlIntegrationTest.java
try {
client.getPublicAccessBlock(r -> r.accountId(INVALID_ACCOUNT_ID));
fail("Expected exception");
} catch (S3ControlException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use ExpectedException instead of all this boilerplate try/catch/fail stuff.

...3control/src/main/java/software/amazon/awssdk/services/s3control/S3ControlConfiguration.java Outdated
return dualstackEnabled;
}

private boolean resolveBoolean(Boolean customerSuppliedValue, boolean defaultValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming customerSuppliedValue to builderSuppliedValue or even just suppliedValue.

...3control/src/main/java/software/amazon/awssdk/services/s3control/S3ControlConfiguration.java Outdated
}

@NotThreadSafe
public interface Builder extends CopyableBuilder<Builder, S3ControlConfiguration> { // (8)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this comment mean?

...3control/src/main/java/software/amazon/awssdk/services/s3control/S3ControlConfiguration.java
@SdkPublicApi
@Immutable
@ThreadSafe
public final class S3ControlConfiguration implements ServiceConfiguration,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a toBuilder method?

@spfink spfink merged commit ae8f716 into master Oct 3, 2019
This was referenced Oct 3, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@millems millems millems left review comments

@bmaizels bmaizels bmaizels approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@spfink @codecov-io @millems @bmaizels