-
Notifications
You must be signed in to change notification settings - Fork 117
Extract SSL configuration handling to a separate class #123
Conversation
6fec6b6 to
1659810
Compare
|
Soft dependency on #122 to fix merge conflicts as the refactors are done in a chain. |
dd599d4 to
8c69aa9
Compare
1659810 to
4ba1f46
Compare
8c69aa9 to
d1d76b0
Compare
60ce1d9 to
292a862
Compare
d1d76b0 to
693d1ec
Compare
292a862 to
81463b8
Compare
|
@ash211 @foxish @iyanuobidele this had a soft dependency on #122 which is now merged - can we look to review and merge this one now? |
|
LGTM |
| service: Service, | ||
| driverSubmitSslOptions: SSLOptions): KubernetesSparkRestApi = { | ||
| val urlScheme = if (driverSubmitSslOptions.enabled) { | ||
| kubernetesSslConfiguration: KubernetesSslConfiguration): KubernetesSparkRestApi = { |
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.
nit: can we just call it SSLConfiguration? The prefix Kubernetes seems to be everywhere.
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
| val keyStoreURI = Utils.resolveURI(keyStore) | ||
| val isProvidedKeyStoreLocal = keyStoreURI.getScheme match { | ||
| case "file" | null => true | ||
| case "container" => false |
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.
I think container isn't supposed to work anymore after this change https://github.com/apache-spark-on-k8s/spark/pull/107/files#diff-b5527f236b253e0d9f5db5164bdb43e9R79
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 - changed to local
ash211
left a comment
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.
LGTM let's merge when the build is green
* Extract SSL configuration to a separate class * KubernetesSsl -> Ssl, container -> local
* Extract SSL configuration to a separate class * KubernetesSsl -> Ssl, container -> local
…on-k8s#123) * Extract SSL configuration to a separate class * KubernetesSsl -> Ssl, container -> local (cherry picked from commit 1f8fca0)
…on-k8s#123) * Extract SSL configuration to a separate class * KubernetesSsl -> Ssl, container -> local
More or less a drag-and-drop of all the SSL related logic.
Part of #109