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

Remove target-context flags #617

Merged
merged 8 commits into from
Nov 7, 2019

Conversation

ulyssessouza
Copy link
Contributor

@ulyssessouza ulyssessouza commented Sep 16, 2019

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

  • removed all the --target-context and references to it in the README.md
  • app is installed in context pointed by docker CLI
  • installer image runs in default context by defaut until overriden by --installer-context

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

codecov bot commented Sep 16, 2019

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%> (ø) ⬆️
internal/packager/init.go 61.68% <0%> (-9.13%) ⬇️
internal/commands/root.go 76.59% <0%> (-1.91%) ⬇️
internal/packager/cnab.go 97.91% <0%> (-0.09%) ⬇️
internal/names.go 100% <0%> (ø) ⬆️
internal/commands/inspect.go
internal/cliopts/installerContext.go 25% <0%> (ø)
internal/cnab/cnab.go 64.38% <0%> (+0.09%) ⬆️
internal/commands/credentials.go 70% <0%> (+0.37%) ⬆️
internal/cnab/driver.go 86.36% <0%> (+1.45%) ⬆️
... 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.

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.

What about e2e tests https://github.com/docker/app/blob/master/e2e/commands_test.go#L480
Should we simplify them too so they stick to the user's main flow?

@kinghuang
Copy link

kinghuang commented Sep 19, 2019

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

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

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!

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.

Code looks good, but I wonder if the rm action should also have this installer context flag 🤔

@ndeloof ndeloof force-pushed the remove-target-context-flag branch 7 times, most recently from 242480c to f4f93b5 Compare November 5, 2019 10:22
@ndeloof ndeloof force-pushed the remove-target-context-flag branch 8 times, most recently from 25cfc15 to 1c648fd Compare November 6, 2019 13:05
@silvin-lubecki silvin-lubecki changed the title [WiP] Remove target-context flags Remove target-context flags Nov 6, 2019
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

ulyssessouza and others added 5 commits November 6, 2019 15:30
Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Create a new DockerCli used to run installer image within requested context
cli.Cli (not DockerCli implementation) doesn't offer any way to switch
contexts once intialized.

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
from within the installer image, this would only resolve to container's loopback
so we use computer external IP

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
as tabwritter will change formatting depending on number of digits in IP
address

10.100.97.67:32795/c-myapp:latest
10.100.102.144:32795/c-myapp:latest

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
We need to select interface used to access default gateway, as docker0
bridge or Windows DockerNAT won't route traffic to containers by exposed
port

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof ndeloof force-pushed the remove-target-context-flag branch 2 times, most recently from cdeb901 to 2d5374c Compare November 6, 2019 14:44
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof ndeloof force-pushed the remove-target-context-flag branch 2 times, most recently from d241fda to 216f639 Compare November 7, 2019 11:08
Without this check, Linux bridge docker0 or Winwdows DockerNAT could be
selected as "host IP" but won't allow to route to container
Same issue probalby exists on Linux with docker0 bridge
Here we discover the default gateway, and search for the interface
configured on the same network.

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
README.md Show resolved Hide resolved
internal/commands/list.go Outdated Show resolved Hide resolved
Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

Couple of nits but looks good

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof ndeloof merged commit a850314 into docker:master Nov 7, 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