docker / app

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

Build invocation image locally #456

Closed rumpl closed 5 years ago

rumpl commented 5 years ago

- What I did Made a new Dockerfile for building the invocation image locally

- How I did it Took the Dockerfile that existed and removed all the unnecessary targets and changed the first target to be the same as before we were cross-compiling the docker cli.

- How to verify it Run make invocation-image

- A picture of a cute animal (not mandatory but encouraged) github has problems uploading the image, here is an ASCII sloth, it's cute, promise

      `""==,,__  
        `"==..__"=..__ _    _..-==""_
             .-,`"=/ /\ \""/_)==""``
            ( (    | | | \/ |
             \ '.  |  \;  \ /
              |  \ |   |   ||
         ,-._.'  |_|   |   ||
        .\_/\     -'   ;   Y
       |  `  |        /    |-.
       '. __/_    _.-'     /'
   jgs        `'-.._____.-'
GordonTheTurtle commented 5 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 "feat-local-invocation-image" git@github.com:rumpl/app.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

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

codecov[bot] commented 5 years ago

Codecov Report

Merging #456 into master will increase coverage by 0.11%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #456      +/-   ##
==========================================
+ Coverage   63.45%   63.56%   +0.11%     
==========================================
  Files          56       56              
  Lines        2816     2830      +14     
==========================================
+ Hits         1787     1799      +12     
- Misses        772      773       +1     
- Partials      257      258       +1
Impacted Files Coverage Δ
internal/packager/registry.go 70.12% <0%> (-1.11%) :arrow_down:
internal/packager/cnab.go 100% <0%> (ø) :arrow_up:
internal/packager/init.go 54.81% <0%> (ø) :arrow_up:
cmd/docker-app/install.go 62.31% <0%> (+0.55%) :arrow_up:
cmd/docker-app/upgrade.go 23.07% <0%> (+1.5%) :arrow_up:
cmd/docker-app/bundle.go 47.61% <0%> (+4.51%) :arrow_up:

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 941df3f...9feeadb. Read the comment docs.

rumpl commented 5 years ago

Refactored things so that there is only one way to build the invocation image.

simonferquel commented 5 years ago

I thought that by convention, every target requiring a docker engine was in docker.Makefile. I find it a bit confusing. Why not having a separate dockerfile but keep the existing Makefile target ?

rumpl commented 5 years ago

@simonferquel You are right, I moved it to the other Makefile, thanks.

@chris-crone did the thing with the ARG too since I was changing code.

simonferquel commented 5 years ago

@simonferquel You are right, I moved it to the other Makefile, thanks.

@chris-crone did the thing with the ARG too since I was changing code.

Good the the makefile move, But for the ARG, you indeed gave it a value in vars.mk, but it is still not in use in the invocation-image dockerfile

rumpl commented 5 years ago

It's used here, and sent here.

simonferquel commented 5 years ago

Ah yes, don't we need to keep using the GO_VERSION though ?

rumpl commented 5 years ago

We switched to the dockercore/golang-cross image that docker cli uses for build and it doesn't need the GO_VERSION.

Note: this is only temporary until the docker cli with contexts is released.

simonferquel commented 5 years ago

Ok, LGTM then