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

Add support for Server-Side Encryption via AWS Key Management Service (KMS) #12

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

KevinJMao
Copy link

No description provided.

Copy link

@muteor muteor left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Tests are failing for me, looks like they just need to be updated with the new S3Writer constructor signature.

if (bucket == null || bucket == "") {
throw new ConnectException("S3 bucket must be configured");
}
if (prefix == null) {
prefix = "";
}

String s3Region = config.get(S3SinkConnectorConstants.S3_REGION_CONFIG);
Copy link

Choose a reason for hiding this comment

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

Whats the default here? We probably want this to be optional, so dont set the region if there is not a config key.

import com.amazonaws.services.s3.model.PutObjectRequest;
import com.amazonaws.services.s3.model.S3Object;
import com.amazonaws.services.s3.model.S3ObjectInputStream;
import com.amazonaws.services.s3.model.*;
Copy link

Choose a reason for hiding this comment

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

Would prefer to keep explicit deps please.

import java.io.IOException;
import java.io.Reader;
import java.lang.StringBuilder;
import java.io.*;
Copy link

Choose a reason for hiding this comment

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

And here keep explicit deps.

iamnoah added a commit to iamnoah/kafka-connect-s3 that referenced this pull request Dec 11, 2020
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