Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reworked Dockerfile for a more production oriented usage #36

Merged
merged 27 commits into from
Jul 21, 2019
Merged

Reworked Dockerfile for a more production oriented usage #36

merged 27 commits into from
Jul 21, 2019

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Jul 1, 2019

  • Added .dockerignore to speed up build context and avoid rebuilding when unecessary
  • Specified Alpine and Go versions as build arguments
  • Specified Go target CPU architecture as build arguments to be able to build for ARM devices
  • Specified base images as build arguments to be able to build for ARM devices
  • Trimmed down size of final image using -a -installsuffix cgo -ldflags="-s -w" go build flags and by copying the statically built binary only to the final image
  • Added clear build and run instructions for the Docker container

- Added .dockerignore to speed up build context and avoid rebuilding when unecessary
- Specified Alpine and Go versions as build arguments
- Specified Go target CPU architecture as build arguments to be able to build for ARM devices
- Specified base images as build arguments to be able to build for ARM devices
- Trimmed down size of final image using `-a -installsuffix cgo -ldflags="-s -w"` go build flags and by copying the statically built binary only to the final image
- Added clear build and run instructions for the Docker container
@qdm12
Copy link
Contributor Author

qdm12 commented Jul 1, 2019

Hi there !

Hi ! I just added that so I can run your program without downloading the binary, especially for die hard of running everything in a Docker container... And added ARM build support too. By the way, it seems your program requires Git to function, is this expected? If not, we could manage to run it in a Scratch container as a static binary which would make it very tiny 👍

@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 1, 2019

Hmm, I think that needing git to function is a bug. The initial codebase was taken from lazygit and maybe something wasn't removed yet.

LGTM. Additionally you may want to add these to go build:

-ldflags \
		"-X main.commit=$(COMMIT) \
		-X main.date=$(shell date -u +%Y-%m-%d) \
		-X main.version=0.2.4 \
		-X main.buildSource=Docker"

I mean, really only version, commit and buildSource would be enough, cause specyfing date could destroy reproducible building.

@qdm12
Copy link
Contributor Author

qdm12 commented Jul 1, 2019

Done, I've added Docker labels with these as well, it might turn to be useful.
By the way, the error without git is:

*errors.errorString invalid dimensions
/go/src/github.com/jesseduffield/lazydocker/vendor/github.com/jesseduffield/gocui/gui.go:164 (0x7d1339)
/go/src/github.com/jesseduffield/lazydocker/pkg/gui/layout.go:84 (0x8010e4)
/go/src/github.com/jesseduffield/lazydocker/vendor/github.com/jesseduffield/gocui/gui.go:387 (0x7d2d90)
/go/src/github.com/jesseduffield/lazydocker/vendor/github.com/jesseduffield/gocui/gui.go:504 (0x7d389e)
/go/src/github.com/jesseduffield/lazydocker/vendor/github.com/jesseduffield/gocui/gui.go:412 (0x7d2fe4)
/go/src/github.com/jesseduffield/lazydocker/pkg/gui/gui.go:309 (0x7f70f3)
/go/src/github.com/jesseduffield/lazydocker/pkg/gui/subprocess.go:20 (0x80725f)
/go/src/github.com/jesseduffield/lazydocker/pkg/app/app.go:54 (0x81a71a)
/go/src/github.com/jesseduffield/lazydocker/pkg/app/app.go:54 (0x81a70d)
/usr/local/go/src/runtime/proc.go:200 (0x42d77c)
/usr/local/go/src/runtime/asm_amd64.s:1337 (0x458541)

Also, is xdg-utils necessary? It works without for me but I wanted to check with you first.

Thanks !

EDIT: Never mind, this issue arises with git from time to time as well. Maybe it's my terminal which sucks, I'm checking now.

@qdm12
Copy link
Contributor Author

qdm12 commented Jul 1, 2019

ok so it seems both git and xdg-utils are not necessary.

However, running the container directly without first starting a shell /bin/sh fails because of the error of gocui above, probably because the screen size is zero or something.

So I will update the instructions, where you need to launch the container and then enter lazydocker for the GUI to work out.

qdm12 added 2 commits July 1, 2019 11:51
- Runs the container with `/bin/sh` initially
- Invoke `lazydocker` from within the container, or gocui fails as the terminal window is invalid at launch
@qdm12
Copy link
Contributor Author

qdm12 commented Jul 1, 2019

It works great on my linux host and my windows host, let me know if I should change anything !

Dockerfile Outdated Show resolved Hide resolved
@jesseduffield
Copy link
Owner

Hey man thanks for making this :) Very impressive.

I left one comment, other than that this LGTM, happy to merge if @dawidd6 is happy with it

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 1, 2019

There seems to be a problem with getting containers logs. It works with normal installation, but not in Docker. @jesseduffield are the logs fetched from command line Docker client?

@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 1, 2019

diff --git a/Dockerfile b/Dockerfile
index 351e334..0abcc0f 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -24,5 +24,6 @@ LABEL org.label-schema.schema-version="1.0.0-rc1" \
     org.label-schema.vcs-description="The lazier way to manage everything docker" \
     org.label-schema.docker.cmd="docker run -it -v /var/run/docker.sock:/var/run/docker.sock lazydocker" \
     org.label-schema.version=${VERSION}
-ENTRYPOINT [ "/bin/sh" ]
+RUN printf "resize > /dev/null\nlazydocker \$@\n" > /bin/run && chmod +x /bin/run
+ENTRYPOINT [ "sh", "/bin/run" ]
 COPY --from=builder /go/src/github.com/jesseduffield/lazydocker/lazydocker /usr/bin/lazydocker
diff --git a/README.md b/README.md
index c651f8f..0af4172 100644
--- a/README.md
+++ b/README.md
@@ -97,8 +97,6 @@ go get github.com/jesseduffield/lazydocker
     docker run -it -v /var/run/docker.sock:/var/run/docker.sock lazydocker
     ```
 
-1. Then enter `lazydocker`
-
 ## Usage
 
 Call `lazydocker` in your terminal. I personally use this a lot so I've made an alias for it like so:

@qdm12 you can apply this patch on top of your sources in reworking-dockerfile branch if you want to immediately run lazydocker instead of first interactively running sh. Would make a pull request to your fork, but... I'm lazy.

@jesseduffield
Copy link
Owner

@dawidd6 yes they are fetched vis the command line. Though there are various places in the app that use the docker client package to do things. Perhaps there are issues due to that?

@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 1, 2019

@jesseduffield heh, so now there are 2 possibilities:

  1. Try to rewrite those calls in pure Go code.
  2. Install docker package in container.

Obviously the second one is an instant fix, but in the long run, would be better to do the first. Though I don't really know if it's feasible, would need to dig deeper into the code.

qdm12 added 3 commits July 1, 2019 15:37
- Git commit is passed as a build argument as explained in README.md
- `.git` directory is ignored by Docker for a quicker build and smaller context
- the build arg VCS_REF is used both to tag the image and the Go program
Thanks to the script:

```sh
#!/bin/sh

resize > /dev/null
lazydocker $@
```
Dockerfile Outdated Show resolved Hide resolved
@jesseduffield
Copy link
Owner

as for the resize thing, perhaps we can something like this in app.go:

func (app *App) Run() error {
	// before we do anything, we need to check that we have some window space available
	for {
		width, _, err := terminal.GetSize(int(os.Stdin.Fd()))
		if err != nil {
			return err
		}
		if width == 0 {
			time.Sleep(time.Millisecond * 50)
		}
		break
	}

	err := app.Gui.RunWithSubprocesses()

	return err
}

I'm in the middle of a long-running migration from dep to go modules right now so I can't actually test that locally :P

qdm12 added 2 commits July 2, 2019 13:57
- Build arguments are injected by the build hook
- Badges added: number of pulls, stars and if the build is automated
@codecov-io
Copy link

Codecov Report

Merging #36 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #36   +/-   ##
=======================================
  Coverage   18.06%   18.06%           
=======================================
  Files          12       12           
  Lines        1052     1052           
=======================================
  Hits          190      190           
  Misses        856      856           
  Partials        6        6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a51bb83...d78648c. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Jul 2, 2019

Codecov Report

Merging #36 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #36   +/-   ##
=======================================
  Coverage   18.06%   18.06%           
=======================================
  Files          12       12           
  Lines        1052     1052           
=======================================
  Hits          190      190           
  Misses        856      856           
  Partials        6        6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a51bb83...7107a4c. Read the comment docs.

hooks/build Show resolved Hide resolved
pkg/app/app.go Outdated Show resolved Hide resolved
@ibnesayeed
Copy link

Actually after some thoughts, it's better to bind mount the Docker binary as its version might be different from the host, potentially causing issues.

Bind mount for the binary is not ideal unless the binary on the host is static and can work cross-OS. I have created a new ticket #109 to use the API instead of the CLI binary.

@jesseduffield
Copy link
Owner

I forgot where we were up to with this. If I recall correctly, it was ready to go? I think there might have been some discussion on another issue but I can't find it now

@qdm12
Copy link
Contributor Author

qdm12 commented Jul 11, 2019

I need this last question resolved

Also now that you use go modules, I need to changed the go binary build stage as it should be different now. I will do that the coming 2 days.

@pascalandy
Copy link

I'll be glad to test if you need help :)

@qdm12
Copy link
Contributor Author

qdm12 commented Jul 16, 2019

Hi all,

I tested my latest Dockerfile with the lastest master code (using modules) and it fails with:

go: finding github.com/davecgh/go-spew v1.1.1
go: finding github.com/client9/misspell v0.3.4
go: finding golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33
go: finding github.com/onsi/ginkgo v1.6.0
go: error loading module requirements
The command '/bin/sh -c go mod download' returned a non-zero code: 1

during my go mod download stage.

The full Dockerfile is:

ARG BASE_IMAGE_BUILDER=golang
ARG ALPINE_VERSION=3.10
ARG GO_VERSION=1.12.6

FROM ${BASE_IMAGE_BUILDER}:${GO_VERSION}-alpine${ALPINE_VERSION} AS builder
ARG GOARCH=amd64
ARG GOARM
RUN apk --update -q --progress add git ca-certificates
RUN go get -u -v golang.org/x/vgo 2> /dev/null
WORKDIR /tmp/gobuild
COPY go.mod go.sum ./
RUN go mod download
COPY ./ .
RUN CGO_ENABLED=0 GOOS=linux GOARCH=${GOARCH} GOARM=${GOARM} go build -a -ldflags="-s -w \
    -X main.commit=${VCS_REF} \
    -X main.version=${VERSION} \
    -X main.buildSource=Docker"

FROM scratch
ARG BUILD_DATE
ARG VCS_REF
ARG VERSION
LABEL org.label-schema.schema-version="1.0.0-rc1" \
    maintainer="jessedduffield@gmail.com" \
    org.label-schema.build-date=$BUILD_DATE \
    org.label-schema.vcs-ref=$VCS_REF \
    org.label-schema.url="https://github.com/jesseduffield/lazydocker" \
    org.label-schema.vcs-description="The lazier way to manage everything docker" \
    org.label-schema.docker.cmd="docker run -it -v /var/run/docker.sock:/var/run/docker.sock lazydocker" \
    org.label-schema.version=${VERSION}
ENTRYPOINT [ "/lazydocker" ]
VOLUME [ "/.config/jesseduffield/lazydocker" ]
COPY --from=builder /tmp/gobuild/lazydocker /lazydocker

Any idea why is this? Thanks!

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 16, 2019

@qdm12

RUN go get -u -v golang.org/x/vgo 2> /dev/null

Why is this needed?

COPY go.mod go.sum ./
RUN go mod download

That's totally unnecessary. See below.

RUN CGO_ENABLED=0 GOOS=linux GOARCH=${GOARCH} GOARM=${GOARM} go build -a -ldflags="-s -w \
    -X main.commit=${VCS_REF} \
    -X main.version=${VERSION} \
    -X main.buildSource=Docker"

Just append to this -mod=vendor so vendored dependencies are used instead of downloading them.

@qdm12
Copy link
Contributor Author

qdm12 commented Jul 16, 2019

It's just to make the build faster, so that dependencies are not downloaded at every build. Do you think it's possible to go mod download and use the vendor directory at the same time?

For now I'll make it a one line build as you suggested but this is quite slow for development purposes.

- Volume did not work properly as the config would be persistent but not shared across restart of the container running interactively.
- Docker run instructions were therefore updated and simplified to bind mount the config directory as volume do not really work in this situation.
As @ibnesayeed mentionned, some environment variables are actually defined even in Scratch Docker images. `PATH` contains `/bin` so there is no need to overwrite it.
@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 16, 2019

But you don't need to download dependencies as they are already available offline in vendor/ and I don't know how can that slow down the build.

@qdm12
Copy link
Contributor Author

qdm12 commented Jul 16, 2019

Hi, that's great then, less work for me 😄 It would be eventually interesting to get rid of the vendor directory and have only git commit references in the go.mod file, or am I missing something out? Thanks.

@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 16, 2019

@jesseduffield likes vendoring, cause it makes it easier to quickly make changes to dependencies and see the results. Plus also with vendoring it is enough to just clone one repo and then build, while without vendoring there is also a need to download dependencies from different sources before building.

So, in conclusion, I think that vendoring will stay here for a while.

@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 16, 2019

I just tried the newest iteration of Dockerfile and it seems like logs, stats, attaching and other stuff handled by docker CLI binary are still not working. I tried using alpine image instead of the scratch one, as I thought it could be an issue with PATH, but it wasn't the case. Dunno really what to do with that.

@qdm12
Copy link
Contributor Author

qdm12 commented Jul 16, 2019

I had my CPU stats working on my side. I'll try attaching to containers as well. Are you sure you are trying the last Dockerfile with the current master branch?

@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 16, 2019

Yep, i just merged this branch from your fork with latest upstream master and I don't get those stats.

@qdm12
Copy link
Contributor Author

qdm12 commented Jul 21, 2019

Hi all, sorry for the long delay.

I get the stats, config and Top. Screenshot below:

image

But I don't get the logs. What are your thoughts on this? I cannot figure out why it does not work.

@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 21, 2019

Me neither. I get the same result.

I'd suggest merging this PR already as it is very outdated comparing to current master (go modules) and open another PR for fixing remaining issues.

@qdm12
Copy link
Contributor Author

qdm12 commented Jul 21, 2019

Yes I wanted to suggest the same thing but didn't want to be rude.

It will still be much better than the current Dockerfile implementation. And, as you pointed out, it'll be easier to develop from a new branch as copying back and forth between repositories (one with modules, one without) isn't great.

@dawidd6 dawidd6 merged commit 25e8775 into jesseduffield:master Jul 21, 2019
@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 21, 2019

It is done. Please open another PR.

@tmardesen-greatamerica
Copy link

I am using the latest Go installation, and I do not have Top or Stats working (This is installed on windows server 2019), what is the work around? To use the dockerfile for Lazydocker?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants