AmazonS3InstallationService add constructor#1168
AmazonS3InstallationService add constructor#1168smallufo wants to merge 1 commit intoslackapi:mainfrom
Conversation
|
Thanks for the contribution! Before we can merge this, we need @smallufo to sign the Salesforce Inc. Contributor License Agreement. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1168 +/- ##
============================================
- Coverage 75.25% 75.20% -0.06%
- Complexity 4004 4005 +1
============================================
Files 430 430
Lines 12528 12538 +10
Branches 1261 1262 +1
============================================
+ Hits 9428 9429 +1
- Misses 2355 2362 +7
- Partials 745 747 +2 ☔ View full report in Codecov by Sentry. |
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for submitting this PR, but we're yet unable to merge this PR as-is. There are a few things to consider:
- decide better constructor interface design
- need to do the same for AmazonS3OAuthStateService
- write some tests as much as possible
We may add built-in constructor for your use case in the near future (it may be based on this PR) but it's not our top priority right now. So, if you're already happy with your own implementation, please go ahead with it.
Thanks again for your time to work on this!
| import org.jetbrains.annotations.NotNull; | ||
| import org.jetbrains.annotations.Nullable; |
There was a problem hiding this comment.
Can you remove these annotations? We consistently don't use them.
There was a problem hiding this comment.
sure ,please do as you wish. I add them because I see them in the dependency graph
| public Initializer initializer() { | ||
| return (app) -> { | ||
| if (region != null) { | ||
| System.setProperty("AWS_REGION" , region); |
There was a problem hiding this comment.
I don't think the system prop name can be the same with env variable. The key should be "aws.region"
There was a problem hiding this comment.
Also, modifying the global system properties this way is not a generally recommended programming pratice. Instead, you can simply instantiate credential object to avoid affecting any other parts of your app.
There was a problem hiding this comment.
I saw different codes ("AWS_REGION" , "aws.region") in the whole AWS code base , I am not familiar with this (searching environment / system variables). I am not also favor of this type.
You can modify to the correct variable name
| private final String bucketName; | ||
| private boolean historicalDataEnabled; | ||
|
|
||
| public AmazonS3InstallationService(@Nullable String region, @NotNull String accessKey, @NotNull String secretKey, @NotNull String bucketName) { |
There was a problem hiding this comment.
Having four string arguments is not a very easy-to-use interface. more importantly this is not flexible enough for future enhancement. I think receiving a single object representing crendential inputs (either AWS SDK's class or a custom class with builder interface) + bucketName works better for this use case.
There was a problem hiding this comment.
OK, depends on you. Just provide a constructor for user to prepare necessary values/keys. Rather than deep digging the system looking for variables.
This also eases testing / mocking.
There was a problem hiding this comment.
Point : Let user control what is inputed .
seratch
added
enhancement
acharyashashank
left a comment
acharyashashank
left a comment
There was a problem hiding this comment.
Can we also think to migrate to newer version of AWS SDK v2? I have attached the screenshot of official announcement from Amazon SDK Github: https://github.com/aws/aws-sdk-java
|
@acharyashashank Oh, thanks for letting us know this! We'll resolve the issue separtely and ship a new minor version that upgrades the version as early as possible. |
|
This PR is outdated and #1419 will provide a better customization option for developers; so let me close this one now. Thank you so much again for taking the time to make this PR! |
3 participants
This pull request may resolve #1165
Category (place an
xin each of the[ ])Requirements
Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.