Conversation
geekingfrog
left a comment
There was a problem hiding this comment.
Some additional notes:
- slop machines put useless comments everywhere. Things like
# Set working directory
WORKDIR /app
This all useless garbage, please remove all useless comments that just say what the code is doing. Comments should be to explain the why, not the how. Unless very complex code.
-
Does the autohost automaticallly download engines? Personally, I would drop everything autohost related for now. That can be a different PR. One can still go rather far locally without a autohost setup.
-
the commit history is complete garbage and provide no useful info. Either squash everything into one commit (meh), or take some time and care to craft a meaningful commit history with nice commit message to help reviewer and archeologist who try to figure out the reasoning behind some code.
I haven't even try to run anything, there are too many things to fix first. Once it's in a better shape I'll give it a go.
| with: | ||
| registry: ghcr.io | ||
| username: ${{ github.actor }} | ||
| password: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
what's this github token? Where does that live?
There was a problem hiding this comment.
it is autopopulated by github in actions/workflows, no need to set it up in any way.
.github/workflows/docker-publish.yml
Outdated
| push: true | ||
| tags: ${{ steps.tags.outputs.dev_tags }} | ||
| cache-from: type=gha,scope=dev | ||
| cache-to: type=gha,mode=max,scope=dev |
There was a problem hiding this comment.
Do we really need to push a dev version as well? I'm not convinced
There was a problem hiding this comment.
i wanted to setup an environment for bar-lobby and to run the tachyon setup tasks etc. this dev image is necessary and it would be convenient if you didn't have to build it yourself there.
There was a problem hiding this comment.
speaking about bar-lobby, i personally would not want to drop the autohost stuff because that was the main driver for doing this. but if you prefer i can extract it to a 2nd pull request, let me know.
There was a problem hiding this comment.
https://github.com/thvl3/BAR-Devtools
Might accomplish some of the automated setup you need, I found it worked well enough for me to get bar-lobby working
Dockerfile.dev
Outdated
|
|
||
| # Install hex and rebar | ||
| RUN mix local.hex --force && \ | ||
| mix local.rebar --force |
There was a problem hiding this comment.
why are these 2 tools needed?
There was a problem hiding this comment.
i used other dockerfiles (e.g. the release Dockerfile) as inspiration and it is also installed there, afaik hex is a package manager for elixir that is used to install the dependencies and the other one is for native compiling some libraries? i'll test if it is really required to install it
Edit: went with a totally different approach and reused the main Dockerfile and just added a dev stage, should improve maintainability by a lot
| echo "Waiting for PostgreSQL..." | ||
| while true; do | ||
| set +e | ||
| CREATE_OUTPUT=$(mix ecto.create 2>&1) | ||
| CREATE_STATUS=$? | ||
| set -e | ||
|
|
||
| echo "$CREATE_OUTPUT" | ||
|
|
||
| if [ $CREATE_STATUS -eq 0 ] || echo "$CREATE_OUTPUT" | grep -qi "already exists"; then | ||
| break | ||
| fi | ||
|
|
||
| echo "PostgreSQL not ready yet, retrying..." | ||
| sleep 2 | ||
| done | ||
| echo "PostgreSQL is ready" |
There was a problem hiding this comment.
No, this is not the responsability of the container running teiserver to check postgres is there. If you want such dependency, compose is there for that.
Also, this is for dev, if teiserver cannot start because pg isn't available, the user can figure out what to do.
|
|
||
| echo "Running migrations..." | ||
| mix ecto.migrate | ||
| echo "Migrations complete" |
There was a problem hiding this comment.
teiserver already does that in the application code.
There was a problem hiding this comment.
Yes it does, but if the database is not setup i can't run tachyon setup steps.
README.md
Outdated
| For a quick local stack (Postgres, SMTP, metrics, Grafana, recoil-autohost, teiserver), use the compose setup in this repo. | ||
|
|
||
| - Linux/macOS: `./docker/start.sh` | ||
| - Windows (PowerShell): `./docker/start.ps1` |
There was a problem hiding this comment.
ditch this and just put the command to run.
README.md
Outdated
|
|
||
| The start scripts auto-select `podman` when present, otherwise `docker` (or honor `CONTAINER_RUNTIME` if set). | ||
|
|
||
| This starts `docker-compose.dev.yml`, builds the `teiserver` dev image, runs migrations, and sets up fake data plus tachyon OAuth apps. |
There was a problem hiding this comment.
Drop this, it's going to fall out of sync. "This setup and starts everything that you should need for local dev" is enough.
There was a problem hiding this comment.
Why do you want to push dev images? I would drop the entire thing
There was a problem hiding this comment.
for bar-lobby so you can run one command and have a working setup.
| @default_autohost_bot_name "tachyon-autohost" | ||
|
|
||
| defp autohost_bot_name, | ||
| do: System.get_env("AUTOHOST_BOT_NAME") || @default_autohost_bot_name |
There was a problem hiding this comment.
Drop that, just hardcode the entire thing. If someone needs a different name/bot, they can do that by themselves.
sorry i am used to squashing on merge commit so i usually don't write meaningful commit messages. i'll invest some time to improve it and keep it in mind for the future. Thanks for the thorough code review! Autohost will download engines automatically soon, once the PR is merged. I'll split the autohost stuff to a seperate PR to simplify it. |
…t by reusing the existing Dockerfile
…or dev and release to github registry. dev image is published so it can be used by other repos like bar-lobby to start a local environment quickly
…s by adding additional env variables and default values to the config
…server - clean up postgres installation from mise.toml and adjust default env variables for smtp integration
d479062 to
4ff9f9f
Compare
…asks to run in docker contexts, make fake generation task idempotent
4ff9f9f to
9aaaa7c
Compare
geekingfrog
left a comment
There was a problem hiding this comment.
I think one last thing I'd like to see is putting everything in the same host network.
Because if I want to run some component, typically teiserver, on the host (with iex -S mix phx.server, which gives me a shell onto the running app, which is super handy), then things like victoriametrics don't work.
I tested that on linux and it works fine, but I'm not sure how/if that would work on windows, do you have any thoughts about that?
| end | ||
|
|
||
| if already_has_data do | ||
| IO.puts("Demo data already present, skipping generation") |
There was a problem hiding this comment.
nitpick: prefer Mix.shell().info()
Yeah i see that issue as well, the start scripts and accessing everything with the network ip would have solved that problem. I'll give it a thought and try some ideas. |
|
If there's no other way than the bash templating then so be it, but that seems rather convoluted tbh. Especially since we can make assumptions for a dev machine, like ports and whatnot. |
|
i'll look at the remaining issues next week, didnt forget about it |
This PR introduces a docker compose dev environment for Teiserver so devs can run a full local stack (DB, SMTP, metrics, Grafana, Teiserver, and recoil-autohost) with minimal manual setup. Solves issue #988
I didn't test with podman, not sure if it is fully compatible
Highlights
docker-compose.dev.ymlwith:teiserver,postgres,mailhog,victoriametrics,grafanamise.tomland README usage docs.Bootstrapping changes
mix teiserver.tachyon_setupnow correctly uses TEI_DB_HOSTNAME for connection instead of defaulting to localhost always by requiring app.startCI
Test video
SMTP / metrics
chrome_3HGyONiAEa.mp4
AI Disclosure
I used GPT 5.3-codex and Claude 4.6 Sonet & Opus, mostly for troubleshooting and generating boilerplate AND for the changes to the elixir code that is setting up data, this needs thorough review because i don't know much about elixir yet.