Annotate manifest hash #237
Conversation
cgonyeo
force-pushed
the
annotate-manifest-hash
branch
from
29f2b5d to
f461ffd
Compare
|
Some test is unhappy, I'll fix it on Monday |
| @@ -53,15 +53,15 @@ func NewFileBackend(file *os.File, debug, info log.Logger) *FileBackend { | |||
| } | |||
| } | |||
|
|
|||
| func (lb *FileBackend) GetImageInfo(dockerURL string) ([]string, *common.ParsedDockerURL, error) { | |||
| func (lb *FileBackend) GetImageInfo(dockerURL string) ([]string, string, *common.ParsedDockerURL, error) { | |||
There was a problem hiding this comment.
this return signature is getting messy, it is not obvious what []string, string is. Can we document or name the return values?
| @@ -53,13 +53,13 @@ import ( | |||
| // BuildACI takes a Docker layer, converts it to ACI and returns its output | |||
| // path and its converted ImageManifest. | |||
| type Docker2ACIBackend interface { | |||
| GetImageInfo(dockerUrl string) ([]string, *common.ParsedDockerURL, error) | |||
| BuildACI(layerIDs []string, dockerURL *common.ParsedDockerURL, outputDir string, tmpBaseDir string, compression common.Compression) ([]string, []*schema.ImageManifest, error) | |||
| GetImageInfo(dockerUrl string) ([]string, string, *common.ParsedDockerURL, error) | |||
There was a problem hiding this comment.
same comment applies here as above. can we document what []string, string are?
| @@ -346,7 +347,7 @@ func GenerateEmptyManifest(name string) (*schema.ImageManifest, error) { | |||
| }, nil | |||
| } | |||
|
|
|||
| func GenerateManifestV22(name string, dockerURL *common.ParsedDockerURL, config *typesV2.ImageConfig, imageDigest string, lowerLayers []*schema.ImageManifest, debug log.Logger) (*schema.ImageManifest, error) { | |||
| func GenerateManifestV22(name, manhash string, dockerURL *common.ParsedDockerURL, config *typesV2.ImageConfig, imageDigest string, lowerLayers []*schema.ImageManifest, debug log.Logger) (*schema.ImageManifest, error) { | |||
There was a problem hiding this comment.
woah, this is getting immense ;-) at least we could make this a newline separated arglist and squash imageDigest with name, manhash. Also some documentation would welp. I am becoming concerned about the size of this arglist.
cgonyeo
force-pushed
the
annotate-manifest-hash
branch
2 times, most recently
from
6676468 to
651980b
Compare
|
Fixed the test, and added some comments to hopefully address the nits. |
|
I'd prefer using the upstream 'sha256' hash if we can since it'll make things a bit nicer for the user to read. It also has one small benefit! If the user does |
|
I agree with @euank, so please don't merge this until I can update that logic. I've got the rkt PR based on this ready though, so as soon as this goes in the rest of rkt/rkt#2937 will hopefully go pretty quickly. |
cgonyeo
force-pushed
the
annotate-manifest-hash
branch
from
651980b to
8de20ca
Compare
|
This now should have the behavior I want, but #238 should be merged first so I don't have to include that commit in this PR. |
This commit notes the sha256 hash of manifests when fetching images for v2.1 or v2.2, and records either this or the app ID in an annotation in the manifest of the produced ACI.
This commit allows a user of the library to provide a list of manifest hashes, presumably generated from annotations left on images by prior invocations of docker2aci, for images it already has. If docker2aci fetches a manifest and the hash for it matches one of the elements in the user provided list, docker2aci will stop working, and return with no error and no produced ACIs.
cgonyeo
force-pushed
the
annotate-manifest-hash
branch
from
8de20ca to
065228f
Compare
|
Since @lucab helped me with vendoring, I just removed the vendoring commits and re-pushed this. |
|
Bump on this, I have a PR for rkt once this is done :) |
There are two commits in here. In order they:
These two changes should allow for rkt to provide a list of docker manifest hashes (obtained by ACI manifest annotations) for images it already has but wants docker2aci to check for an update for. If docker2aci pulls the manifest for an image and it's in the list of images already owned by the caller, it doesn't have to download the image.
This is part of the work for rkt/rkt#2937