feat(desktop): add Docker Desktop detection and client skeleton #11593

milas · 2024-03-08T20:17:08Z

What I did
Use the system info Engine API to auto-discover the Desktop integration API endpoint (a local AF_UNIX socket or named pipe).

If present and responding to a /ping, the client is populated on the Compose service for use. Otherwise, it's left nil, and Compose will not try to use any functionality that relies on Docker Desktop (e.g. opening docker-desktop:// deep links to the GUI).

This refactors some of the in-memory socket dialer code, so we can share it easier (time to bring back docker/go-connections? 🤔) and make it slightly more robust.

Related issue
JIRA: COMP-457

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

codecov · 2024-03-08T21:09:14Z

Codecov Report

Attention: Patch coverage is 15.62500% with 108 lines in your changes are missing coverage. Please review.

Project coverage is 57.75%. Comparing base (4efb897) to head (7452a14).

Files	Patch %	Lines
internal/desktop/client.go	0.00%	37 Missing ⚠️
pkg/compose/desktop.go	14.28%	24 Missing ⚠️
internal/memnet/conn.go	0.00%	21 Missing ⚠️
pkg/compose/compose.go	0.00%	9 Missing ⚠️
cmd/compose/compose.go	68.42%	5 Missing and 1 partial ⚠️
internal/memnet/conn_unix.go	0.00%	5 Missing ⚠️
cmd/main.go	50.00%	2 Missing and 1 partial ⚠️
internal/tracing/docker_context.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11593      +/-   ##
==========================================
- Coverage   58.20%   57.75%   -0.45%     
==========================================
  Files         135      138       +3     
  Lines       11611    11717     +106     
==========================================
+ Hits         6758     6767       +9     
- Misses       4182     4277      +95     
- Partials      671      673       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jhrotko · 2024-03-11T12:30:57Z

pkg/compose/compose.go

+	if s.dockerCli != nil {
+		errs = append(errs, s.dockerCli.Client().Close())
+	}
+	if s.desktopCli != nil {
+		errs = append(errs, s.desktopCli.Close())
+	}


merge if statements?

not the same CLI here

I dont know how to read :)

jhrotko · 2024-03-11T12:56:49Z

Could we also add an e2e test for this cases? unix, windows and darwin?

ndeloof · 2024-03-11T13:32:38Z

cmd/compose/compose.go

 			if err != nil {
 				return err
 			}
 			cmd.SetContext(ctx)
+
+			// (6) Desktop integration
+			if db, ok := backend.(desktop.IntegrationService); ok {


I wish we refactor this init sequence so we don't mutate backend but use setup builder according to options:

backend := compose.NewComposeService(dockerCli, WithDryRun(xx), WithDesktopIntegration(xx))

Agreed 😞 the init is overly complicated, especially because of the split between Docker/CLI/plugins, Compose itself, and Cobra 🐍

This was already kind of hacky - I didn't want to add this to api.Service but the concrete composeService implementation is unexported

I'll see if I can refactor this in another PR to swap to options an consolidate some things, e.g. we have several lazy/memoized components but they all happen in different ways currently

I didn't want to add this to api.Service

why not ?

milas · 2024-03-11T19:37:35Z

Could we also add an e2e test for this cases? unix, windows and darwin?

Yes, this is a great call out! This will require me to sort out a couple things first - need to get updated Docker Desktop builds on our CI runners & service accounts on Hub for them to log in as, so I made a ticket in JIRA.

I'll go ahead and get started on the test and t.Skip() it for the moment at least

Use the system info Engine API to auto-discover the Desktop integration API endpoint (a local `AF_UNIX` socket or named pipe). If present and responding to a `/ping`, the client is populated on the Compose service for use. Otherwise, it's left `nil`, and Compose will not try to use any functionality that relies on Docker Desktop (e.g. opening `docker-desktop://` deep links to the GUI). Signed-off-by: Milas Bowman <milas.bowman@docker.com>

jhrotko · 2024-03-11T12:54:16Z

internal/memnet/conn.go

+	var d net.Dialer
+	switch network {
+	case "unix":
+		if err := validateSocketPath(addr); err != nil {


build is complaining this is undefined for windows arch

jhrotko · 2024-03-11T12:55:58Z

internal/memnet/conn_windows.go

 	return winio.DialPipeContext(ctx, addr)
 }
+
+func validateUnixSocketPath(addr string) error {


rename to validateSocketPath to avoid build errors

jhrotko · 2024-03-11T15:33:13Z

pkg/compose/compose.go

+	if s.dockerCli != nil {
+		errs = append(errs, s.dockerCli.Client().Close())
+	}
+	if s.desktopCli != nil {
+		errs = append(errs, s.desktopCli.Close())
+	}


I dont know how to read :)

…er#11593)

milas added the area/desktop Features that are integrated with Docker Desktop label Mar 8, 2024

milas requested a review from a team March 8, 2024 20:17

milas self-assigned this Mar 8, 2024

milas requested review from ndeloof, glours and jhrotko and removed request for a team March 8, 2024 20:17

milas force-pushed the desktop-detection branch from 85f1050 to cab1ac5 Compare March 8, 2024 20:57

jhrotko reviewed Mar 11, 2024

View reviewed changes

ndeloof reviewed Mar 11, 2024

View reviewed changes

ndeloof approved these changes Mar 11, 2024

View reviewed changes

glours approved these changes Mar 11, 2024

View reviewed changes

milas force-pushed the desktop-detection branch from cab1ac5 to 7452a14 Compare March 11, 2024 19:51

jhrotko approved these changes Mar 12, 2024

View reviewed changes

milas merged commit 17d4229 into docker:main Mar 12, 2024
28 checks passed

milas deleted the desktop-detection branch March 12, 2024 13:47

laurazard mentioned this pull request Mar 12, 2024

Add navigation menu with in compose up (attached) #11605

Merged

rarguelloF pushed a commit to rarguelloF/compose that referenced this pull request Mar 15, 2024

feat(desktop): add Docker Desktop detection and client skeleton (dock…

7b7f6a0

…er#11593)

BrewTestBot mentioned this pull request Mar 15, 2024

docker-compose 2.25.0 Homebrew/homebrew-core#166240

Merged

temenuzhka-thede pushed a commit to temenuzhka-thede/compose that referenced this pull request Sep 17, 2024

feat(desktop): add Docker Desktop detection and client skeleton (dock…

cdbdf2f

…er#11593)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(desktop): add Docker Desktop detection and client skeleton #11593

feat(desktop): add Docker Desktop detection and client skeleton #11593

milas commented Mar 8, 2024 •

edited by jira bot

Loading

codecov bot commented Mar 8, 2024 •

edited

Loading

jhrotko Mar 11, 2024 •

edited

Loading

ndeloof Mar 11, 2024

jhrotko Mar 11, 2024

jhrotko commented Mar 11, 2024

ndeloof Mar 11, 2024

milas Mar 11, 2024

ndeloof Mar 11, 2024

milas commented Mar 11, 2024

jhrotko Mar 11, 2024

jhrotko Mar 11, 2024

jhrotko Mar 11, 2024

feat(desktop): add Docker Desktop detection and client skeleton #11593

feat(desktop): add Docker Desktop detection and client skeleton #11593

Conversation

milas commented Mar 8, 2024 • edited by jira bot Loading

codecov bot commented Mar 8, 2024 • edited Loading

Codecov Report

jhrotko Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhrotko commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milas commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milas commented Mar 8, 2024 •

edited by jira bot

Loading

codecov bot commented Mar 8, 2024 •

edited

Loading

jhrotko Mar 11, 2024 •

edited

Loading