Draft: fix: simplify deployment #1526
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR simplifies the deployment by containerizing Nginx with a custom image and consolidated configuration, refactors SMTP environment variables to use a unified naming scheme across all services, and enhances the worker’s mailer setup by inferring TLS mode based on port. Class diagram for AppFlowy Worker mailer TLS logicclassDiagram
class Config {
+MailerConfig mailer
}
class MailerConfig {
+String smtp_host
+u16 smtp_port
+String smtp_username
+String smtp_email
+String smtp_password
-String smtp_tls_kind (removed, now inferred)
}
class AFWorkerMailer
class Mailer {
+new(username, email, password, host, port, tls_kind)
}
Config --> MailerConfig
AFWorkerMailer --> Mailer
%% Note: smtp_tls_kind is now inferred in get_worker_mailer based on smtp_port
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @henri9813 - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Assignment to smtp_tls_kind is not valid Rust syntax. (link)
General comments:
- In application.rs you’re reassigning
smtp_tls_kindbut declared it immutable—make itlet mut smtp_tls_kind(or refactor to avoid mutation) so it compiles. - The same SMTP environment variables are repeated across multiple services in docker-compose.yml—consider using an env_file or compose extensions (
x-fields) to DRY up the config. - The new nginx.conf is quite large and has many repeated CORS and proxy blocks—consider splitting common settings into include files or using macros to improve readability and maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In application.rs you’re reassigning `smtp_tls_kind` but declared it immutable—make it `let mut smtp_tls_kind` (or refactor to avoid mutation) so it compiles.
- The same SMTP environment variables are repeated across multiple services in docker-compose.yml—consider using an env_file or compose extensions (`x-` fields) to DRY up the config.
- The new nginx.conf is quite large and has many repeated CORS and proxy blocks—consider splitting common settings into include files or using macros to improve readability and maintainability.
## Individual Comments
### Comment 1
<location> `docker-compose.yml:10` </location>
<code_context>
- - ${NGINX_PORT:-80}:80 # Disable this if you are using TLS
+ - ${NGINX_PORT:-80}:80
- ${NGINX_TLS_PORT:-443}:443
- volumes:
- - ./nginx/nginx.conf:/etc/nginx/nginx.conf
- - ./nginx/ssl/certificate.crt:/etc/nginx/ssl/certificate.crt
- - ./nginx/ssl/private_key.key:/etc/nginx/ssl/private_key.key
# You do not need this if you have configured to use your own s3 file storage
</code_context>
<issue_to_address>
Removing nginx volume mounts may break SSL if certificates are not present in the image.
Make sure the Docker image includes the necessary SSL certificates and nginx configuration files to prevent deployment failures.
</issue_to_address>
### Comment 2
<location> `docker-compose.yml:136` </location>
<code_context>
- - APPFLOWY_MAILER_SMTP_USERNAME=${APPFLOWY_MAILER_SMTP_USERNAME}
- - APPFLOWY_MAILER_SMTP_EMAIL=${APPFLOWY_MAILER_SMTP_EMAIL}
- - APPFLOWY_MAILER_SMTP_PASSWORD=${APPFLOWY_MAILER_SMTP_PASSWORD}
- - APPFLOWY_MAILER_SMTP_TLS_KIND=${APPFLOWY_MAILER_SMTP_TLS_KIND}
+ - APPFLOWY_MAILER_SMTP_HOST=${SMTP_HOST}
+ - APPFLOWY_MAILER_SMTP_PORT=${SMTP_PORT}
</code_context>
<issue_to_address>
Removal of APPFLOWY_MAILER_SMTP_TLS_KIND may cause issues for non-standard SMTP ports.
Relying on port-based inference for TLS mode may fail with SMTP providers that use non-standard ports or require explicit TLS settings.
</issue_to_address>
### Comment 3
<location> `services/appflowy-worker/src/application.rs:170` </location>
<code_context>
}
async fn get_worker_mailer(config: &Config) -> Result<AFWorkerMailer, Error> {
+ let smtp_tls_kind = "none"
+ if config.mailer.smtp_port == 465 {
+ smtp_tls_kind = "wrapper"
+ } else if config.mailer.smtp_port == 587 {
</code_context>
<issue_to_address>
Assignment to smtp_tls_kind is not valid Rust syntax.
Declare smtp_tls_kind as mutable using 'let mut smtp_tls_kind = ...' to allow reassignment.
</issue_to_address>
### Comment 4
<location> `services/appflowy-worker/src/application.rs:171` </location>
<code_context>
async fn get_worker_mailer(config: &Config) -> Result<AFWorkerMailer, Error> {
+ let smtp_tls_kind = "none"
+ if config.mailer.smtp_port == 465 {
+ smtp_tls_kind = "wrapper"
+ } else if config.mailer.smtp_port == 587 {
+ smtp_tls_kind = "opportunistic"
+ }
</code_context>
<issue_to_address>
Inferring TLS kind from port may not cover all SMTP configurations.
Some providers use non-standard ports or require explicit TLS settings. Allowing a configurable TLS kind would improve flexibility.
Suggested implementation:
```rust
async fn get_worker_mailer(config: &Config) -> Result<AFWorkerMailer, Error> {
let smtp_tls_kind = config.mailer.smtp_tls_kind.as_str();
let mailer = Mailer::new(
```
1. Add a new field `smtp_tls_kind: String` (or appropriate type) to your mailer config struct (e.g., in `Config` or `MailerConfig`).
2. Update your configuration loading logic (e.g., from environment variables or config files) to support the new `smtp_tls_kind` field.
3. Update documentation and deployment configuration to require/allow setting `smtp_tls_kind` (e.g., "none", "wrapper", "opportunistic").
4. Remove any other logic that infers TLS kind from port elsewhere in the codebase, if present.
</issue_to_address>
### Comment 5
<location> `docker/nginx/nginx.conf:29` </location>
<code_context>
+
+
+server {
+ ssl_certificate /etc/nginx/ssl/certificate.crt;
+ ssl_certificate_key /etc/nginx/ssl/private_key.key;
+
+ listen 80;
</code_context>
<issue_to_address>
No fallback or error handling for missing SSL certificates.
Document the required SSL files or add a startup check to verify their presence, as nginx will not start without them.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
server {
ssl_certificate /etc/nginx/ssl/certificate.crt;
ssl_certificate_key /etc/nginx/ssl/private_key.key;
listen 80;
=======
#
# NOTE: The following server block requires SSL certificate and key files to be present at:
# /etc/nginx/ssl/certificate.crt
# /etc/nginx/ssl/private_key.key
# Nginx will fail to start if these files are missing.
# Ensure these files exist before starting the container.
#
# You may add a startup script to check for these files, e.g.:
# [ -f /etc/nginx/ssl/certificate.crt ] && [ -f /etc/nginx/ssl/private_key.key ] || { echo "Missing SSL files"; exit 1; }
#
server {
ssl_certificate /etc/nginx/ssl/certificate.crt;
ssl_certificate_key /etc/nginx/ssl/private_key.key;
listen 80;
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| - APPFLOWY_MAILER_SMTP_USERNAME=${APPFLOWY_MAILER_SMTP_USERNAME} | ||
| - APPFLOWY_MAILER_SMTP_EMAIL=${APPFLOWY_MAILER_SMTP_EMAIL} | ||
| - APPFLOWY_MAILER_SMTP_PASSWORD=${APPFLOWY_MAILER_SMTP_PASSWORD} | ||
| - APPFLOWY_MAILER_SMTP_TLS_KIND=${APPFLOWY_MAILER_SMTP_TLS_KIND} |
There was a problem hiding this comment.
issue (bug_risk): Removal of APPFLOWY_MAILER_SMTP_TLS_KIND may cause issues for non-standard SMTP ports.
Relying on port-based inference for TLS mode may fail with SMTP providers that use non-standard ports or require explicit TLS settings.
Comment on lines
170
to
171
| let smtp_tls_kind = "none" | ||
| if config.mailer.smtp_port == 465 { |
There was a problem hiding this comment.
issue (bug_risk): Assignment to smtp_tls_kind is not valid Rust syntax.
Declare smtp_tls_kind as mutable using 'let mut smtp_tls_kind = ...' to allow reassignment.
Comment on lines
+171
to
+173
| if config.mailer.smtp_port == 465 { | ||
| smtp_tls_kind = "wrapper" | ||
| } else if config.mailer.smtp_port == 587 { |
There was a problem hiding this comment.
suggestion: Inferring TLS kind from port may not cover all SMTP configurations.
Some providers use non-standard ports or require explicit TLS settings. Allowing a configurable TLS kind would improve flexibility.
Suggested implementation:
async fn get_worker_mailer(config: &Config) -> Result<AFWorkerMailer, Error> {
let smtp_tls_kind = config.mailer.smtp_tls_kind.as_str();
let mailer = Mailer::new(- Add a new field
smtp_tls_kind: String(or appropriate type) to your mailer config struct (e.g., inConfigorMailerConfig). - Update your configuration loading logic (e.g., from environment variables or config files) to support the new
smtp_tls_kindfield. - Update documentation and deployment configuration to require/allow setting
smtp_tls_kind(e.g., "none", "wrapper", "opportunistic"). - Remove any other logic that infers TLS kind from port elsewhere in the codebase, if present.
henri9813
force-pushed
the
simplify-deployment
branch
from
78083da to
2f6f4af
Compare
|
I'm not rust expert, but there's something to made around the TLS configuration for autodetection or having a variable which is optional. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Improve "plug and play" aspect from appflowy.
TODO