Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

Push local docker images and use relocation map #719

Merged
merged 13 commits into from
Nov 8, 2019

Conversation

eunomie
Copy link
Member

@eunomie eunomie commented Oct 28, 2019

- 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

  • check the help of docker app push command: should not contain option anymore
  • build an application that contains local images (like dockercoins example)
    • docker app build -t user/repo/dockercoins:push .
  • push it to a registry
    • docker app push user/repo/dockercoins:push
  • all images must be available in the registry after the push:
    • invocation image (not tagged anymore with -invoc)
    • service images that already exists in the registry (like if you reference a redis) must be mounted or copied (like before)
    • service images that were built by docker app build
  • drop all images built (or use an other machine)
    • for id in $(cat bundle.json | jq '.images | to_entries[] | .value.contentDigest | select(. != null)' | sed 's/"//g' | sed 's/sha256\://g'); do docker rmi -f $id; done
  • pull it from the registry
    • docker app pull user/repo/dockercoins:push
    • no dockercoins built images are pulled here, only bundle and relocation map
  • run it
    • docker app run user/repo/dockerocoins:push
    • images are pulled at runtime

- 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
Copy link

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
Copy link

codecov bot commented Oct 30, 2019

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% <ø> (ø) ⬆️
internal/packager/cnab.go 97.91% <ø> (ø) ⬆️
internal/commands/image/list.go 83.67% <ø> (ø) ⬆️
internal/cnab/driver.go 77.77% <0%> (-2.87%) ⬇️
internal/packager/bundle.go 64% <100%> (ø) ⬆️
internal/commands/run.go 61.53% <100%> (+0.86%) ⬆️
internal/store/installation.go 83.33% <100%> (+6.66%) ⬆️
internal/store/digest.go 84% <100%> (ø) ⬆️
internal/commands/image/tag.go 85% <100%> (ø) ⬆️
internal/commands/image/inspect.go 71.42% <50%> (-0.58%) ⬇️
... 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 eunomie force-pushed the relocation-push branch 3 times, most recently from 65b342b to db73c8b Compare October 30, 2019 10:51
@eunomie eunomie force-pushed the relocation-push branch 3 times, most recently from 43cf827 to c0aa853 Compare October 30, 2019 16:47
@eunomie eunomie changed the title [WIP] Push local docker images Push local docker images Oct 30, 2019
@eunomie eunomie marked this pull request as ready for review October 30, 2019 16:57
@eunomie eunomie closed this Oct 30, 2019
@eunomie eunomie reopened this Oct 30, 2019
@eunomie
Copy link
Member Author

eunomie commented Oct 30, 2019

note: cnab-to-oci dependency must be updated after cnabio/cnab-to-oci#78 is merged (and maybe released)

@eunomie eunomie changed the title Push local docker images [WIP] Push local docker images Oct 30, 2019
Gopkg.toml Outdated Show resolved Hide resolved
@eunomie eunomie force-pushed the relocation-push branch 4 times, most recently from ea52b57 to c877797 Compare October 31, 2019 10:27
@eunomie eunomie changed the title [WIP] Push local docker images Push local docker images Oct 31, 2019
@eunomie
Copy link
Member Author

eunomie commented Nov 7, 2019

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
Copy link
Member Author

eunomie commented Nov 7, 2019

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

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

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM

RunE: func(cmd *cobra.Command, args []string) error {
return runPush(dockerCli, firstOrEmpty(args), opts)
return runPush(dockerCli, firstOrEmpty(args))
Copy link
Contributor

Choose a reason for hiding this comment

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

can't be empty, so args[0]

return nil, errors.Wrapf(err, "failed to read bundle")
}

relocationMapFileName := filepath.Join(filepath.Dir(filename), "relocation-map.json")
Copy link
Contributor

Choose a reason for hiding this comment

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

"relocation-map.json" should be a const

Allow to push all images from the push/fixup command

Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
Only visible when trying to install it but still an issue

Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
By example we do not display the full reference anymore but use relocation map

Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
When a relocation map is generated from `cnab-to-oci` save it
within the same directory as the `bundle.json` file.

More information about reloaction map can be found in corresponding
`cnab-to-oci` issue: cnabio/cnab-to-oci#58

The `bundle.Bundle` struct is now wrapped in a `relocated.Bundle`
that will also contain the relocation map.

Methods to fetch `bundle.json` and `relocation-map.json` as well
as en entire bundle with the relocation map at once are moved to
a `fetch` package to avoid dependency cycle.

Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
Allow to replace local images by the one from the registry
using stored `relocation-map.json` when creating the installation.
The `bundle.json` is not modified.

Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
Be sure to remove the non needed lines.

Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
@eunomie eunomie force-pushed the relocation-push branch 5 times, most recently from 038e57d to 0f5dd8a Compare November 8, 2019 08:27
@rumpl rumpl merged commit 44932b6 into docker:master Nov 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants