-
Notifications
You must be signed in to change notification settings - Fork 117
Support service account override #451
Support service account override #451
Conversation
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.
| .editOrNewSpec() | ||
| .withServiceAccount(account) | ||
| .withServiceAccountName(account) | ||
| .endSpec() |
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.
indent these in a couple spaces for each sub-item. Even though the methods are all straight chained, there's a structure we've been trying to keep clear through indentation
| }.getOrElse(driverSpec.driverPod) | ||
| }.getOrElse( | ||
| driverServiceAccount.map { | ||
| account => |
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.
move account onto previous line to match style in rest of this file
|
This logic implements a policy that kube secret credentials override service acct, and also prevent service acct from being applied. Is that the right policy? |
|
I think this is the right policy. |
|
I thought we had validation that you can't apply both secret creds and a service account -- you have to pick one or the other |
|
I think either is fine really, but we should have preference for the non-service account based authentication. |
|
I was imagining what amounts to supplementing permissions from one with the other, but that might be a corner case. |
|
It sounds like the current logic is restoring the original logic that was clobbered - LGTM |
|
It seems everybody is fine with this, although it isn't approved yet. Maybe we can approve and merge this soon. I also wonder if we need to have a dot release in case more people start to use Kubernetes 1.6. |
|
@foxish, you're assigned, do you want to merge? |
|
LGTM |
|
Thanks everyone for the reviews! |
Fixes #448.