-
Notifications
You must be signed in to change notification settings - Fork 536
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
builder,provision: adding the latest tag to deploy image #1973
Conversation
for i, tag := range tags { | ||
opts := docker.CommitContainerOptions{Container: c.ID, Repository: repository, Tag: tag} | ||
done := limiter.Start(c.HostAddr) | ||
image, err := client.CommitContainer(opts) |
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.
Calling client.CommitContainer
for each tag is not necessary and it may cause a performance hit, it should be called only once (for the original tag), after the Commit is done we should call client.TagImage
to tag the latest tag. This will also allow us to move the commit logic and the image size logic to outside the for loop (removing the ugly if i == 0
block).
Only the PushImage
logic should be inside this for loop.
provision/kubernetes/deploy.go
Outdated
@@ -140,6 +141,21 @@ func createDeployPod(params createPodParams) error { | |||
} | |||
} | |||
params.cmds = cmds | |||
destinationImageWithoutTag := params.destinationImage | |||
parts := strings.Split(params.destinationImage, ":") |
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.
You should use the more robust image.SplitImageName
(in the app/image
pkg) function to split the image as it already handle some corner cases.
provision/kubernetes/deploy.go
Outdated
echo " ---> Sending image to repository (${sz})" | ||
docker push "${img}" | ||
`, params.destinationImage, baseName, buildIntercontainerStatus, buildIntercontainerDone), | ||
strings.Join( |
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.
Nitpick, moving this line and the line below it to the line above should allow us to preserve the old indentation level shortening the diff for this PR. Something like:
"sh", "-ec", strings.Join(append([]string{fmt.Sprintf(`
...
`, ...
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.
I've just noticed that this PR is missing changes to the swarm provisioner. It'll be similar to the docker provisioner but somewhere around
tsuru/provision/swarm/docker.go
Lines 178 to 195 in 82b0563
func commitPushBuildImage(client *docker.Client, img, contID string, app provision.App) (string, error) { | |
parts := strings.Split(img, ":") | |
repository := strings.Join(parts[:len(parts)-1], ":") | |
tag := parts[len(parts)-1] | |
_, err := client.CommitContainer(docker.CommitContainerOptions{ | |
Container: contID, | |
Repository: repository, | |
Tag: tag, | |
}) | |
if err != nil { | |
return "", errors.WithStack(err) | |
} | |
err = dockercommon.PushImage(client, repository, tag, dockercommon.RegistryAuthConfig()) | |
if err != nil { | |
return "", err | |
} | |
return img, nil | |
} |
provision/swarm/provisioner_test.go
Outdated
@@ -1095,6 +1095,12 @@ func (s *S) TestDeploy(c *check.C) { | |||
err := app.CreateApp(a, s.user) | |||
c.Assert(err, check.IsNil) | |||
attached := s.attachRegister(c, s.clusterSrv, true, a) | |||
tags := []string{} | |||
s.clusterSrv.CustomHandler("/images/.*/push", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
c.Assert(r.URL.Path, check.Equals, "/images/"+fmt.Sprintf("registry.tsuru.io/tsuru/app-myapp")+"/push") |
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.
No need for Sprintf() and concat here, just make it a single string.
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.
true. fixed that
This PR is related to #1945. Now, whenever you deploy a new version of your app, you can pull the last image using the tag
latest
.