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

Fix panic if listContainers fails… #443

Merged
merged 1 commit into from
Jun 9, 2016

Conversation

vdemeester
Copy link
Contributor

@vdemeester vdemeester commented Jun 8, 2016

… and also share context accross API call, as this is how it's meant to be used (and it allows to skip some calls if cancel is called).

Fixes #442

/cc @samber @emilevauge

PS: this need a little bit of testing though 👼

Signed-off-by: Vincent Demeester [email protected]

@vdemeester vdemeester added this to the 1.0 milestone Jun 8, 2016
@emilevauge
Copy link
Member

emilevauge commented Jun 8, 2016

@vdemeester entering light speed bug fixing

@vdemeester
Copy link
Contributor Author

Ok it's green, at least I did not break anything easily detected 😅
But still need some exercices 😸

@emilevauge
Copy link
Member

@vdemeester doesn't seem risky to me.
LGTM :)

@samber
Copy link
Contributor

samber commented Jun 8, 2016

Sharing context seems good but i think it's not enough. What about doing the append(.....) in an else condition in listContainers() ?

for _, container := range containerList {
        containerInspected, err := dockerClient.ContainerInspect(ctx, container.ID)
        if err != nil {
            log.Warnf("Failed to inpsect container %s, error: %s", container.ID, err)
        } else {
            containersInspected = append(containersInspected, containerInspected)
        }
    }

@vdemeester
Copy link
Contributor Author

@samber oh definitely, I thought it was in the else… I guess I needed a nap today 😃
I'll update 😉

… and also share context accross API call, as this is how it's meant to
be used (and it allows to skip some calls if `cancel` is called).

Signed-off-by: Vincent Demeester <[email protected]>
@emilevauge
Copy link
Member

@samber OK to merge this one ?

@samber
Copy link
Contributor

samber commented Jun 9, 2016

yes ;-)

@emilevauge emilevauge merged commit d1ffbd8 into traefik:master Jun 9, 2016
@vdemeester vdemeester deleted the 442-and-share-context branch June 9, 2016 07:43
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.

4 participants