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

jhrotko · 2024-03-12T13:01:15Z

What I did
We want to open Docker Desktop through a shortcut in docker compose up.

To use this feature run

docker compose up

... yes it comes as default for attached mode :). it will render the navigation menu at the bottom.
You can disable this feature while using up run:

docker compose up --menu=[false|true]

OR set environment var COMPOSE_MENU=[false|true]
OR use Docker Desktop experimental features and enable/disable - Compose: TUI nav bar

Screen.Recording.2024-03-16.at.17.01.26.mov

When an error occurs, it will be displayed at the top of the navigation menu for 10s.
We are also sending telemetry to understand if users' usage of this new feature. PR merged in pinata: https://github.com/docker/pinata/pull/27207

This PR introduces a keyboard listener that is currently listening to the following events:

CTRL+C
ENTER
w -> Start watch mode (alternatively you can also start watch mode with docker compose up --watch)
v -> Open compose project in Docker Desktop through a deeplink docker-desktop://dashboard/apps/NAME

Related issue

https://docker.atlassian.net/jira/software/c/projects/COMP/boards/576?selectedIssue=COMP-417

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

ndeloof · 2024-03-12T13:33:40Z

pkg/compose/up.go

+				case keyboard.KeyCtrlC:
+					keyboard.Close()
+					formatter.KeyboardInfo.ClearInfo()
+					gracefulTeardown()


I wonder about the impact doing so as context is not cancelled.

How can I test this scenario? What is expected if the context is not cancelled?

I'm not sure. We rely on <-ctx.Done() in a few place to stop goroutines after Ctrl+C, typically to stop collecting logs, watch container events, etc. I assume after the application stopped those will be released anyway, but this might bring some new race conditions.

I think it should be okay here (although it makes me think the graceful termination code/doneCh/ctx.Done() handling could use a refactor).

I wonder if cancelling the context here would be better.

Also I wonder, since we're already capturing SIGINT above (when we do signal.Notify(signalChan, syscall.SIGINT, syscall.SIGTERM), if the new keyboard stuff is going to interfere with that.

Changed the way we handle the tear down. I am sending a SIGTERM signal to signalChan https://github.com/docker/compose/pull/11605/files#diff-f5beca66c4f279fbf26bb1aad4367b541d1b0096b5e68d03515fb922201e7aadR290

ndeloof · 2024-03-12T13:35:31Z

go.mod

@@ -81,6 +83,7 @@ require (
 	github.com/docker/docker-credential-helpers v0.8.0 // indirect
 	github.com/docker/go v1.5.1-1.0.20160303222718-d30aec9fd63c // indirect
 	github.com/docker/go-metrics v0.0.1 // indirect
+	github.com/eiannone/keyboard v0.0.0-20220611211555-0d226195f203


Probably does what we expect, by I wonder we want to rely on a dependency which hasn't been updated for 2 years

laurazard · 2024-03-12T16:52:11Z

cmd/formatter/shortcut.go

+func (lk *LogKeyboard) createBuffer() {
+	fmt.Print("\012") // new line
+	fmt.Print("\012")
+	fmt.Print("\033[2A") // go back 3 lines
+}
+
+func (lk *LogKeyboard) printInfo() {
+	height := goterm.Height()
+	fmt.Print("\0337") // save cursor position
+	if lk.err != nil {
+		fmt.Printf("\033[%d;0H", height-1) // Move to before last line
+		fmt.Printf("\033[K" + errorColor + "[Error]   " + lk.err.Error())
+	}
+	fmt.Printf("\033[%d;0H", height) // Move to last line
+	// clear line
+	fmt.Print("\033[K" + navColor("  >> [CTRL+G] open project in Docker Desktop [$] get more features"))
+	fmt.Print("\0338") // restore cursor position
+}
+
+func (lk *LogKeyboard) ClearInfo() {
+	height := goterm.Height()
+	fmt.Print("\0337") // save cursor position
+	if lk.err != nil {
+		fmt.Printf("\033[%d;0H", height-1)
+		fmt.Print("\033[2K") // clear line
+	}
+	fmt.Printf("\033[%d;0H", height) // Move to last line
+	fmt.Print("\033[2K")             // clear line
+	fmt.Print("\0338")               // restore cursor position
+}


Just thinking/not a blocker, but I'm starting to wonder if we should think about migrating over to an actual TUI lib as opposed to rolling our own. bubbletea is pretty nice for this kind of stuff.

agreed, we should consider this lib for a further interaction if this feature gets traction

laurazard · 2024-03-12T16:54:34Z

cmd/formatter/shortcut.go

+		fmt.Print("\033[2K") // clear line
+	}
+	fmt.Printf("\033[%d;0H", height) // Move to last line
+	fmt.Print("\033[2K")             // clear line


Might be more readable to put all these hieroglyphs in consts

Did huge refactor on this file. Was still work in progress at the time. It should be more readable now :)

laurazard · 2024-03-12T16:57:43Z

Looks really good overall! Happy with the "TUI" improvements 🥳

I think that's the plan from what @jhrotko has mentioned, but I wanted to confirm that the idea is to use #11593 and only enable this when running this on a Docker Desktop environment (cc @milas), so that people running this headless/on bare Linux w/o DD don't get confused.

ndeloof · 2024-03-13T16:02:37Z

cmd/compose/up.go

@@ -127,6 +128,7 @@ func upCommand(p *ProjectOptions, dockerCli command.Cli, backend api.Service) *c
 	flags.BoolVar(&up.wait, "wait", false, "Wait for services to be running|healthy. Implies detached mode.")
 	flags.IntVar(&up.waitTimeout, "wait-timeout", 0, "Maximum duration to wait for the project to be running|healthy")
 	flags.BoolVarP(&up.watch, "watch", "w", false, "Watch source code and rebuild/refresh containers when files are updated.")
+	flags.BoolVar(&up.navigationBar, "navigation-menu", true, "While running in attach mode, enable shortcuts and shortcuts info bar.")


"shortcuts and shortcuts" ?

Did a fix. Let me know what do you think

codecov · 2024-03-18T10:57:35Z

Codecov Report

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

Project coverage is 54.75%. Comparing base (3950460) to head (831a5af).

Files	Patch %	Lines
cmd/formatter/shortcut.go	0.00%	192 Missing ⚠️
cmd/formatter/ansi.go	16.12%	26 Missing ⚠️
pkg/compose/up.go	33.33%	10 Missing and 2 partials ⚠️
cmd/formatter/colors.go	0.00%	8 Missing ⚠️
cmd/formatter/logs.go	60.00%	2 Missing and 4 partials ⚠️
internal/tracing/keyboard_metrics.go	45.45%	4 Missing and 2 partials ⚠️
pkg/compose/watch.go	73.33%	3 Missing and 1 partial ⚠️
cmd/compose/compose.go	70.00%	1 Missing and 2 partials ⚠️
internal/tracing/wrap.go	75.00%	1 Missing ⚠️
pkg/compose/compose.go	66.66%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11605      +/-   ##
==========================================
- Coverage   55.62%   54.75%   -0.87%     
==========================================
  Files         142      145       +3     
  Lines       12258    12553     +295     
==========================================
+ Hits         6818     6874      +56     
- Misses       4752     4984     +232     
- Partials      688      695       +7

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

docs/reference/docker_compose_up.yaml

laurazard · 2024-03-19T15:58:26Z

Dockerfile

@@ -101,6 +101,7 @@ RUN --mount=type=bind,target=. \
 FROM build-base AS test
 ARG CGO_ENABLED=0
 ARG BUILD_TAGS
+ENV COMPOSE_MENU=FALSE


Is this necessary for building?

yes, because by default it should be true, in order not to break tests this is easier to set the COMPOSE_MENU false in all containers

(as a follow-up) We can have the e2e tests set this when they exec Compose to avoid needing it globally like this

laurazard · 2024-03-19T16:03:00Z

cmd/formatter/colors.go

+	return fmt.Sprintf("%s%s%s", ansiColorCode(code), s, ansiColorCode("0"))
 }

-func ansi(code string) string {
+// Everything about ansiColorCode color https://hyperskill.org/learn/step/18193
+func ansiColorCode(code string) string {
 	return fmt.Sprintf("\033[%sm", code)
 }


Didn't realize we had this – isn't the stuff in aec enough? I think we use it sometimes

compose/pkg/progress/colors.go

Lines 30 to 36 in 9b0e3d5

DoneColor colorFunc = aec.BlueF.Apply

TimerColor colorFunc = aec.BlueF.Apply

CountColor colorFunc = aec.YellowF.Apply

WarningColor colorFunc = aec.YellowF.With(aec.Bold).Apply

SuccessColor colorFunc = aec.GreenF.Apply

ErrorColor colorFunc = aec.RedF.With(aec.Bold).Apply

PrefixColor colorFunc = aec.CyanF.Apply

then it's used like

compose/pkg/progress/tty.go

Line 258 in 9b0e3d5

SuccessColor(strings.Join(completion, "")),

I need to add the bold to the prefix color :( \033[36m -> cyan vs \033[1;36m -> bold + cyan. The prefix color is not abstract enough for me to add the bold format

dock-tor · 2024-03-21T14:41:07Z

cmd/compose/up.go

@@ -181,6 +186,9 @@ func runUp(
 	if err != nil {
 		return err
 	}
+	if !upOptions.navigationMenuChanged {


seems bit complicated to manage value set by env var, is there any drawback doing the same as https://github.com/docker/compose/blob/main/cmd/compose/up.go#L109-L110 ?

(this comment is mine, I was connected with by @docker.com email while commenting :P)

we also want to have into consideration the value of the Docker Desktop experimental feature value

(not for this PR)

We definitely need to figure out a re-usable pattern for these types of use cases to simplify driving experimental settings via Docker Desktop and combining with flags/options in Compose

Our init sequence has gotten pretty complex between Docker CLI plugin framework + Cobra + Compose itself 😓

milas

LGTM - few random comments, biggest question is over that one conditional but otherwise no major concerns from me

milas · 2024-03-21T18:39:49Z

Dockerfile

@@ -101,6 +101,7 @@ RUN --mount=type=bind,target=. \
 FROM build-base AS test
 ARG CGO_ENABLED=0
 ARG BUILD_TAGS
+ENV COMPOSE_MENU=FALSE


(as a follow-up) We can have the e2e tests set this when they exec Compose to avoid needing it globally like this

milas · 2024-03-21T18:43:32Z

cmd/compose/up.go

@@ -181,6 +186,9 @@ func runUp(
 	if err != nil {
 		return err
 	}
+	if !upOptions.navigationMenuChanged {


(not for this PR)

We definitely need to figure out a re-usable pattern for these types of use cases to simplify driving experimental settings via Docker Desktop and combining with flags/options in Compose

Our init sequence has gotten pretty complex between Docker CLI plugin framework + Cobra + Compose itself 😓

milas · 2024-03-22T01:17:37Z

cmd/formatter/shortcut.go

+	timeStart time.Time
+}
+
+func (ke *KeyboardError) shoudlDisplay() bool {


s/shoudlDisplay/shouldDisplay

milas · 2024-03-22T01:22:30Z

cmd/formatter/shortcut.go

+
+		MoveCursor(height-linesOffset(menu), 0)
+		ClearLine()
+		fmt.Print(menu)


(for follow-up)

You can make it easier to test this by storing stdout/io.Writer on the type and writing to that.

you mean, instead of using fmt.Print first pipe all the characters to stdout and then print?

add an io.Writer field in KeyboardError type, and use fmt.Fprint. This will allow unit tests to pass a bytes.Buffer as writer and check output
For actual use, should be initialized to dockercli.Out() to respect docker CLI plugin architecture

milas · 2024-03-22T01:24:38Z

cmd/formatter/shortcut.go

+	}
+}
+
+func (lk *LogKeyboard) navigationMenu() string {


fyi strings.Builder can sometimes be handy for these types of finicky formatting formatting type functions

(this is readable/fine as-is! no need to change, just sharing)

https://pkg.go.dev/strings#Builder

milas · 2024-03-22T01:28:46Z

cmd/formatter/shortcut.go

+	lk.Watch.switchWatching()
+	if !lk.Watch.isWatching() && lk.Watch.Cancel != nil {
+		lk.Watch.Cancel()
+	} else {


should this be

lk.Watch.switchWatching() if !lk.Watch.isWatching() { if lk.Watch.Cancel != nil { lk.Watch.Cancel() } } else {

(that is, even if the cancel function is nil, we only want to start watch if isWatching == true, right?)

yes, I think this was an old bug that was fixed, I think it was because the configuration validation was being done after and not before at some point. Both expressions are equivalent

milas · 2024-03-22T01:30:05Z

pkg/compose/compose.go

@@ -320,3 +320,7 @@ func (s *composeService) RuntimeVersion(ctx context.Context) (string, error) {
 	return runtimeVersion.val, runtimeVersion.err

 }
+
+func (s *composeService) isDesktopIntegrationActive() bool {


I refactored other places in the code that also did this validation

ndeloof · 2024-03-22T09:58:57Z

pkg/compose/up.go

@@ -124,7 +146,7 @@ func (s *composeService) Up(ctx context.Context, project *types.Project, options
 		return err
 	})

-	if options.Start.Watch {
+	if options.Start.Watch && !options.Start.NavigationMenu {


IIUC --watch would then not start watch mode until menu is disabled. Why ?

because this is being managed inside Keyboard Manager, I need the context to start/stop watch mode afterwards

If the --watch is passed and the menu is active, this will start the watch command here https://github.com/docker/compose/pull/11605/files#diff-809980e7b57a811e276a5d7951e759d3071d157ebad94f0a52370476a1fbfcedR103-R105

right. yet more spaghetti code to our already confusing "architecture" 😅

ndeloof · 2024-03-22T10:00:32Z

pkg/compose/watch.go

 	if !watching {
 		return fmt.Errorf("none of the selected services is configured for watch, consider setting an 'develop' section")
 	}
-	options.LogTo.Log(api.WatchLogger, "watch enabled")
+	options.LogTo.Log(api.WatchLogger, "Watch started")


I personally preferred "enabled", but that's just me

I agree, even the navigation menu message switches between watch enabled/disabled

Signed-off-by: Joana Hrotko <joana.hrotko@docker.com>

See docker/compose#11605

jhrotko changed the title ~~Open dd compose project~~ Add nav bar with info and error message in compose up (attached) Mar 12, 2024

jhrotko self-assigned this Mar 12, 2024

jhrotko requested review from ndeloof, laurazard, glours and milas March 12, 2024 13:12

ndeloof reviewed Mar 12, 2024

View reviewed changes

jhrotko force-pushed the open-dd-compose-project branch from 6c22546 to 18bcf36 Compare March 12, 2024 15:47

laurazard reviewed Mar 12, 2024

View reviewed changes

jhrotko force-pushed the open-dd-compose-project branch 2 times, most recently from 3bafd93 to 3a009bc Compare March 13, 2024 11:45

ndeloof reviewed Mar 13, 2024

View reviewed changes

jhrotko force-pushed the open-dd-compose-project branch from 1bbc729 to ae6b3fa Compare March 15, 2024 11:47

jhrotko changed the title ~~Add nav bar with info and error message in compose up (attached)~~ Add navigation menu with in compose up (attached) Mar 16, 2024

jhrotko force-pushed the open-dd-compose-project branch from 85dd5e0 to 9dc8a12 Compare March 16, 2024 20:07

jhrotko marked this pull request as ready for review March 16, 2024 20:18

jhrotko force-pushed the open-dd-compose-project branch 4 times, most recently from b0d1d2d to 11189b2 Compare March 18, 2024 10:39

jhrotko force-pushed the open-dd-compose-project branch from ce514e9 to 63fb90c Compare March 19, 2024 15:35

laurazard reviewed Mar 19, 2024

View reviewed changes

docs/reference/docker_compose_up.yaml Outdated Show resolved Hide resolved

laurazard reviewed Mar 19, 2024

View reviewed changes

jhrotko force-pushed the open-dd-compose-project branch 2 times, most recently from 43a5fb5 to aec5805 Compare March 19, 2024 17:12

jhrotko added the kind/feature label Mar 20, 2024

jhrotko force-pushed the open-dd-compose-project branch from 68d5908 to d0ce3bb Compare March 20, 2024 14:43

dock-tor approved these changes Mar 21, 2024

View reviewed changes

jhrotko force-pushed the open-dd-compose-project branch 6 times, most recently from c7f443b to 33b4f1d Compare March 21, 2024 18:01

milas approved these changes Mar 22, 2024

View reviewed changes

ndeloof reviewed Mar 22, 2024

View reviewed changes

jhrotko force-pushed the open-dd-compose-project branch 5 times, most recently from 4ed8a30 to fc8b195 Compare March 22, 2024 16:26

Add Navigation Menu to compose up

831a5af

Signed-off-by: Joana Hrotko <joana.hrotko@docker.com>

jhrotko force-pushed the open-dd-compose-project branch from fc8b195 to 831a5af Compare March 22, 2024 16:28

ndeloof enabled auto-merge (rebase) March 22, 2024 16:31

ndeloof merged commit e9dc820 into docker:main Mar 22, 2024
32 checks passed

BrewTestBot mentioned this pull request Mar 22, 2024

docker-compose 2.26.0 Homebrew/homebrew-core#166922

Merged

matt9ucci added a commit to matt9ucci/DockerCompletion that referenced this pull request Apr 8, 2024

Compose 2.26.1: new --menu (Experimental)

df1b28f

See docker/compose#11605

	DoneColor colorFunc = aec.BlueF.Apply
	TimerColor colorFunc = aec.BlueF.Apply
	CountColor colorFunc = aec.YellowF.Apply
	WarningColor colorFunc = aec.YellowF.With(aec.Bold).Apply
	SuccessColor colorFunc = aec.GreenF.Apply
	ErrorColor colorFunc = aec.RedF.With(aec.Bold).Apply
	PrefixColor colorFunc = aec.CyanF.Apply

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

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

Conversation

jhrotko commented Mar 12, 2024 • edited Loading

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

jhrotko Mar 16, 2024 • edited Loading

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

laurazard commented Mar 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 18, 2024 • 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

jhrotko Mar 20, 2024 • edited Loading

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

milas left a 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

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

jhrotko Mar 22, 2024 • edited Loading

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

jhrotko commented Mar 12, 2024 •

edited

Loading

jhrotko Mar 16, 2024 •

edited

Loading

codecov bot commented Mar 18, 2024 •

edited

Loading

jhrotko Mar 20, 2024 •

edited

Loading

jhrotko Mar 22, 2024 •

edited

Loading