-
Notifications
You must be signed in to change notification settings - Fork 182
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
Enhancements: configurable database file path, default port, and optimised Docker image #109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the changes! Just a few review comments (and a question).
main.go
Outdated
@@ -34,6 +34,9 @@ func main() { | |||
http.HandleFunc("/get", GetCommentsHandler) | |||
|
|||
port := os.Getenv("COMMENTO_PORT") | |||
if port == "" { | |||
port = "8080" | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already done: see config.go:12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, that's right. For some reason it didn't work at some point. I should've checked config.go
. This will be resolved.
main.go
Outdated
fp := os.Getenv("COMMENTO_DATABASE_FILE") | ||
if fp == "" { | ||
fp = "sqlite3.db" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be performed in config.go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
main.go
Outdated
fp = "sqlite3.db" | ||
} | ||
|
||
err = LoadDatabase("sqlite:file=" + fp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do os.Getenv("...")
directly after you've made the config.go change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done as well
.editorconfig | ||
.gitignore | ||
Dockerfile | ||
example/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newline at EOF please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, this isn't being reflected on the Github editor / viewer. But I've definitely added it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now, weirdly.
RUN go get -v . | ||
RUN go install . | ||
RUN go build -ldflags '-linkmode external -extldflags -static -w' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a review comment, just curious: what does this do? I haven't been writing Go long enough ^^
Searching tells me -linkmode external
can be used to load go variables at compile time. Is that the reasoning?
-static
is static linking, I can tell. -w
is some kind of executable size reduction technique from what I read (add -s
as well?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I went with this versus a standard go build
is because I wanted to build a static binary from the golang
Docker image, and still be able to use it in alpine
. There are a couple of problems if we rely on the former.
One of which is your project's use of mattn/go-sqlite3
. It is a cgo
package, not a Go package. Your project however, is in Go. So we have situation where we need a combination of Go, and non-Go code to be in the final executable without dynamic libraries. So, -linkmode external
simply tells cmd/linker
to invoke the host linker (often gcc
) to combine your Go code (compiled by Go to an object file go.o
, but not processed any further) with your non-Go code.
It is also possibly using cgo
from the net
library (see https://golang.org/pkg/net/#hdr-Name_Resolution).
Everything after -extldflags
are flags for gcc
(-w
to suppress warnings IIRC).
TL;DR: I just want a static binary. If I relied on the standard go build
, we'd get an 'output' that relies on dynamic linking, and it will not be executable on alpine
.
(see mattn/go-sqlite3#384 (comment) for more information)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, thanks for the explanation.
(PS: @MrSaints you'll need to use git-rebase to amend the commits; don't use merge commits or add a new commit on top of the three.) |
Set it to `/data/` by default for Docker
… size The main goal here is to optimise for caching, build speed and final image size. It takes advantage of a multi-stage Docker build. - Build backend, and frontend in separate images - Copy artifacts from build images to final, execution image - Add `.dockerignore` to speed up build times (and improve caching) - Copy, and install npm packages before building (again, to improve caching) - Build Go binary statically The final size is under 20MB compared to the hundreds it was before.
@adtac Cheers for the review. I've made the appropriate changes. |
Configurable database file path
The sqlite3 database file path is currently hardcoded. It'd be great if it can be configured. For example, if we were to mount a Docker volume for persistence, it'd be far easier if we could mount a data directory rather than a specific file which most implementations do not nicely support, e.g. Rancher.
Default port
I noticed that there's no default set, and the binding can be set to
:
. I have set it to8080
as per the default environment variable for it.Optimised Docker image
The main goal here is to optimise for caching, build speed and final image size. It takes advantage of a multi-stage Docker build.
.dockerignore
to speed up build times (and improve caching)The final size is under 20MB compared to the hundreds.