Provide a docker image option about lazydocker HOT 31 CLOSED

yes eventually but I don't want to merge it until I know it works for our use case :) Also we're using go modules now, not dep

from lazydocker.

Docui also has a docker run command: docker run --rm -itv /var/run/docker.sock:/var/run/docker.sock skanehira/docui

At the moment i think this is not possible due to missing feather in gocui, (For details see awesome-gocui/gocui#18)

If that fix is merged into jesses's gocui fork we could make a docker image for lazydocker too.

from lazydocker.

@jesseduffield I think @mjarkk is right. We should apply this patch on gocui, lazygit can benefit from it too and there would be no need to make some workarounds in Dockerfile or twice in two codebases.

from lazydocker.

#36

from lazydocker.

I'm almost done fixing one or two last issues for the pull request to be merged, it should be done on my side by tonight.

from lazydocker.

I only just read this thread then. So to be clear, if we merge that gocui PR (https://github.com/awesome-gocui/gocui/pull/18/files) into my gocui fork, we won't need to add any window-size checking code in #36? Is that correct @dawidd6 @qdm12 ?

If so I'm happy to go ahead and do that now

from lazydocker.

It could be, for now we have the following code to check on the terminal size:

func waitForTerminalSpace() error {
	// before we do anything, we need to check that we have some window space available
	width, height, err := terminal.GetSize(int(os.Stdin.Fd()))
	if err != nil {
		return err
	}
	if width > 0 && height > 0 {
		return nil
	}
	winch := make(chan os.Signal)
	signal.Notify(winch, syscall.SIGWINCH)
	select {
	case <-winch:
		return nil
	case <-time.After(time.Second):
		return fmt.Errorf("there is no available terminal space")
	}
}

And it works pretty well I think. Should I commit it now or wait for you to implement changes to your gocui fork?

from lazydocker.

I think the gocui fork is the right approach. I've got a PR up here #103

from lazydocker.

Would you potentially be able to cherry pick across that commit and see if it works on your branch?

from lazydocker.

Yes I will try this out, although I'm not very familiar with Go dep unfortunately so can't promise anything 😄

But would that solve it really? Wouldn't we need it to be merged to its master branch?

from lazydocker.

So I tried with the branch window-check, and my Docker modifications but it fails, this time with another error:

a1a28 pc=0x803f21
github.com/jesseduffield/lazydocker/pkg/gui.(*Gui).RunWithSubprocesses(0xc000080900, 0xc000314a00, 0x0)        /go/src/github.com/jesseduffield/lazydocker/pkg/gui/subprocess.go:20 +0x2f fp=0xc0001a1ab8 sp=0xc0001a1a78 pc=0x81460f
github.com/jesseduffield/lazydocker/pkg/app.(*App).Run(...)
        /go/src/github.com/jesseduffield/lazydocker/pkg/app/app.go:54
main.main()
        /go/src/github.com/jesseduffield/lazydocker/main.go:62 +0x863 fp=0xc0001a1f98 sp=0xc0001a1ab8 pc=0x827f43
runtime.main()
        /usr/local/go/src/runtime/proc.go:200 +0x20c fp=0xc0001a1fe0 sp=0xc0001a1f98 pc=0x42d77c
runtime.goexit()
        /usr/local/go/src/runtime/asm_amd64.s:1337 +0x1 fp=0xc0001a1fe8 sp=0xc0001a1fe0 pc=0x458591

goroutine 33 [select]:
github.com/jesseduffield/lazydocker/vendor/github.com/jesseduffield/termbox-go.Init.func1()
        /go/src/github.com/jesseduffield/lazydocker/vendor/github.com/jesseduffield/termbox-go/api.go:89 +0x332
created by github.com/jesseduffield/lazydocker/vendor/github.com/jesseduffield/termbox-go.Init
        /go/src/github.com/jesseduffield/lazydocker/vendor/github.com/jesseduffield/termbox-go/api.go:86 +0x76b

goroutine 18 [syscall]:
os/signal.signal_recv(0x0)
        /usr/local/go/src/runtime/sigqueue.go:139 +0x9c
os/signal.loop()
        /usr/local/go/src/os/signal/signal_unix.go:23 +0x22
created by os/signal.init.0
        /usr/local/go/src/os/signal/signal_unix.go:29 +0x41

from lazydocker.

Did you remove all modifications made to pkg/app/app.go?

from lazydocker.

Yes, just the Dockerfile and .dockerignore I forgot to mention

from lazydocker.

After applying this patch it works:

diff --git a/vendor/github.com/jesseduffield/gocui/gui.go b/vendor/github.com/jesseduffield/gocui/gui.go
index 9ea46f5..1cb412f 100644
--- a/vendor/github.com/jesseduffield/gocui/gui.go
+++ b/vendor/github.com/jesseduffield/gocui/gui.go
@@ -878,8 +878,9 @@ func (g *Gui) getTermWindowSize() (int, int, error) {
 	}
 	defer out.Close()
 
+	timeout := time.Second * 3
 	signalCh := make(chan os.Signal, 1)
-	signal.Notify(signalCh, syscall.SIGWINCH, syscall.SIGINT)
+	signal.Notify(signalCh, syscall.SIGWINCH)
 
 	for {
 		_, _, _ = syscall.Syscall(syscall.SYS_IOCTL,
@@ -892,15 +893,10 @@ func (g *Gui) getTermWindowSize() (int, int, error) {
 		}
 
 		select {
-		case signal := <-signalCh:
-			switch signal {
-			// when the terminal window size is changed
-			case syscall.SIGWINCH:
-				continue
-			// ctrl + c to cancel
-			case syscall.SIGINT:
-				return 0, 0, errors.New("There was not enough window space to start the application")
-			}
+		case <-signalCh:
+			continue
+		case <-time.After(timeout):
+			return 0, 0, errors.New("There was not enough window space to start the application")
 		}
 	}
 }

dunno if this is what we want.

from lazydocker.

I just tested this out myself and it looks like the issue was that because our window size struct didn't include the pixel counts (it only included rows/cols) we got a seg fault. I've just pushed a new commit to my branch which resolves this: a680446

A couple things on my mind now

1. I think we should move the docker-compose file out of the readme and into an actual docker-compose file
2. How should we deal with mounting the user's config? If you don't mount the config, you'll always get the prompt at the beginning asking whether you want to report error messages. And it's not always obvious where that config file can be found

from lazydocker.

Hi I'll try it out tomorrow 😄

1. I like having all useful stuff in the readme with collapsibles, but it's just a preference, as you wish!
2. You should bind mount /.config/jesseduffield/lazydocker. Another way would be to have Docker volume described in the Dockerfile so that a bind mount is possible but not needed. I think that's a good solution in the end.

from lazydocker.

sounds good :)

from lazydocker.

Hi @jesseduffield,

The current state of the Docker image lacks the Docker binary too, so should we:

• merge #36 now and then work on issue #109
• Fix issue #109 then merge #36
• Find a workaround for now (i.e. bind mount Docker or use a static binary of Docker) then merge #36 then fix #109 and remove the workaround?

from lazydocker.

I think option 3 is the best. Bind mounting docker means you don't need to worry about a version mismatch, but using a static binary is far more convenient. I'd lean towards the binary side but I'm very convenience-biased. What do you think?

from lazydocker.

Yes I agree with you. What we could however do is have a build argument specifying the docker version to bundle in, so that if someone encounter compatibility issues, he can build the image with an older Docker version.

from lazydocker.

sounds good to me

from lazydocker.

I am pretty much done building and putting docker in /docker in the final scratch image, resulting in a 68.9MB image (not great but not bad either). But what information is gathered using /docker? I am trying to find if it can pick up the docker binary from /docker or if I should place it somewhere else. I am also testing it on a raspberry pi to check it works for ARM.

from lazydocker.

I am trying to find if it can pick up the docker binary from /docker or if I should place it somewhere else.

Place the binary in one of the directories in PATH or create a symlink accordingly.

from lazydocker.

Ok thanks! I will just set the PATH environment variable to PATH=/ in the Scratch container.

How can I test it works? I am not sure what information is gathered from the docker CLI exactly.

I should be done tomorrow (I forgot some code on another computer and don't want to write it again :) )

from lazydocker.

Logs and CPU usage are taken from cli client.

from lazydocker.

Ok thanks! I will just set the PATH environment variable to PATH=/ in the Scratch container.

I am not sure if it is a wise decision to expose / in PATH. Why don't you place the binary in something like /usr/local/bin/ which is usually already in PATH. Alternatively, you can create a symlink like:

$ ln -s /docker /usr/local/bin/docker

from lazydocker.

But scratch image is empty IIRC, so there isn't really any problem with setting the PATH to / as long as there are only binaries in / and that is the case.

from lazydocker.

Users might bind mount files and folders there at run time.

from lazydocker.

So should we put all these binaries in, for example /bin or leave them in /? I don't really see the point of mounting files there, except for /.config

I cannot create symlinks and there is no path defined as its a scratch image.

from lazydocker.

I cannot create symlinks and there is no path defined as its a scratch image.

Yes, I understand that ln would not work in scratch, but standard default value of PATH (/usr/sbin:/usr/bin:/sbin:/bin) should be available even in an empty container. This means, placing binaries in any of these directories should work.

from lazydocker.

Closing because I believe this issue is resolved. Let me know if that's not the case

from lazydocker.