docker / app

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

Remove target-context flags #617

Closed ulyssessouza closed 4 years ago

ulyssessouza commented 4 years ago

As a user of the Docker App CLI tool So that I have a consistent user experience with the Docker CLI I want the target context flag removed

- What I did Remove all the --target-context and references to it in the README.md

- How I did it

- How to verify it Check the commands to see that the flags are not there anymore

- Description for the changelog Remove target-context flags

- A picture of a cute animal (not mandatory but encouraged) 2baby-goats

codecov[bot] commented 4 years ago

Codecov Report

Merging #617 into master will increase coverage by 1.13%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #617      +/-   ##
==========================================
+ Coverage   70.94%   72.08%   +1.13%     
==========================================
  Files          61       61              
  Lines        3493     3374     -119     
==========================================
- Hits         2478     2432      -46     
+ Misses        695      623      -72     
+ Partials      320      319       -1
Impacted Files Coverage Δ
internal/commands/image/list.go 83.67% <100%> (ø) :arrow_up:
internal/packager/init.go 61.68% <0%> (-9.13%) :arrow_down:
internal/commands/root.go 76.59% <0%> (-1.91%) :arrow_down:
internal/packager/cnab.go 97.91% <0%> (-0.09%) :arrow_down:
internal/names.go 100% <0%> (ø) :arrow_up:
internal/commands/inspect.go
internal/cliopts/installerContext.go 25% <0%> (ø)
internal/cnab/cnab.go 64.38% <0%> (+0.09%) :arrow_up:
internal/commands/credentials.go 70% <0%> (+0.37%) :arrow_up:
internal/cnab/driver.go 86.36% <0%> (+1.45%) :arrow_up:
... and 7 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 8acd0a8...cadc25c. Read the comment docs.

kinghuang commented 4 years ago

I strongly object to this change, assuming docker-app continues to spin up a container with the invocation image in order to do installs/uninstalls.

In my environment, it is not possible to run arbitrary containers on the swarm manager nodes. With docker-app 0.8.0, I've had to make sure that my Docker context is set to my local Docker engine (e.g., DOCKER_CONTEXT=default), and run docker-app commands with --target-context=the-remote-cluster. Otherwise, it attempts to run the invocation container on the manager node and fails.

Removing --target-context will completely break my ability to deploy apps, if an invocation container is needed. I need a way to tell docker-app to run its invocation containers in one context, and deploy to a different context.

ulyssessouza commented 4 years ago

@kinghuang Thank you for you feedback! What do you think about instead of this, we use --context as the target and introducing a new experimental flag called --invocation-context?

So in your example, you could just use something like:

$ docker app install --context the-remote-cluster --invocation-context default myapp
kinghuang commented 4 years ago

Yes, I believe an --invocation-context option will work, and achieves the goal of consistency with the rest of the Docker CLI. An associated environment variable for the option would be greatly appreciated, too!