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

[glours]

What I did
Change the default behaviour of the cp command when copying from host to a service. Now it will copy the source to all the containers of a service instead of the first one previously

Deprecation of the --all flag

(not mandatory) A picture of a cute animal, if possible in relation with what you did

[glours]

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

[thaJeztah]

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.

[ndeloof]

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?

[thaJeztah]

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 🤔)

[glours]

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

[thaJeztah]

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

[ndeloof]

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!

[thaJeztah]

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.")

[ndeloof]

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

[thaJeztah]

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.

[thaJeztah]

Perhaps s.getContainers() should accept an index;

• if index != 0; return the specified container (slice with one container)
• otherwise return all containers

That way the code could be simplified / share more;

• for copyFrom, it always picks the first from that list
• for copyTo, it always iterates over the list of containers

[thaJeztah]

Looks like the first and last branch are basically "the same", so this could be;

func (s *composeService) listContainersTargetedForCopy(ctx context.Context, projectName string, index int, direction copyDirection, serviceName string) (Containers, error) {
	switch {
	case index > 0:
		container, err := s.getSpecifiedContainer(ctx, projectName, oneOffExclude, true, serviceName, index)
		if err != nil {
			return nil, err
		}
		return Containers{container}, nil
	default:
		containers, err := s.getContainers(ctx, projectName, oneOffExclude, true, serviceName)
		if err != nil {
			return nil, err
		}
		if len(containers) < 1 {
			return nil, fmt.Errorf("no container found for service %q", serviceName)
		}
		if direction == fromService {
			// copy from container defaults to the first container
			return containers[:1], nil
		}
		return containers, nil
	}
}

[glours]

I prefer to keep the s.getContainers() as it because it's wildly used in the rest of the code. Having a bit more complexity in the copyFrom and copyTo seems to be a good tradeoff IHMO.

I updated the switch as you proposed 👍

[thaJeztah]

Can't comment on the line below (and unrelated to this PR), but wondering why we do the validation of direction in the loop? The direction is defined above, and will always be the same for all containers, so I think we can return early (with an error, if needed) at the start of the function, so that we don't have to first collect the containers, and do the same check for each one of them.

[glours]

I addressed it 😉