-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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 non default keystore and truststore type in presto CLI and JDBC #22556
Conversation
Suggest changing title, Description, release note entry, and commit message from "keystroke" to "keystore", based on the edited files and on the Desription in Issue 22555. |
@steveburnett changes done |
@@ -178,7 +180,7 @@ public static void setupSsl( | |||
// load TrustStore if configured, otherwise use KeyStore | |||
KeyStore trustStore = keyStore; | |||
if (trustStorePath.isPresent()) { | |||
trustStore = loadTrustStore(new File(trustStorePath.get()), trustStorePassword); | |||
trustStore = loadTrustStore(new File(trustStorePath.get()), trustStorePassword, trustStoreType.get()); |
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 add a validation to ensure that these parameters are present before getting.
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.
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.
Understood, but that's in a different module. Best practice is to either fail, or only pass in String (and check not null).
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.
done
c8610cf
to
3c67586
Compare
Looks good, but the commit message is too long (see contributing guide). Perhaps just mention |
3c67586
to
f59fefe
Compare
Changed commit message. |
f59fefe
to
e1343e7
Compare
@tdcmeehan Review comments are incorporated. Could you please review. |
Description
Added Support for non default keystore and truststore type in presto CLI and JDBC.
Motivation and Context
Issue : #22555
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.