make runAttach public and allow passing context #4637

ndeloof · 2023-11-06T09:03:08Z

- 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 · 2023-11-06T09:06:03Z

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 · 2023-11-06T09:17:43Z

cli/command/container/attach.go

+func RunAttach(dockerCli command.Cli, opts *attachOptions) error {
+	return RunAttachWithContext(context.Background(), dockerCli, opts)
+}
+
+func RunAttachWithContext(ctx context.Context, dockerCli command.Cli, opts *attachOptions) error {


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

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

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.

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

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

because attachOptions is not exported

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

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 · 2023-11-06T10:04:46Z

cli/command/container/attach.go

+// RunAttach executes an `attach` command
+func RunAttach(dockerCli command.Cli, opts *AttachOptions) error {
+	return RunAttachWithContext(context.Background(), dockerCli, opts)
+}
+
+// RunAttachWithContext executes an `attach` command
+func RunAttachWithContext(ctx context.Context, dockerCli command.Cli, opts *AttachOptions) error {


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

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

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 · 2023-11-07T11:33:08Z

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.

Trying to make "runAttach" not depend on attachOptions, and to see if we can make it accept container.AttachOptions instead relates to docker#4637 Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2023-11-07T12:12:39Z

cli/command/container/attach.go

-func runAttach(dockerCli command.Cli, opts *attachOptions) error {
-	ctx := context.Background()
+// RunAttach executes an `attach` command
+func RunAttach(ctx context.Context, dockerCli command.Cli, opts *AttachOptions) error {


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 · 2023-11-07T16:00:08Z

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

Signed-off-by: Nicolas De Loof <[email protected]>

thaJeztah

LGTM

thanks! I hate it ❤️

Trying to make "runAttach" not depend on attachOptions, and to see if we can make it accept container.AttachOptions instead relates to docker#4637 Signed-off-by: Sebastiaan van Stijn <[email protected]>

ndeloof mentioned this pull request Nov 6, 2023

Add docker compose attach? (to support STDIN) docker/compose#11153

Closed

thaJeztah reviewed Nov 6, 2023

View reviewed changes

ndeloof force-pushed the RunExecWithContextb branch 3 times, most recently from 4f85512 to 667665c Compare November 6, 2023 09:58

thaJeztah reviewed Nov 6, 2023

View reviewed changes

ndeloof force-pushed the RunExecWithContextb branch from 667665c to 71b3c52 Compare November 6, 2023 10:13

thaJeztah mentioned this pull request Nov 7, 2023

WIP: rewrite runAttach to be exported #4643

Draft

thaJeztah reviewed Nov 7, 2023

View reviewed changes

ndeloof force-pushed the RunExecWithContextb branch from 71b3c52 to 6f55d47 Compare November 7, 2023 12:58

thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Nov 7, 2023

thaJeztah added this to the 25.0.0 milestone Nov 7, 2023

make runAttach public and allow passing context

a4abe42

Signed-off-by: Nicolas De Loof <[email protected]>

ndeloof force-pushed the RunExecWithContextb branch from 6f55d47 to a4abe42 Compare November 8, 2023 13:25

make container an explicit, required parameter

a2ec50a

Signed-off-by: Nicolas De Loof <[email protected]>

thaJeztah approved these changes Nov 8, 2023

View reviewed changes

thaJeztah merged commit ad861cd into docker:master Nov 8, 2023
76 checks passed

thaJeztah added area/metrics area/metrics/otel labels Mar 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make runAttach public and allow passing context #4637

make runAttach public and allow passing context #4637

ndeloof commented Nov 6, 2023

codecov-commenter commented Nov 6, 2023 •

edited

Loading

thaJeztah Nov 6, 2023

thaJeztah Nov 6, 2023

thaJeztah Nov 6, 2023

ndeloof Nov 6, 2023

ndeloof Nov 6, 2023

ndeloof Nov 6, 2023

thaJeztah Nov 6, 2023

thaJeztah Nov 6, 2023

ndeloof Nov 6, 2023

thaJeztah Nov 7, 2023

thaJeztah commented Nov 7, 2023

thaJeztah Nov 7, 2023

thaJeztah commented Nov 7, 2023

thaJeztah left a comment

make runAttach public and allow passing context #4637

make runAttach public and allow passing context #4637

Conversation

ndeloof commented Nov 6, 2023

codecov-commenter commented Nov 6, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Nov 7, 2023

Choose a reason for hiding this comment

thaJeztah commented Nov 7, 2023

thaJeztah left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 6, 2023 •

edited

Loading