make runAttach public and allow passing context

[ndeloof]

- What I did
make runAttach a public method to be used by docker compose (enforce alignment and avoid code duplication)
introduce runXXWithContext to pass current context

- How I did it

- How to verify it

- Description for the changelog

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

[codecov-commenter]

Codecov Report

Merging #4637 (a2ec50a) into master (9cb175f) will not change coverage.
Report is 13 commits behind head on master.
The diff coverage is 64.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4637   +/-   ##
=======================================
  Coverage   59.74%   59.74%           
=======================================
  Files         288      288           
  Lines       24849    24849           
=======================================
  Hits        14846    14846           
  Misses       9117     9117           
  Partials      886      886           

[thaJeztah]

Perhaps we only need one; we can move the context.Background() to NewAttachCommand, and make the (now exported) RunAttach accept a context.

[thaJeztah]

I'm also looking if we can change this function to accept just the apiClient, but looks like that may need more work.

[thaJeztah]

Oh! Actually, I don't think just exporting this will help you, because attachOptions is not exported, so we may need a function that takes container.AttachOptions instead (but that means we need some way to set all the required options in it, which looks to be somewhat involved still.

[ndeloof]

I tried to limit changes to the strict minimum :)
I wonder why we use a context.Background() here and not the cobra cmd.Context()

[ndeloof]

change this function to accept just the apiClient

this indeed would have much more impacts, as dockerCli is passed to a bunch of public functions, whenever only APIClient or Streams is actually required. This would require a more significant deprecation/newFunc dance :P

[ndeloof]

because attachOptions is not exported

I've been to fast here indeed :)
also exported AttachOptions (same way I did for ExecOptions on 2d26839)

[thaJeztah]

I wonder why we use a context.Background() here and not the cobra cmd.Context()

My best guess; because it originally wasn't there when this code was written (it was added in 2020, and likely code wasn't updated at the time)

[thaJeztah]

Given that runAttach was un-exported before this, I think it's fine to only have one function (RunAttach) and have that accept a context; we can probably create and pass the context in NewAttachCommand / RunE

[ndeloof]

right, just wanted to be homogeneous with RunExecWithContext in method naming

[thaJeztah]

Looking at RunExec, I see it was exported for compose as well;

• publish RunExec for use by docker/compose #3436

These functions were never intended to be exported "as-is", and I'd be ok with changing the signature of RunExec to accept a context (we want to add contexts to these as well for OTEL integration; see #4556

[thaJeztah]

For future reference; we need to have a look at these. The current situation is far from ideal; there's too much logic in the CLI that is not always trivial to replicate. BUT those implementations are written very specifically for the docker CLI itself, and in many cases tightly coupled with the UX, or specific to how the CLI implemented things. Basically, these functions were not exported for that reason.

While exporting relieves some of the pain in compose to allow re-using the logic, it also means that some of these now become a public API, which can complicate maintenance, as we now have to take potential external consumers into account. That's "mostly" fine if "external" means "compose" (we can co-ordinate if we make breaking changes), but may catch "other" consumers off-guard. Longer term, we SHOULD have more of these functions exported (and move them to the client side, not in docker/cli), trying to keep CLI-specifics out where possible.

Perhaps we should add a warning to these function's GoDoc to outline that while they're exported, they are not considered a stable API (as in: signatures may change); “exported for internal use, don’t depend on this”.

I’m also looking a bit if we can reduce the blast-area on these changes. Most notably the AttachOptions (similar with ExecOptions). as those options were created for the flags/options on these commands, and basically directly coupled with the docker CLI UX. I'm looking if we can get those structs out of the exported functions, and perhaps instead make it accept container.AttachOptions, which is exported, and more designed as a public API.

Looking at that, there’s two options missing;

• opts.container: we can pass that as argument to RunAttach (RunAttach(context, containerID, opts))
• opts.noStdin: need to look at that one closer how exactly it’s used; perhaps we can fix that as well through one of the other options?

I'm "ok" with these changes, but we should look at some of those follow-ups (at least the "document as unstable") before we've dug ourselves in a hole that we can't get out of.

[thaJeztah]

I was exploring some options in #4643, and all those are probably out of scope for this PR, but perhaps if would make sense to include one part of that PR, and to pass containerID as separate argument, instead of stuffing it into AttachOptions. Thinking here that containerID (/name) is a required argument, whereas (most? all?) options should ideally be optional.

[thaJeztah]

@ndeloof looks like a test needs to be updated because the signature changed; otherwise I think we should be good to go (baring future work / follow-ups to possibly improve some of this)

[thaJeztah]

LGTM

thanks! I hate it ❤️