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

Introduce CNAB core commands #443

Merged
merged 14 commits into from
Jan 31, 2019

Conversation

silvin-lubecki
Copy link
Contributor

- What I did
docker-app is now a CNAB client. It can install any CNAB bundle.
I added then the 4 core CNAB commands:

  • install
  • uninstall
  • status
  • upgrade
    But also a new command bundle to build a CNAB bundle from a Docker Application Package.
    This PR introduces a new binary run which is the new docker-app CNAB "backend". It is executed inside the invocation image and answers to the 4 CNAB actions.

Note This PR purpose is to clean the cnab-preview branch and introduce the CNAB commands to docker-app. Some things still needs to be done in multiple follow-ups (and to avoid a mega PR, this one is big enough):

  • rewrite Push/Pull
  • Convert inspect command as a CNAB custom action

- How to verify it

$ docker  context create swarm --description "swarm context" --default-stack-orchestrator=swarm --docker=host=unix:///var/run/docker.sock
swarm
Successfully created context "swarm"

$ docker context ls
NAME                DESCRIPTION                               DOCKER ENDPOINT               KUBERNETES ENDPOINT   ORCHESTRATOR
default             Current DOCKER_HOST based configuration
swarm *             swarm context                             unix:///var/run/docker.sock                         swarm

$ docker-app install examples/hello-world/hello-world.dockerapp --name hello --target-context=swarm
Creating network hello_default
Creating service hello_hello

$ export DOCKER_TARGET_CONTEXT=swarm

$ docker-app status hello
ID                  NAME                MODE                REPLICAS            IMAGE                        PORTS
0m1wn7jrgkgj        hello_hello         replicated          1/1                 hashicorp/http-echo:latest   *:8080->5678/tcp

$ docker-app uninstall hello
Removing service hello_hello
Removing network hello_default

- Description for the changelog

  • CNAB introduction, adding install/uninstall/status/upgrade and bundle commands

- A picture of a cute animal (not mandatory but encouraged)
image

In future commits, the docker-app binary will be mutated as a frontend, and all the installation logic will be moved to the run binary.
run comes with the 4 CNAB core commands: install/uninstall/status/upgrade.

Adds github.com/deislabs/duffle as vendoring

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
@simonferquel
Copy link
Contributor

From CI:

  • 1 test is failing
  • there is a bug in the tag generation, which is not the same depending on the git client version. I think we fixed that in the pre-announcement repo (there is a way to explicitly fix the number of characters coming from the commit SHA we want in the tag)

[[override]]
name = "github.com/deislabs/duffle"
branch = "dockercon-version"
source = "github.com/silvin-lubecki/duffle"
Copy link
Contributor

Choose a reason for hiding this comment

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

What is missing from the duffle upstream exactly ? Can we list that and make appropriate PRs ?

Gopkg.toml Outdated Show resolved Hide resolved
cmd/docker-app/bundle.go Outdated Show resolved Hide resolved
cmd/docker-app/install.go Show resolved Hide resolved
docker.Makefile Outdated Show resolved Hide resolved
@silvin-lubecki silvin-lubecki force-pushed the add-cnab-core-commands branch 4 times, most recently from 4740985 to c10d616 Compare January 17, 2019 10:29
Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Really went quickly but question: what is cmd/run package ? (doesn't seem to get imported) — or is in the invocation image ? (if yes, the name.. well.. I don't like it — cnap-run or cnab-invoke or …)

README.md Outdated
merge Merge a multi-file application into a single file
push Push the application to a registry
render Render the Compose file for the application
split Split a single-file application into multiple files
status Get an application status
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between inspect and status ?

Copy link
Member

Choose a reason for hiding this comment

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

inspect is run on an at rest bundle, status gives information about an installed application.

If this isn't clear, we should make sure the help makes it so.

if err := bndl.Validate(); err != nil {
return err
}
h := duffleHome()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is room for extracting some code here in testable function 👼

Jenkinsfile.baguette Outdated Show resolved Hide resolved
@silvin-lubecki silvin-lubecki force-pushed the add-cnab-core-commands branch 3 times, most recently from 880cb56 to 717b21b Compare January 17, 2019 14:43
Copy link
Member

@chris-crone chris-crone left a comment

Choose a reason for hiding this comment

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

Just done to the end of the README thus far

echo "Releasing $TAG_NAME"
dir('bin') {
release('docker/app')
parallel {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to parallelise the release? Might add complexity for cleaning up if things go wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

dir('src/github.com/docker/app') {
checkout scm
sh 'make -f docker.Makefile load-invocation-image'
Copy link
Member

Choose a reason for hiding this comment

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

We must be sure to clean up the invocation image after each run.

README.md Show resolved Hide resolved
@@ -6,7 +6,7 @@ An *experimental* utility to help make Compose files more reusable and sharable.

You can find some preview binaries of `docker-app` with CNAB support [here](https://github.com/docker/app/releases/tag/cnab-dockercon-preview).
Copy link
Member

Choose a reason for hiding this comment

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

Must remember to remove this when we next release.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
swarm * swarm context unix:///var/run/docker.sock swarm
```

Here is an example installing an application package, querying a status and then uninstalling it:
Copy link
Member

Choose a reason for hiding this comment

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

@garethr How do we want to ensure that people aren't confused by CNAB vs app package? Should we call it an application bundle or even just application?

README.md Outdated Show resolved Hide resolved
README.md Outdated
merge Merge a multi-file application into a single file
push Push the application to a registry
render Render the Compose file for the application
split Split a single-file application into multiple files
status Get an application status
Copy link
Member

Choose a reason for hiding this comment

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

inspect is run on an at rest bundle, status gives information about an installed application.

If this isn't clear, we should make sure the help makes it so.

@silvin-lubecki
Copy link
Contributor Author

@vdemeester cmd/run package is a new binary, embedded in the cnab-base image. It is copied here https://github.com/docker/app/pull/443/files#diff-3254677a7917c6c01f55212f86c57fbfR52 . The name comes from the CNAB standard, as the entrypoint needs to be /cnab/app/run.
That said, you are right the name is terrible, and we could rename it while we copy it.

@rumpl
Copy link
Member

rumpl commented Jan 17, 2019

The lines

cmd.Flags().StringVar(&opts.targetContext, "target-context", "", "Context on which to request the application status")
cmd.Flags().StringArrayVarP(&opts.credentialsets, "credential-set", "c", []string{}, "Use a duffle credentialset (either a YAML file, or a credential set present in the duffle credential store)")

Are repeated multiple times, maybe we could refactor it at some time

@silvin-lubecki
Copy link
Contributor Author

Fixed @rumpl PTAL

@silvin-lubecki
Copy link
Contributor Author

@vdemeester cmd/run renamed to cmd/cnab-run. PTAL 🐱

@silvin-lubecki silvin-lubecki force-pushed the add-cnab-core-commands branch 2 times, most recently from 6d2620c to 156d992 Compare January 18, 2019 15:45
@silvin-lubecki silvin-lubecki force-pushed the add-cnab-core-commands branch 2 times, most recently from 81b003a to 56f56fb Compare January 21, 2019 13:55
@codecov
Copy link

codecov bot commented Jan 31, 2019

Codecov Report

Merging #443 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #443   +/-   ##
=======================================
  Coverage   58.46%   58.46%           
=======================================
  Files          57       57           
  Lines        2889     2889           
=======================================
  Hits         1689     1689           
  Misses        981      981           
  Partials      219      219

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 e69b627...196387f. Read the comment docs.

* Fix tag generation by truncating the git sha.
* Fix saving cnab base image to a global tmp directory and put it in a local git ignored directory (_build).
* Remove the cnab base image from the CI local daemon
* Load invocation image directly instead of using a make target.

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
@silvin-lubecki silvin-lubecki force-pushed the add-cnab-core-commands branch 3 times, most recently from a616ac8 to 1caad68 Compare January 31, 2019 15:35
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Install command also detects if the argument is a Docker Application Package. It will then bundle it and run the CNAB action Install.
It is now a CNAB installer, and all the docker-app deployment logic is moved to the docker-app backend binary run in cmd/run/install.go.
The installation is added in the duffle claim store.

* Rename deploy command to install (with still alias to deploy)

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
It will run the Uninstall CNAB action in the cmd/run/uninstall.go to remove a Docker Application Package deployment.

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
For a Docker Application Package, it will run a "docker stack services" on the deployed stack.

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
*Fix running docker-app validate only on application package examples and skip CNAB examples.

Signed-off-by: Christopher Crone <christopher.crone@docker.com>
…xt has not been specified.

Factorize credentialset and target-context flags.
Extract all helper functions to their own go file.

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
@silvin-lubecki
Copy link
Contributor Author

I squashed the as much commits as I could, it's ready to be merged as I think every comments has been tackled.

internal/packager/packing.go Outdated Show resolved Hide resolved
@silvin-lubecki
Copy link
Contributor Author

@simonferquel @vdemeester can you update your review? 🐱

Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

SGTM 😉

@simonferquel simonferquel merged commit 6c84a2b into docker:master Jan 31, 2019
@silvin-lubecki
Copy link
Contributor Author

🎉

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.

8 participants