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

new take on "Kompose will keep trying its job #477" #536

Merged
merged 2 commits into from
Apr 10, 2017

Conversation

kadel
Copy link
Member

@kadel kadel commented Apr 3, 2017

This is reopening of #477 there were some issues that we forgot to address.

Issue was that if it can't connect to cluster it panics.
Problem is that we continue after every error.
There should be return after some errors, and after every error in switch should be break so we don't
try to get reaper using client which creation field or calling Stop on reaper that field to create.

I've added new commit on top of original @surajnarwade's commit, so its easier to review.
Once we agreed that this is the way we wan't to do it we can squash it

surajnarwade and others added 2 commits April 3, 2017 18:19
fixes kubernetes#270

all errors in undeploy method(both kubernetes and openshift) are appended in a slice of errors,
and then it will be returned after successful deletion of all components.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 3, 2017
@kadel kadel requested review from cdrage and surajnarwade April 3, 2017 16:45
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

LGTM on the changes made (returning earlier)

}

client, namespace, err := k.GetKubernetesClient()
if err != nil {
return err
errorList = append(errorList, err)
return errorList
Copy link
Member

Choose a reason for hiding this comment

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

This is better! 👍

@cdrage
Copy link
Member

cdrage commented Apr 10, 2017

LGTM

@cdrage cdrage merged commit 27fc70a into kubernetes:master Apr 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants