Skip to content
This repository was archived by the owner on Oct 31, 2025. It is now read-only.

Conversation

@KevinTabary
Copy link

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

This PR will somehow fix the issue raised here: #19

@KevinTabary
Copy link
Author

@debanshuk I don't know if you received notifications about this PR, could you please review it?

@KevinTabary
Copy link
Author

@debanshuk Gently reminder :)

@debanshuk
Copy link
Contributor

@KevinTabary, Thanks for the PR. I'll try to review it by EOW.

Copy link
Contributor

@debanshuk debanshuk left a comment

Choose a reason for hiding this comment

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

@KevinTabary, Published a few comments. Apart from those, changes look good to me. Kindly address the comments.

Comment on lines -14 to +17
1. Asymmetric or Symmetric encryption (RSA or ECDSA based for asymmetric keys and AES based for symmetric keys).
2. Asymmetric or Symmetric encryption (RSA or ECDSA based for asymmetric keys and AES based for symmetric keys).
1. Classes: `com.nimbusds.jose.aws.kms.crypto.KmsDefaultEncrypter`
and `com.nimbusds.jose.aws.kms.crypto.KmsDefaultDecrypter`
1. Asymmetric signing (RSA or ECDSA based).
3. Asymmetric signing (RSA or ECDSA based).
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an unnecessary change and should be avoided.

Comment on lines +35 to +37
implementation 'software.amazon.awssdk:kms:2.30.18'
implementation 'commons-cli:commons-cli:1.9.0'
implementation 'commons-codec:commons-codec:1.18.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using exact versions, can we use ranges here?

// Use JUnit Jupiter for testing.
testImplementation 'org.junit.jupiter:junit-jupiter:5.+'
testImplementation 'org.assertj:assertj-core:[3,4)'
testImplementation 'org.junit.platform:junit-platform-launcher:1.11.4'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed? Usually we shouldn't use the junit-platform-* dependencies directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a backward incompatible change, please also bump up the major version in this file.

import lombok.var;
import software.amazon.awssdk.core.SdkBytes;
import software.amazon.awssdk.services.kms.KmsClient;
import software.amazon.awssdk.services.kms.model.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid using * imports in source files.

@debanshuk
Copy link
Contributor

debanshuk commented May 6, 2025

@KevinTabary, Published a few comments. Apart from those, changes look good to me. Kindly address the comments.

Friendly nudge on this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants