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.

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

AmazonS3InstallationService add constructor #1168

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

AmazonS3InstallationService add constructor #1168

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3InstallationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.slack.api.bolt.service.builtin;

import com.amazonaws.auth.AWSCredentials;
import com.amazonaws.auth.BasicAWSCredentials;
import com.amazonaws.auth.DefaultAWSCredentialsProviderChain;
import com.amazonaws.services.s3.AmazonS3;
import com.amazonaws.services.s3.AmazonS3ClientBuilder;
Expand All @@ -14,25 +15,49 @@
import com.slack.api.bolt.service.InstallationService;
import com.slack.api.bolt.util.JsonOps;
import lombok.extern.slf4j.Slf4j;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
Comment on lines +18 to +19
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove these annotations? We consistently don't use them.

Copy link
Author

Choose a reason for hiding this comment

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

sure ,please do as you wish. I add them because I see them in the dependency graph


import java.io.IOException;
import java.util.Optional;

@Slf4j
public class AmazonS3InstallationService implements InstallationService {

@Nullable
private String region;
private String accessKey;
private String secretKey;

private final String bucketName;
private boolean historicalDataEnabled;

public AmazonS3InstallationService(@Nullable String region, @NotNull String accessKey, @NotNull String secretKey, @NotNull String bucketName) {
Copy link
Member

@seratch seratch Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Point : Let user control what is inputed .

this.region = region;
this.accessKey = accessKey;
this.secretKey = secretKey;
this.bucketName = bucketName;
}

public AmazonS3InstallationService(String bucketName) {
this.bucketName = bucketName;
}

@Override
public Initializer initializer() {
return (app) -> {
if (region != null) {
System.setProperty("AWS_REGION" , region);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the system prop name can be the same with env variable. The key should be "aws.region"

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

}

// The first access to S3 tends to be slow on AWS Lambda.
AWSCredentials credentials = getCredentials();
AWSCredentials credentials;
if (accessKey != null && secretKey != null) {
credentials = new BasicAWSCredentials(accessKey, secretKey);
} else {
credentials = getCredentials();
}

if (credentials == null || credentials.getAWSAccessKeyId() == null) {
throw new IllegalStateException("AWS credentials not found");
}
Expand Down