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

Return image name and id from dockerPush and dockerBuildAndPush #117

Merged

Conversation

olofwalker
Copy link
Contributor

@olofwalker olofwalker commented Aug 11, 2020

This PR fixes issue #108

In short, the PR changes the return types of dockerBuildAndPush and dockerPush to return a Map[String,ImageId] instead.

This information is very useful for post build/push operations.

The `dockerPush` task now returns the SHA of the image.
`dockerPush` and `dockerBuild` now returns a map with the name and id of
the image.
@olofwalker olofwalker changed the title Return image id from docker push Return image name and id from dockerPush and dockerBuildAndPush Aug 11, 2020
@marcus-drake
Copy link
Owner

Thanks for the PR!

I would like to understand the use case better.
The return type have been changed to Map[ImageName,ImageId]. Will there be 2 or more values (ImageId) in the Map that are different? Or are they all the same as the output from the docker task?

@olofwalker
Copy link
Contributor Author

@marcuslonnberg Hej Markus!

Since dockerPush takes a sequence of images to push, I used a map to return the id of them all.

Not a sbt-docker expert so this is just my understanding how it was supposed to work.

@olofwalker
Copy link
Contributor Author

Ok, I see what you mean, I'll fix that.

Adding a new class for the return value from 'dockerPush'
@marcus-drake
Copy link
Owner

If I understand it correctly I think that one could currently use the plugin like this to achieve the same result:

myTask := {
  val imageId = dockerBuildAndPush.value
  val imageNames = (imageNames in docker).value

  // usage of imageId and imageNames
  ...
}

@olofwalker
Copy link
Contributor Author

@marcuslonnberg

Here is how I understand it.

The image id in docker parlance is the randomly generated ID assigned to images in Docker v1, it's still there and printed when running docker build. The SHA is a digest that is computed based on the image content to be able to verify the content.

The docker build function in sbt-docker returns either the image id or the digest, depending on the output.

This digest is however not the same digest returned from the push operation. The push operation modifies the image before uploading it. This SHA returned from docker push is the on you later can use to pull the image.

The PR also uses a map since there can be one or more images pushed.

Does that make sense?

@marcus-drake
Copy link
Owner

Thanks for the explanation! Now I see what you are out after.

Copy link
Owner

@marcus-drake marcus-drake left a comment

Choose a reason for hiding this comment

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

Looks good overall!

A few comments regarding the digest parsing.

src/main/scala/sbtdocker/DockerPush.scala Outdated Show resolved Hide resolved
@@ -44,5 +47,20 @@ object DockerPush {
val process = Process(command)
val exitValue = process ! processLog
if (exitValue != 0) sys.error("Failed to push")

val PushedImageId = ".* sha256:([0-9a-f]+) .*".r
Copy link
Owner

Choose a reason for hiding this comment

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

Since it is the image digest that is matched, the name of the constant should reflect that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes!

@@ -44,5 +47,20 @@ object DockerPush {
val process = Process(command)
val exitValue = process ! processLog
if (exitValue != 0) sys.error("Failed to push")

val PushedImageId = ".* sha256:([0-9a-f]+) .*".r
Copy link
Owner

Choose a reason for hiding this comment

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

It seems that Docker documentation refers to the digest with sha256: as a prefix. Shouldn't it therefore be included here in the ImageDigest output?

Documentation for ImageDigest should then be updated.

Copy link
Contributor Author

@olofwalker olofwalker Aug 14, 2020

Choose a reason for hiding this comment

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

Yes, I think so, we can do something like this:

/**
  * The image digest, the format of the digest is `algorithm:hex-string`
  * @param digest A digest of the image, as a string.
  * @param algorithm The algorithm used to produce the digest.
  */
final case class ImageDigest(digest: String, algorithm: String = "sha256") {
  override def toString = s"$algorithm:$digest"
}

Comment on lines 53 to 61
val imageId = lines.collect {
case PushedImageDigestSha256(digest) => ImageDigest("sha256", digest)
}.lastOption

imageId match {
case Some(id) =>
imageName -> id
case None =>
throw new DockerPushException("Could not parse Docker image id")
Copy link
Owner

Choose a reason for hiding this comment

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

This block refers to "image id" at some places can we update this to refer to "image digest" instead?

After this it can be merged

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!

@olofwalker
Copy link
Contributor Author

olofwalker commented Aug 14, 2020

@marcuslonnberg Is it possible to cut a new release when merged? We have our own fork now and that is less than ideal ...

@marcus-drake marcus-drake merged commit 5cb2f7d into marcus-drake:master Aug 16, 2020
@marcus-drake
Copy link
Owner

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.

2 participants