appc / docker2aci

library and CLI tool to convert Docker images to ACIs (archived, see https://github.com/rkt/rkt/issues/4024)
Apache License 2.0
186 stars 60 forks source link

Annotate manifest hash #237

Closed cgonyeo closed 7 years ago

cgonyeo commented 7 years ago

There are two commits in here. In order they:

  1. Add an annotation to produced ACIs with a hash of the original docker manifest (or sometimes the app ID depending on the version of the HTTP API used)
  2. tweaks the library API so that callers can provide a list of manifest hashes they already have

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 https://github.com/coreos/rkt/issues/2937

cgonyeo commented 7 years ago

Some test is unhappy, I'll fix it on Monday

cgonyeo commented 7 years ago

Fixed the test, and added some comments to hopefully address the nits.

euank commented 7 years ago

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 rkt fetch docker://foo@sha256:.... and we annotated that, we can know that we don't need to do any network traffic at all.

cgonyeo commented 7 years ago

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 https://github.com/coreos/rkt/issues/2937 will hopefully go pretty quickly.

cgonyeo commented 7 years ago

This now should have the behavior I want, but https://github.com/appc/docker2aci/pull/238 should be merged first so I don't have to include that commit in this PR.

cgonyeo commented 7 years ago

Since @lucab helped me with vendoring, I just removed the vendoring commits and re-pushed this.

cgonyeo commented 7 years ago

Bump on this, I have a PR for rkt once this is done :)