Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

WIP: dockerization #16

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

WIP: dockerization #16

wants to merge 2 commits into from

Conversation

FND
Copy link
Contributor

@FND FND commented Jul 21, 2021

No description provided.

README.md Outdated

* run:

$ docker run -it --rm --name naveed -p8080:8465
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this assumes specific port numbers

README.md Outdated
* run:

$ docker run -it --rm --name naveed -p8080:8465
-v "$(pwd)/config:/app/config" \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this assumes there's a config directory from which we can retrieve our configuration files (customized naveed.ini and tokens.cfg) - should this perhaps reside within Dockerfile instead?

naveed

note that you will need to add the configuration files mentioned above to
the container and you might also need to adjust the port numbers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

vague hand-waving

- use multi-stage build: build the binary in a `alpine:golang3.11` image
  but use `busybox:stable` for the run time image and just pick the
  build artifacts ... this reduces the image size from 384mb -> 12.4mb
- build with CGO_ENABLED=0 to avoid dependency to glibc: this makes it
  more compatible with different types of linux images
- add EXPOSE statement to Dockerfile
- add `USER=nobody` statement to avoid running as root within the
  container
- remove `-it` option from README: this is only needed if you want to
  interactively use the container
- use 0.0.0.0 as the default listing host ... otherwise requests from
  other hosts will not be responded to

However: `sendmail` is missing from the container and therefore this
will not run correctly. As I don't know whether it usually runs with
sendmail I stopped at this point -- happy to support again
Copy link
Contributor Author

@FND FND left a comment

Choose a reason for hiding this comment

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

thanks @kesselborn! I've left a few comments, mostly as reminders for myself, though some are questions for my edification, if you're inclined to enlighten me

RUN go build -o bin/naveed
CMD ["/app/bin/naveed"]

FROM busybox
Copy link
Contributor Author

Choose a reason for hiding this comment

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

what's this used for?

Choose a reason for hiding this comment

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

this is a "multistage" Docker image: its as if you have a two docker files in one but you can access the first docker image. I build the binary in t he golang:alpine image (which contains the go compiler, etc.) and copy the result in a new image that is based on busybox, a minimal Linux Distro. This way, the resulting image is much smaller (384mb vs. 12mb)

FROM busybox

COPY --from=base /app/bin/naveed /usr/bin/naveed
COPY --from=base /app/naveed.ini /
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this works because / is CMD's working directory?

Choose a reason for hiding this comment

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

correct

COPY --from=base /app/bin/naveed /usr/bin/naveed
COPY --from=base /app/naveed.ini /

EXPOSE 8465/tcp
Copy link
Contributor Author

Choose a reason for hiding this comment

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

8465 is just the default, so this assumes people didn't customize this setting in naveed.ini - which should generally be ok, but might result in confusion?

Choose a reason for hiding this comment

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

yeah .. you can remove it as well. For docker images it would be more explicit when the port (and the listening address) could be passed as options or env vars

@@ -2,7 +2,7 @@
# * relative paths originate from the application's working directory
# * changes require restarting the server

host = "localhost"
host = "0.0.0.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure we want this to be the default though?

Choose a reason for hiding this comment

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

mmm ... ok: your choice. For production localhost doesn't make sense unless this runs on a host that contains a reverse proxy

$ docker run -it --rm --name naveed -p8080:8465
-v "$(pwd)/config:/app/config" \
$ docker run --rm --name naveed -p8080:8465
-v "$(pwd)/config:/" \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

that config dir seems bogus; it's not actually being used anywhere and doesn't exist by default either - overall the instructions here are confusing; at the very least, assumptions need to be made explicit

Choose a reason for hiding this comment

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

yup ... this adjustment was just made to keep it consistent with the prior image (as I moved the default config file to /)
.
Thinking about this again: this is actually bullshit: it would overwrite / :D .. if that's even possible -- sorry

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

Successfully merging this pull request may close these issues.

2 participants