docker / app

Make your Docker Compose applications reusable, and share them on Docker Hub
Apache License 2.0
1.57k stars 176 forks source link

Push local docker images and use relocation map #719

Closed eunomie closed 4 years ago

eunomie commented 4 years ago

- What I did

- How I did it

Integration of the latest cnab-to-oci that performs the push.

Note for the reviewer: the change is not that big, but contains multiple libraries bump so the diff is really big. Changes are in different commits to be easier to understand and review. Bump commits contains both bump and immediate changes to be able to compile. But e2e tests are updated in dedicated commits. One e2e test is still deactivated the time I found why it doesn't work well.

- How to verify it

- Description for the changelog

push command now pushes missing images from the local docker daemon image store to the registry. Options about platform and tags has been removed and must be defined in a previous step, like during build. CNAB schema is now in version 1.0.0.

GordonTheTurtle commented 4 years ago

Please sign your commits following these rules: https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work The easiest way to do this is to amend the last commit:

$ git clone -b "relocation-push" git@github.com:eunomie/app.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354601344
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

codecov[bot] commented 4 years ago

Codecov Report

Merging #719 into master will decrease coverage by 0.42%. The diff coverage is 76.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #719      +/-   ##
==========================================
- Coverage    70.9%   70.48%   -0.43%     
==========================================
  Files          61       62       +1     
  Lines        3221     3208      -13     
==========================================
- Hits         2284     2261      -23     
- Misses        625      641      +16     
+ Partials      312      306       -6
Impacted Files Coverage Δ
internal/bundle/parameters.go 86.51% <ø> (ø) :arrow_up:
internal/packager/cnab.go 97.91% <ø> (ø) :arrow_up:
internal/commands/image/list.go 83.67% <ø> (ø) :arrow_up:
internal/cnab/driver.go 77.77% <0%> (-2.87%) :arrow_down:
internal/packager/bundle.go 64% <100%> (ø) :arrow_up:
internal/commands/run.go 61.53% <100%> (+0.86%) :arrow_up:
internal/store/installation.go 83.33% <100%> (+6.66%) :arrow_up:
internal/store/digest.go 84% <100%> (ø) :arrow_up:
internal/commands/image/tag.go 85% <100%> (ø) :arrow_up:
internal/commands/image/inspect.go 71.42% <50%> (-0.58%) :arrow_down:
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a850314...0f5dd8a. Read the comment docs.

eunomie commented 4 years ago

note: cnab-to-oci dependency must be updated after https://github.com/docker/cnab-to-oci/pull/78 is merged (and maybe released)

eunomie commented 4 years ago

Branch is rebased on top of f141d47

eunomie commented 4 years ago

Rebased on top of 8acd0a838a7b0632f120fcfbb6443f897de62ad7

rumpl commented 4 years ago

It's a shame that cnab-go's bundle.Bundle also has a bunch of functions for read/write them. It makes our code seem clunky because we try to separate the bundle and the store.

I love especially the FromBundle(relocated.FromBundle(bndl)) which then does the dance where a relocated.Bundle is in fact a bundle.Bundle which is a io.WriterTo.

eunomie commented 4 years ago

It's a shame that cnab-go's bundle.Bundle also has a bunch of functions for read/write them. It makes our code seem clunky because we try to separate the bundle and the store.

I think the separation between the store and the bundle is a good thing. But not fully done so the result is weird here. It could be better to have bundle that is... a bundle. And a relocated bundle that is a bundle and the corresponding relocation map. And at an other place tools to read and write them. And bundle should not have a WriteTo method. It's a question of separation of concern and responsibilities.

But I think it's something we can fix/improve in an other PR, as it also requires changes in cnab-go.

eunomie commented 4 years ago

I love especially the FromBundle(relocated.FromBundle(bndl))

This line is no more, it's again a FromBundle(bndl)