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

fix(dockerfile): bugfix for broken docker build + optimization #4154

Closed
wants to merge 1 commit into from

Conversation

ellerbrock
Copy link
Contributor

Hello Hugo's,

i just found out that the Docker Build is broken, so i fixed that for you.
Further i cleaned up the Dockerfile and updated Alpine Linux to Version 3.7 and the Go Lang Build Image to Version 1.10-rc.

Another PR is coming soon with some further optimizations.

fixed-it

Cheers Maik

@ellerbrock ellerbrock mentioned this pull request Dec 10, 2017
Copy link
Contributor

@tjamet tjamet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice using scratch image!

I am personally thinking that building the local repository rather than cloning it from github is interesting, as well as keeping the local re-build optimisation.

Moreover, I am wondering how duplicate this is with #4077 or even #4155, shall we focus on one?

git \
musl-dev && \
go get github.com/golang/dep/cmd/dep && \
go get github.com/kardianos/govendor && \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not relying on the current working directory?
Wouldn't it be something interesting to be able to generate an image from the current directory (for development or testing for example)?

go get github.com/kardianos/govendor && \
govendor get github.com/gohugoio/hugo && \
cd /go/src/github.com/gohugoio/hugo && \
dep ensure && \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although you consider it breaks docker best practices, you may consider that doing this requires re-building
and re-downloading everything for every single docker build.

It is okay when the build step is built by a remote server that always discards cache, but might be interesting when iterating on docker build locally. Docker has even edited its best practices explaining latest versions of docker have "have mitigated the need" of minimizing the number of layers.

As suggested in #4077 distinguishing installing dependent tools, dependencies and actual binary generation allows to benefit the docker build cache while building the image.
Having a dockerfile of (as set in #4077 ):

RUN apk add --no-cache \
    git \
    musl-dev \
 && go get github.com/golang/dep/cmd/dep

COPY Gopkg.lock Gopkg.toml /go/src/github.com/gohugoio/hugo/
WORKDIR /go/src/github.com/gohugoio/hugo
RUN dep ensure -vendor-only
COPY . /go/src/github.com/gohugoio/hugo/
RUN go install -ldflags '-s -w'

considers that:

  • dep, git, musl-dev are external tools that change the less, without a strong dependency on the exact version
  • go dependencies are changing slower than the code itself, and quite long to install that are worth caching in local developments, and needs to be re-built every time Gopkg.* changes
  • hugo code changes the most often and thus needs to be re-build every time a file is changed

@ellerbrock
Copy link
Contributor Author

@tjamet,

thanks for the explanation from the developer view working on localhost.
makes totally sense and i made a new pr with the requested changes #4157

that's why working with other smart people on open source makes so much fun.

cheers maik

@tjamet
Copy link
Contributor

tjamet commented Dec 12, 2017

Thanks for your consideration @ellerbrock I have proposed kind of the same paragraph on best practices :)

@ellerbrock
Copy link
Contributor Author

yeah i saw that one, nice idea to rewrite the docker best practices :D

@ellerbrock
Copy link
Contributor Author

@tjamet any thoughts in merging it?
and like mentioned here: #4155 i already created a hugoio account for you guys with automated builds. i can give it over to you guys ...

cheers maik

@tjamet
Copy link
Contributor

tjamet commented Jan 19, 2018

If I had to choose, I think I would go for merging #4077 that has the advantage of benefiting build cache for who is using it locally while keeping the same final image
But I am a contributor, I can't decide what to merge or not

@stale
Copy link

stale bot commented May 19, 2018

This issue has been automatically marked as stale because it has not had recent activity. The resources of the Hugo team are limited, and so we are asking for your help.
If this is a bug and you can still reproduce this error on the master branch, please reply with all of the information you have about it in order to keep the issue open.
If this is a feature request, and you feel that it is still relevant and valuable, please tell us why.
This issue will automatically be closed in the near future if no further activity occurs. Thank you for all your contributions.

@stale stale bot added the Stale label May 19, 2018
anthonyfok pushed a commit that referenced this pull request Jun 14, 2018
- Hugo container is based on SCRATCH to further reduce the footprint
  and the vulnerability surface
- Update Alpine image to 3.7 in the build container
- Update Go Lang to 1.10 in the build container
- Add .dockerignore file per the Docker best practices

Closes #4154, #4155, #4157
@github-actions
Copy link

github-actions bot commented Feb 4, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants