You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2020/08/13 17:14:21 UTC
[GitHub] [camel] orpiske opened a new pull request #4089: Removes unreachable branch on camel-aws-s3 client implementations
orpiske opened a new pull request #4089:
URL: https://github.com/apache/camel/pull/4089
I was reviewing the S3 code and came across this code. It seems to be unreachable.
[x] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
[x] Each commit in the pull request should have a meaningful subject line and body.
[x] If you're unsure, you can format the pull request title like `[CAMEL-XXX] Fixes bug in camel-file component`, where you replace `CAMEL-XXX` with the appropriate JIRA issue.
[x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
[x] Run `mvn clean install -Psourcecheck` in your module with source check enabled to make sure basic checks pass and there are no checkstyle violations. A more thorough check will be performed on your pull request automatically.
Below are the contribution guidelines:
https://github.com/apache/camel/blob/master/CONTRIBUTING.md
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] gnodet commented on pull request #4089: Removes unreachable branch on camel-aws-s3 client implementations
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #4089:
URL: https://github.com/apache/camel/pull/4089#issuecomment-675915485
@oscerd I've merged the commit. There were 2 branches in a conditional if that could never be executed because they were written like:
```
if ([condition]) {
...
} else if (![condition]) {
...
} else {
...
}
```
So the last branch can never be executed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] orpiske commented on a change in pull request #4089: Removes unreachable branch on camel-aws-s3 client implementations
Posted by GitBox <gi...@apache.org>.
orpiske commented on a change in pull request #4089:
URL: https://github.com/apache/camel/pull/4089#discussion_r471538428
##########
File path: components/camel-aws-s3/src/main/java/org/apache/camel/component/aws/s3/client/impl/S3ClientIAMOptimizedImpl.java
##########
@@ -74,12 +74,14 @@ public AmazonS3 getS3Client() {
if (!configuration.isUseEncryption()) {
clientBuilder = AmazonS3ClientBuilder.standard().withCredentials(new InstanceProfileCredentialsProvider(false));
- } else if (configuration.isUseEncryption()) {
+
+ if (configuration.hasProxyConfiguration()) {
+ clientBuilder = clientBuilder.withClientConfiguration(clientConfiguration);
+ }
Review comment:
I have just moved this part of the PR to a separate commit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] orpiske commented on pull request #4089: Removes unreachable branch on camel-aws-s3 client implementations
Posted by GitBox <gi...@apache.org>.
orpiske commented on pull request #4089:
URL: https://github.com/apache/camel/pull/4089#issuecomment-675847103
@oscerd @gnodet Anything I can do to help with the review for this PR? If you want, I can describe my reasoning for the change and what, exactly, is the unreachable part.
Obs.: not trying to be pushy or anything like that. Just trying to learn if I should have done something better on the PR and if I can make your review easier.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] oscerd commented on pull request #4089: Removes unreachable branch on camel-aws-s3 client implementations
Posted by GitBox <gi...@apache.org>.
oscerd commented on pull request #4089:
URL: https://github.com/apache/camel/pull/4089#issuecomment-675916035
My bad...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] gnodet commented on a change in pull request #4089: Removes unreachable branch on camel-aws-s3 client implementations
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #4089:
URL: https://github.com/apache/camel/pull/4089#discussion_r471496007
##########
File path: components/camel-aws-s3/src/main/java/org/apache/camel/component/aws/s3/client/impl/S3ClientIAMOptimizedImpl.java
##########
@@ -74,12 +74,14 @@ public AmazonS3 getS3Client() {
if (!configuration.isUseEncryption()) {
clientBuilder = AmazonS3ClientBuilder.standard().withCredentials(new InstanceProfileCredentialsProvider(false));
- } else if (configuration.isUseEncryption()) {
+
+ if (configuration.hasProxyConfiguration()) {
+ clientBuilder = clientBuilder.withClientConfiguration(clientConfiguration);
+ }
Review comment:
This seems a slightly different issue. Should this be in a separate commit ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] oscerd commented on pull request #4089: Removes unreachable branch on camel-aws-s3 client implementations
Posted by GitBox <gi...@apache.org>.
oscerd commented on pull request #4089:
URL: https://github.com/apache/camel/pull/4089#issuecomment-675907955
I'll review again this when I come back
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] orpiske edited a comment on pull request #4089: Removes unreachable branch on camel-aws-s3 client implementations
Posted by GitBox <gi...@apache.org>.
orpiske edited a comment on pull request #4089:
URL: https://github.com/apache/camel/pull/4089#issuecomment-674928526
~Adjusting the commit ... an extra file was incorrectly added~ Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] orpiske commented on a change in pull request #4089: Removes unreachable branch on camel-aws-s3 client implementations
Posted by GitBox <gi...@apache.org>.
orpiske commented on a change in pull request #4089:
URL: https://github.com/apache/camel/pull/4089#discussion_r471503140
##########
File path: components/camel-aws-s3/src/main/java/org/apache/camel/component/aws/s3/client/impl/S3ClientIAMOptimizedImpl.java
##########
@@ -74,12 +74,14 @@ public AmazonS3 getS3Client() {
if (!configuration.isUseEncryption()) {
clientBuilder = AmazonS3ClientBuilder.standard().withCredentials(new InstanceProfileCredentialsProvider(false));
- } else if (configuration.isUseEncryption()) {
+
+ if (configuration.hasProxyConfiguration()) {
+ clientBuilder = clientBuilder.withClientConfiguration(clientConfiguration);
+ }
Review comment:
Absolutely! I can move this change to a separate commit. Let me modify this one and push again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] gnodet merged pull request #4089: Removes unreachable branch on camel-aws-s3 client implementations
Posted by GitBox <gi...@apache.org>.
gnodet merged pull request #4089:
URL: https://github.com/apache/camel/pull/4089
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] orpiske commented on pull request #4089: Removes unreachable branch on camel-aws-s3 client implementations
Posted by GitBox <gi...@apache.org>.
orpiske commented on pull request #4089:
URL: https://github.com/apache/camel/pull/4089#issuecomment-674928526
Adjusting the commit ... an extra file was incorrectly added.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org