-
Notifications
You must be signed in to change notification settings - Fork 11
Migrate to AWS SDK v2 #21
base: main
Are you sure you want to change the base?
Conversation
|
@debanshuk I don't know if you received notifications about this PR, could you please review it? |
|
@debanshuk Gently reminder :) |
|
@KevinTabary, Thanks for the PR. I'll try to review it by EOW. |
There was a problem hiding this 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.
| 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). |
There was a problem hiding this comment.
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.
| implementation 'software.amazon.awssdk:kms:2.30.18' | ||
| implementation 'commons-cli:commons-cli:1.9.0' | ||
| implementation 'commons-codec:commons-codec:1.18.0' |
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.*; |
There was a problem hiding this comment.
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.
Friendly nudge on this. |
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