Skip to content
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

[WIP] Kamal deployment #1192

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from
Draft

Conversation

yinho999
Copy link
Contributor

@yinho999 yinho999 commented Jan 20, 2025

  • Add cargo loco generate deployment --kind kamal command
  • Create customize config/deploy.yml, .kamal/secrets and Dockerfile for both sqlite and postgresql + redis.
  • Update development.yaml to open 0.0.0.0 bindings.
  • Override the default config/deploy.yml, .kamal/secrets and Dockerfile.
  • Snapshot testing.
  • Documentation of the use case.
  • Blog / Video Guide

Add Kamal deployment configuration with support for PostgreSQL, SQLite, and background queue workers. Includes deployment templates for Docker, secrets management, and deployment configuration.
@yinho999 yinho999 marked this pull request as draft January 20, 2025 01:26
Copy link
Contributor

@kaplanelad kaplanelad left a comment

Choose a reason for hiding this comment

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

I realize that this PR is still a work in progress and has been marked as a draft, but please take a look at my comments.

Comment on lines +217 to +219
sqlite: bool,
postgres: bool,
background_queue: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that none of the parameters (fallback_file, asset_folder, host, port) and also the new params are strictly necessary.
The deployment function should take a reference to &Config (you already have the config in cli.rs), and each deployment type should be able to utilize the configuration that works best for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kaplanelad I was trying to use &loco-rs::config::Config in loco gen project. However, importing loco-rs within loco-gen will cause cyclic dependency

@@ -0,0 +1,47 @@
to: "Dockerfile"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we not using the same docker template that we already have?

I don't think we need to hold two docker templates

Copy link
Contributor Author

@yinho999 yinho999 Jan 20, 2025

Choose a reason for hiding this comment

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

The current docker template has a different purpose than the one kamal one. As I can see the current dockerfile does not include sea orm and seems to be just compiling the binaries without running it. Not sure if I merge them or leave them separated.

Copy link
Contributor

Choose a reason for hiding this comment

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

The question is, why should SeaORM be included in this docker file.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are using sea-orm-cli for generating entities. Why would you need to generate entities inside the Dockerfile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My current dockerfile for kamal setup is serving the server within the container instead of just compiling, which requires database connection like sqlite or postgres. Therefore SeaOrm is kinda required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait sea orm cli is not required for database connection? Okay I will remove it

Copy link
Contributor

Choose a reason for hiding this comment

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

Please check if you can use the current docker file and not create a new one

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