-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
Dockerize the application for quick start and easy dev #100
base: master
Are you sure you want to change the base?
Conversation
@estk can you explain why Docker is needed for this? |
Hey @Turbo87, it's certainly not necessary. I have found using docker to make operation of web services a bit smoother. It also has the benefit of making dev install near trivial. Feel free to close this if its not something you see as adding value though. |
README.md
Outdated
@@ -11,11 +10,17 @@ This project contains a webserver that connects to the [OpenGliderNet], | |||
saves the received records to a database for 24 hours and relays all data to | |||
any connected WebSocket clients. | |||
|
|||
## Docker Quickstart |
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.
you can add a new subsection using
Docker Quickstart
------------------------------------------------------------------------------
but IMHO these kinds of instructions belong in a CONTRIBUTING.md
, not the README.md
since the production server is not using docker I wouldn't recommend it as the primary way to do things.
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.
Ok, I'll create a CONTRIBUTING.md
# Cache dependencies | ||
RUN mkdir ogn-web-gateway | ||
WORKDIR /ogn-web-gateway | ||
RUN mkdir src; echo "fn main(){}" > src/main.rs |
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.
why are you creating a dummy file here?
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.
Oh, yeah this is a bit of a bummer, rust doesn't have a way to just download deps, the idea is that if cargo.toml does not change, the deps wont have to be pulled again. See rust-lang/cargo#2644. The only thing is with just a touch
of main.rs the build will exit non-zero.
src/main.rs
Outdated
@@ -91,7 +91,7 @@ fn main() { | |||
|
|||
// Create Http server with websocket support | |||
HttpServer::new(move || build_app(redis_executor_addr.clone(), gateway.clone())) | |||
.bind("127.0.0.1:8080") | |||
.bind("0.0.0.0: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.
we should probably make that configurable via env var or CLI option instead 🤔
--host 0.0.0.0 --port 8080
or something like that
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.
Will do!
@Turbo87 I think I've addressed everything, lmk if there is anything else or something new. |
@estk can you extract the CLI options code to a dedicated branch/pull request? I understand that it is somewhat related to the docker changes, but I prefer pull requests that do only one thing :) |
Will do |
I hope my changes to the readme are ok, I wasn't sure how to create a subsection so just changed to the
#
notation in md.