Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cp command: copy to all containers of a service as default behaviour #9437

Merged
merged 4 commits into from
May 10, 2022
Merged

cp command: copy to all containers of a service as default behaviour #9437

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
601dae1
cp command: copy to all containers of a service as default behaviour
glours May 3, 2022
0647243
align cp command index management with exec command
glours May 4, 2022
8b2eca4
update usage of the index flag of the cp command
glours May 4, 2022
046d6ca
cp command from service to host: use the first container found to cop…
glours May 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions cmd/compose/cp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func copyCommand(p *projectOptions, backend api.Service) *cobra.Command {
}
return nil
}),
RunE: Adapt(func(ctx context.Context, args []string) error {
RunE: AdaptCmd(func(ctx context.Context, cmd *cobra.Command, args []string) error {
opts.source = args[0]
opts.destination = args[1]
return runCopy(ctx, backend, opts)
Expand All @@ -64,8 +64,10 @@ func copyCommand(p *projectOptions, backend api.Service) *cobra.Command {
}

flags := copyCmd.Flags()
flags.IntVar(&opts.index, "index", 1, "Index of the container if there are multiple instances of a service [default: 1].")
flags.IntVar(&opts.index, "index", 0, "Index of the container if there are multiple instances of a service [default: 1 when copying from a container].")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a huge fan of specifying a default value that we change depending of the context of the cp command. if you have better idea to do it, let me know

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps remove the [default: 1 when copying from a container]. altogether? (note that Cobra already prints defaults, so in most cases, there's no need to manually put it in the description)

Suggested change
flags.IntVar(&opts.index, "index", 0, "Index of the container if there are multiple instances of a service [default: 1 when copying from a container].")
flags.IntVar(&opts.index, "index", 0, "Index of the container if there are multiple instances of a service .")

As to the flag itself; are indexes guaranteed to start with 1? (i.e., could a container be removed, and it starts with 2 ?)

The "when copying from a container" is a bit confusing, as compose works with "services", correct? (in other words; I don't think there's an option to specify a container - for that you'd use docker cp or docker container cp.

Overall I'm wondering a bit if we should have this option; my train of thought here is that;

  • by default, we should assume all instances of a service to be identical
  • compose to interact with a compose "service" (so consider all to be identical)
  • it there IS a need to target a specific instance (which should be an exception to the rule); perhaps users should use docker cp / docker container cp for the "I know what I'm doing" situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

we need to double check index is aligned wit compose exec --index, and that we didn't duplicated the logic to select a target container here so that we enforce an homogeneous behavior

when copying from a container indeed only make sense with a single container as target. Either we should REQUIRE index to be set when multiple container match, or set "first indexed in the available ones" as default (which might not be 1) - but, again, should behave the same as compose exec

it there IS a need to target a specific instance (which should be an exception to the rule); perhaps users should use docker cp / docker container cp for the "I know what I'm doing" situation.

While it obvious they can just run docker exec <container> as name is predicable with compose, for some reason (?) docker-compose exec was introduced and adopted by many, probably because users prefer to rely on the same tool for everything (when you get a hammer...). So, even with containers as target, a compose command makes sense to them. Or maybe this helps writing scripts that do not depend on te project name (included in container name) set by project dir?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, so it was mostly around "when it's needed to target a specific instance"; I can see a good use for docker compose exec (where it automatically picks the (first) instance of the service). Mostly for --index, because in that case, I would still need to do a docker compose ps to learn about what indexes I can pick.

Taking something like;

services:
  web:
    image: nginx:alpine
docker compose up -d --scale web=3
[+] Running 4/4
 ⠿ Network composescale_default  Created                                    0.1s
 ⠿ Container composescale-web-3  Started                                    0.7s
 ⠿ Container composescale-web-1  Started                                    0.6s
 ⠿ Container composescale-web-2  Started                                    0.7s

"just works":

docker compose exec web echo hello
hello

Exec'ing into a specific instance, my natural approach would be to;

Get a list of what's running;

docker compose ps
NAME                 COMMAND                  SERVICE             STATUS              PORTS
composescale-web-1   "/docker-entrypoint.…"   web                 running             80/tcp
composescale-web-2   "/docker-entrypoint.…"   web                 running             80/tcp
composescale-web-3   "/docker-entrypoint.…"   web                 running             80/tcp

Then, docker compose exec into the one I want to access;

docker compose exec composescale-web-2 sh -c 'echo hello world; echo do more things; echo do other things;'
service "composescale-web-2" is not running container #1

But that doesn't work currently (so then I need to use docker exec), as it will treat it as service-name (also somewhat expected)

So to do the same, I need to look at the output, pick the "index" / instance number, edit my command to pass it as --index;

docker compose exec --index=2 web sh -c 'echo hello world; echo do more things; echo do other things;'
hello world
do more things
do other things

(wondering if we should make the docker compose exec composescale-web-2 work 🤔)

Copy link
Contributor Author

@glours glours May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

why not, @ndeloof @ulyssessouza WDYT? I can do that in a follow up PR if we all agree

Copy link
Member

Choose a reason for hiding this comment

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

(let me "unresolve" so that it's not hidden)

Copy link
Contributor

Choose a reason for hiding this comment

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

docker compose exec <containername> doesn't make sense to me, the whole verbage used by compose is service-centric. Use of --index is unpleasant but I can't imagine a workaround. I'd just expect it's not required for service running 1 replica and user don't have to bother with this BUT when they try to diagnose a distributed-system issue with multiple replicas. But then, obviously, using plain container name with docker exec would be a natural option, and maybe we should deprecate --index support and suggest user to better use docker exec for this purpose. Or maybe this is fine we keep this as legacy/usability, as we - at least - share runExec implementation code!

flags.BoolVar(&opts.all, "all", false, "Copy to all the containers of the service.")
flags.MarkHidden("all") //nolint:errcheck
flags.MarkDeprecated("all", "By default all the containers of the service will get the source file/directory to be copied.") //nolint:errcheck
Comment on lines +69 to +70
Copy link
Member

Choose a reason for hiding this comment

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

Instead of the //nolint, perhaps just "fix" it in code (I know we disabled the errcheck linter in various repositories, because it's often a bit "too eager", and having the //nolint for all of them can be noisy; in this case _ = might be "less noisy", and makes it explicit we want to ignore the error);

Suggested change
flags.MarkHidden("all") //nolint:errcheck
flags.MarkDeprecated("all", "By default all the containers of the service will get the source file/directory to be copied.") //nolint:errcheck
_ = flags.MarkHidden("all")
_ = flags.MarkDeprecated("all", "By default all the containers of the service will get the source file/directory to be copied.")

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer we have noose by end of line with a nolint comment vs start of line, where my mind will focus reading, for useless assignment

flags.BoolVarP(&opts.followLink, "follow-link", "L", false, "Always follow symbol link in SRC_PATH")
flags.BoolVarP(&opts.copyUIDGID, "archive", "a", false, "Archive mode (copy all uid/gid information)")

Expand Down
3 changes: 1 addition & 2 deletions docs/reference/compose_cp.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,9 @@ Copy files/folders between a service container and the local filesystem

| Name | Type | Default | Description |
| --- | --- | --- | --- |
| `--all` | | | Copy to all the containers of the service. |
| `-a`, `--archive` | | | Archive mode (copy all uid/gid information) |
| `-L`, `--follow-link` | | | Always follow symbol link in SRC_PATH |
| `--index` | `int` | `1` | Index of the container if there are multiple instances of a service [default: 1]. |
| `--index` | `int` | `0` | Index of the container if there are multiple instances of a service [default: 1 when copying from a container]. |


<!---MARKER_GEN_END-->
Expand Down
8 changes: 4 additions & 4 deletions docs/reference/docker_compose_cp.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ options:
value_type: bool
default_value: "false"
description: Copy to all the containers of the service.
deprecated: false
hidden: false
deprecated: true
hidden: true
experimental: false
experimentalcli: false
kubernetes: false
Expand Down Expand Up @@ -40,9 +40,9 @@ options:
swarm: false
- option: index
value_type: int
default_value: "1"
default_value: "0"
description: |
Index of the container if there are multiple instances of a service [default: 1].
Index of the container if there are multiple instances of a service [default: 1 when copying from a container].
deprecated: false
hidden: false
experimental: false
Expand Down
6 changes: 5 additions & 1 deletion pkg/compose/cp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ func (s *composeService) Copy(ctx context.Context, projectName string, options a
if options.All {
return errors.New("cannot use the --all flag when copying from a service")
}
// due to remove of the --all flag, restore the default value to 1 when copying from a service container to the host.
if options.Index == 0 {
options.Index = 1
glours marked this conversation as resolved.
Show resolved Hide resolved
}
}
if destService != "" {
direction |= toService
Expand All @@ -72,7 +76,7 @@ func (s *composeService) Copy(ctx context.Context, projectName string, options a
return fmt.Errorf("no container found for service %q", serviceName)
}

if !options.All {
if direction == fromService || (direction == toService && options.Index > 0) {
containers = containers.filter(indexed(options.Index))
}

Expand Down
23 changes: 6 additions & 17 deletions pkg/e2e/cp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,18 @@ func TestCopy(t *testing.T) {
res.Assert(t, icmd.Expected{Out: `nginx running`})
})

t.Run("copy to container copies the file to the first container by default", func(t *testing.T) {
t.Run("copy to container copies the file to the all containers by default", func(t *testing.T) {
res := c.RunDockerComposeCmd("-f", "./fixtures/cp-test/compose.yaml", "-p", projectName, "cp", "./fixtures/cp-test/cp-me.txt", "nginx:/tmp/default.txt")
res.Assert(t, icmd.Expected{ExitCode: 0})

output := c.RunDockerCmd("exec", projectName+"-nginx-1", "cat", "/tmp/default.txt").Stdout()
assert.Assert(t, strings.Contains(output, `hello world`), output)

res = c.RunDockerOrExitError("exec", projectName+"_nginx_2", "cat", "/tmp/default.txt")
res.Assert(t, icmd.Expected{ExitCode: 1})
output = c.RunDockerCmd("exec", projectName+"-nginx-2", "cat", "/tmp/default.txt").Stdout()
assert.Assert(t, strings.Contains(output, `hello world`), output)

output = c.RunDockerCmd("exec", projectName+"-nginx-3", "cat", "/tmp/default.txt").Stdout()
assert.Assert(t, strings.Contains(output, `hello world`), output)
})

t.Run("copy to container with a given index copies the file to the given container", func(t *testing.T) {
Expand All @@ -69,20 +72,6 @@ func TestCopy(t *testing.T) {
res.Assert(t, icmd.Expected{ExitCode: 1})
})

t.Run("copy to container with the all flag copies the file to all containers", func(t *testing.T) {
res := c.RunDockerComposeCmd("-f", "./fixtures/cp-test/compose.yaml", "-p", projectName, "cp", "--all", "./fixtures/cp-test/cp-me.txt", "nginx:/tmp/all.txt")
res.Assert(t, icmd.Expected{ExitCode: 0})

output := c.RunDockerCmd("exec", projectName+"-nginx-1", "cat", "/tmp/all.txt").Stdout()
assert.Assert(t, strings.Contains(output, `hello world`), output)

output = c.RunDockerCmd("exec", projectName+"-nginx-2", "cat", "/tmp/all.txt").Stdout()
assert.Assert(t, strings.Contains(output, `hello world`), output)

output = c.RunDockerCmd("exec", projectName+"-nginx-3", "cat", "/tmp/all.txt").Stdout()
assert.Assert(t, strings.Contains(output, `hello world`), output)
})

t.Run("copy from a container copies the file to the host from the first container by default", func(t *testing.T) {
res := c.RunDockerComposeCmd("-f", "./fixtures/cp-test/compose.yaml", "-p", projectName, "cp", "nginx:/tmp/default.txt", "./fixtures/cp-test/from-default.txt")
res.Assert(t, icmd.Expected{ExitCode: 0})
Expand Down