Skip to content
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

properly handle external kube proxy case #2015

Merged

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Apr 30, 2015

@derekwaynecarr Can you see if this works properly for you?

@liggitt I'm still trying to see if there's a reasonable way for me write an integration test for the external kube case.

@smarterclayton
Copy link
Contributor

Start two masters, have one talk to the other?

@smarterclayton
Copy link
Contributor

Run a master in a container and have it talk to another master in a container in test-integration-docker?

@deads2k
Copy link
Contributor Author

deads2k commented Apr 30, 2015

Start two masters, have one talk to the other?

Tried this. We share an etcd in integration tests, so the controllers went ape-shit.

Run a master in a container and have it talk to another master in a container in test-integration-docker?

I'll look into this.

@derekwaynecarr
Copy link
Member

After chatting with David, this LGTM, but will wait for @liggitt to review in more detail.

@@ -69,10 +69,16 @@ func ValidateMasterConfig(config *api.MasterConfig) fielderrors.ValidationErrorL
if config.KubernetesMasterConfig != nil {
allErrs = append(allErrs, ValidateKubernetesMasterConfig(config.KubernetesMasterConfig).Prefix("kubernetesMasterConfig")...)
}
if (config.KubernetesMasterConfig == nil) && (len(config.MasterClients.ExternalKubernetesKubeConfig) == 0) {
allErrs = append(allErrs, fielderrors.NewFieldInvalid("kubernetesMasterConfig", config.KubernetesMasterConfig, "either kubernetesMasterConfig or masterClients.externalKubernetesKubeConfig must have a value"))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

is it invalid to give us externalKubernetesKubeConfig if you also gave us a kubernetesMasterConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@liggitt
Copy link
Contributor

liggitt commented Apr 30, 2015

question about validate, LGTM otherwise

@deads2k
Copy link
Contributor Author

deads2k commented May 1, 2015

Added another bit of validation. Not having a new test doesn't increase the tech debt (this fixes a bug, nothing new). Derek was hoping to have an image built tonight, so I'll [merge] and come back tomorrow for the test.

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1760/) (Image: devenv-fedora_1408)

@openshift-bot
Copy link
Contributor

Evaluated for origin up to da1980d

openshift-bot pushed a commit that referenced this pull request May 1, 2015
@openshift-bot openshift-bot merged commit d631772 into openshift:master May 1, 2015
@deads2k deads2k deleted the deads-respect-remote-kubeconfig branch May 1, 2015 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants