-
Notifications
You must be signed in to change notification settings - Fork 118
Support custom labels on the driver pod. #27
Conversation
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'd also like to see a test ensuring that these make it out to the resulting pods (if minikube supports that).
And if you pass an empty value, does it behave as expected?
<td>(none)</td> | ||
<td> | ||
Custom labels that will be added to the driver pod. This should be a comma-separated list of label key-value pairs, | ||
where each label is in the format <code><key>=<value></code>. |
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.
get rid of the < -- I think just <code>key=value</code>
is clear enough
@@ -184,7 +188,7 @@ private[spark] class Client( | |||
kubernetesClient.pods().createNew() | |||
.withNewMetadata() | |||
.withName(kubernetesAppId) | |||
.withLabels(selectors) | |||
.addToLabels(resolvedSelectors) |
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.
ok to make this method change here but not in other places?
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.
In this context it's fine but this change is accidental and probably should be reverted to reduce the unnecessary diff.
@ash211 good call - tests caught some subtle mistakes in the logic. Fixed all the comments. |
assert(driverPodLabels.get("label2") == "label2value", "Unexpected value for label2") | ||
} | ||
|
||
test("Run driver with custom labels") { |
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.
Ah this is a mistake in how the tests were adjusted.
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 -- @foxish thoughts?
LGTM. |
* Support custom labels on the driver pod. * Add integration test and fix logic. * Fix tests * Fix minor formatting mistake * Reduce unnecessary diff
* Support custom labels on the driver pod. * Add integration test and fix logic. * Fix tests * Fix minor formatting mistake * Reduce unnecessary diff
* Support custom labels on the driver pod. * Add integration test and fix logic. * Fix tests * Fix minor formatting mistake * Reduce unnecessary diff
* Support custom labels on the driver pod. * Add integration test and fix logic. * Fix tests * Fix minor formatting mistake * Reduce unnecessary diff
* Support custom labels on the driver pod. * Add integration test and fix logic. * Fix tests * Fix minor formatting mistake * Reduce unnecessary diff
Closes #26.