Skip to content
This repository was archived by the owner on Jan 9, 2020. It is now read-only.

Support custom labels on the driver pod. #27

Merged
merged 5 commits into from
Jan 19, 2017

Conversation

mccheah
Copy link

@mccheah mccheah commented Jan 17, 2017

Closes #26.

Copy link

@ash211 ash211 left a 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>&ltkey&gt=&ltvalue&gt</code>.
Copy link

Choose a reason for hiding this comment

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

get rid of the &lt -- 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)
Copy link

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?

Copy link
Author

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.

@mccheah
Copy link
Author

mccheah commented Jan 17, 2017

@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") {
Copy link
Author

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.

Copy link

@ash211 ash211 left a comment

Choose a reason for hiding this comment

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

LGTM -- @foxish thoughts?

@foxish
Copy link
Member

foxish commented Jan 19, 2017

LGTM.

@ash211 ash211 merged commit fd84fca into k8s-support-alternate-incremental Jan 19, 2017
@ash211 ash211 deleted the custom-labels branch January 19, 2017 01:30
ash211 pushed a commit that referenced this pull request Feb 8, 2017
* Support custom labels on the driver pod.

* Add integration test and fix logic.

* Fix tests

* Fix minor formatting mistake

* Reduce unnecessary diff
ash211 pushed a commit that referenced this pull request Mar 8, 2017
* Support custom labels on the driver pod.

* Add integration test and fix logic.

* Fix tests

* Fix minor formatting mistake

* Reduce unnecessary diff
foxish pushed a commit that referenced this pull request Jul 24, 2017
* Support custom labels on the driver pod.

* Add integration test and fix logic.

* Fix tests

* Fix minor formatting mistake

* Reduce unnecessary diff
ifilonenko pushed a commit to ifilonenko/spark that referenced this pull request Feb 25, 2019
ifilonenko pushed a commit to ifilonenko/spark that referenced this pull request Feb 25, 2019
* Support custom labels on the driver pod.

* Add integration test and fix logic.

* Fix tests

* Fix minor formatting mistake

* Reduce unnecessary diff
puneetloya pushed a commit to puneetloya/spark that referenced this pull request Mar 11, 2019
* Support custom labels on the driver pod.

* Add integration test and fix logic.

* Fix tests

* Fix minor formatting mistake

* Reduce unnecessary diff
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.

3 participants