Skip to content

Conversation

@Pertempto
Copy link
Contributor

@Pertempto Pertempto commented Oct 24, 2025

  • Fix dockerfile
  • Improve Gin router config for production
  • Figure out how to get database file to work properly inside docker

@github-actions
Copy link
Contributor

github-actions bot commented Oct 24, 2025

Review

Summary of Changes

  • Dockerfile switched to Debian Bookworm, enabled CGO for SQLite compatibility, added necessary dependencies and persistent volume mounts.
  • Server configuration improved for production: environment-based Gin mode, custom minimal logger, and graceful shutdown.
  • Storage updated for flexible path handling, defaulting to /app/data/simple-sync.db inside the container.
  • Added data/ to .dockerignore and .gitignore for better Docker builds and repository hygiene.

Overall Feedback

No changes requested—these updates address the CGO issue for SQLite and add production-ready features like graceful shutdown. The volume mounts should resolve the database persistence TODO. Excellent work improving the Docker setup! 😊

@github-actions
Copy link
Contributor

@Pertempto, the custom logger formatter in src/main.go has a bug: the fmt.Sprintf uses %s for param.StatusCode, which is an int, causing a runtime panic due to format mismatch. Also, it misses param.Protocol and has incorrect ordering, plus an extra placeholder in the format string. Consider fixing it to:

return fmt.Sprintf("%s - %s \"%s %s %s\" %d %s \"%s\"\n",
    param.TimeStamp.Format(time.RFC3339),
    param.ClientIP,
    param.Method,
    param.Path,
    param.Protocol, // Added protocol
    param.StatusCode,
    param.Latency,
    param.ErrorMessage)

Note the %d for status code and added protocol.

@Pertempto
Copy link
Contributor Author

Also, it misses param.Protocol and has incorrect ordering, plus an extra placeholder in the format string

param.Protocol does not exist

@github-actions
Copy link
Contributor

@Pertempto, just a quick check on the TODO in the PR description about getting the database file to work properly inside docker. Any update on that, or is it resolved with these changes?

@Pertempto
Copy link
Contributor Author

@Pertempto, just a quick check on the TODO in the PR description about getting the database file to work properly inside docker. Any update on that, or is it resolved with these changes?

Still working on that...

@Pertempto Pertempto merged commit dc1518a into main Oct 24, 2025
2 checks passed
@Pertempto Pertempto deleted the ae-fix-docker-config branch October 24, 2025 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants