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

Image deploy hooks #1881

Merged
merged 14 commits into from Dec 13, 2017

Conversation

Projects
None yet
3 participants
@ggarnier
Member

ggarnier commented Dec 11, 2017

Set deploy hooks when deploying from an image

@@ -172,16 +223,50 @@ func runCommandInContainer(client provision.BuilderDockerClient, evt *event.Even
}
waiter, err := client.AttachToContainerNonBlocking(attachOptions)
if err != nil {
return err
return "", err

This comment has been minimized.

@cezarsa

cezarsa Dec 12, 2017

Member

We should return cont.ID both here and after a StartContainer error to allow the container to be removed even after a failure. Another possibility is removing the container inside this function if we are to return an error.

return map[string]interface{}{
"healthcheck": yaml.Healthcheck,
"hooks": map[string]interface{}{

This comment has been minimized.

@cezarsa

cezarsa Dec 12, 2017

Member

I think having "hooks": yaml.Hooks, would work here.

path := defaultArchivePath + "/current"
cmd := fmt.Sprintf("(cat %s/tsuru.yml || cat %s/tsuru.yaml || cat %s/app.yml || cat %s/app.yaml || true) 2>/dev/null", path, path, path, path)
var buf bytes.Buffer
err := runCommandInContainer(client, evt, imageID, cmd, app, &buf, nil)
containerID, err := runCommandInContainer(client, evt, imageID, cmd, app, &buf, nil)

This comment has been minimized.

@cezarsa

cezarsa Dec 12, 2017

Member

What do you think about moving the defer RemoveContainer(containerID) call to this function instead of returning the ID and making the parent remove it?

This comment has been minimized.

@ggarnier

ggarnier Dec 12, 2017

Member

The problem is that I call runCommandInContainer also in runBuildHooks function, and I can't remove it there, because it's commited

This comment has been minimized.

@cezarsa

cezarsa Dec 12, 2017

Member

Oh right, you can only remove it after it's been pushed. 👍

repo, tag := splitImageName(imageID)
opts := docker.CommitContainerOptions{
Container: containerID,
Repository: repo,

This comment has been minimized.

@cezarsa

cezarsa Dec 12, 2017

Member

I think the only reason why Repository and Tag is needed here is because of a bug in docker-cluster, as it isn't storing information about the image if they are not present. Let's leave it like this for now but we may be able to simplify this in the future.

@ggarnier

This comment has been minimized.

Member

ggarnier commented Dec 12, 2017

@cezarsa PTAL

fmt.Fprintf(evt, " ---> Running %q\n", cmd)
containerID, err := runCommandInContainer(client, evt, imageID, cmd, app, evt, evt)
if err != nil {
return "", err

This comment has been minimized.

@cezarsa

cezarsa Dec 13, 2017

Member

Return containerID here to ensure removal in case of partial failure.

@ggarnier ggarnier merged commit 8afb354 into master Dec 13, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ggarnier ggarnier deleted the image-deploy-hooks branch Dec 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment