feat: allow pass hostname in docker env

[tomerqodo]

Benchmark PR from qodo-benchmark#419

Summary by CodeRabbit

• New Features

  • Added support for configuring a custom SMTP local hostname through a new optional environment variable, enabling users to specify an alternative hostname for SMTP email communications.

• Chores

  • Updated environment configuration templates and Docker Compose configuration to include the new SMTP local hostname setting across all deployment environments.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Added SMTP_LOCAL_HOSTNAME configuration option to override the local hostname for SMTP HELO/EHLO operations. The feature extends environment configurations, the MailConfig model, and SMTP client initialization across all relevant configuration files and tests.

Changes

Cohort / File(s)	Summary
Environment Configuration api/.env.example, api/tests/integration_tests/.env.example, docker/.env.example, docker/docker-compose.yaml	Added SMTP_LOCAL_HOSTNAME environment variable declarations with empty defaults across config files. Enables hostname override for SMTP operations in different deployment contexts.
SMTP Configuration Model api/configs/feature/__init__.py	Extended MailConfig with new SMTP_LOCAL_HOSTNAME field (str | None) to support optional local hostname configuration with Field documentation.
SMTP Client Implementation api/libs/smtp.py	Modified SMTP client to import dify_config, resolve local_hostname from configuration, and pass local_hostname parameter to SMTP constructor. Refactored TLS/SSL conditional logic to use ternary-based SMTP class selection. Adjusted opportunistic TLS flow to perform EHLO, STARTTLS, EHLO sequence with resolved hostname.
SMTP Client Tests api/tests/unit_tests/libs/test_smtp_client.py	Updated test assertions to expect local_hostname=ANY parameter in SMTP mock constructor calls across TLS/SSL and non-TLS paths. Added ANY import from unittest.mock.
Mail Send Task Tests api/tests/unit_tests/tasks/test_mail_send_task.py	Updated SMTP mock assertions to include local_hostname=ANY parameter across TLS, opportunistic TLS, and standard paths. Added ANY import from unittest.mock.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A little hostname joins the SMTP crew,
Behind NAT walls, we tunnel right through,
HELO and EHLO now speak their true name,
No rejections, no troubles—we're in the game! 📧

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding support for passing SMTP local hostname through environment variables in Docker configuration.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

🧹 Recent nitpick comments

api/tests/unit_tests/libs/test_smtp_client.py (1)

49-67: Consider adding constructor argument assertion for SMTP_SSL branch.

The test_smtp_tls_ssl_branch_and_timeout test validates timeout handling but doesn't assert that local_hostname is passed to the SMTP_SSL constructor, unlike the other two SMTP tests.

💡 Optional: Add constructor assertion

     with pytest.raises(TimeoutError):
         client.send(_mail())
+    mock_smtp_ssl_cls.assert_called_once_with("smtp.example.com", 465, timeout=10, local_hostname=ANY)
     mock_smtp.quit.assert_called_once()

api/libs/smtp.py (1)

32-32: Unnecessary assertion.

The SMTP and SMTP_SSL constructors will always return an instance or raise an exception—they never return None. This assertion serves no runtime purpose.

If this is intended as a type narrowing hint for static analyzers, consider using a type guard or removing the | None from the type annotation on line 24 since the variable is immediately assigned.

💡 Suggested simplification

-        smtp: smtplib.SMTP | None = None
+        smtp: smtplib.SMTP | smtplib.SMTP_SSL
         local_host = dify_config.SMTP_LOCAL_HOSTNAME or ""
         try:
             # Use ternary to select SMTP class based on TLS mode
             smtp = (smtplib.SMTP_SSL if (self.use_tls and not self.opportunistic_tls) else smtplib.SMTP)(
                 self.server, self.port, timeout=10, local_hostname=local_host or None
             )
-
-            assert smtp is not None

Note: This would require updating the finally block check from if smtp: to use a different approach, so the current pattern may be intentional for the finally clause safety. If so, consider keeping the | None annotation but removing the assert.

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ab09726 and 3e5625b.

📒 Files selected for processing (8)

• api/.env.example
• api/configs/feature/__init__.py
• api/libs/smtp.py
• api/tests/integration_tests/.env.example
• api/tests/unit_tests/libs/test_smtp_client.py
• api/tests/unit_tests/tasks/test_mail_send_task.py
• docker/.env.example
• docker/docker-compose.yaml

🔇 Additional comments (15)

api/tests/unit_tests/tasks/test_mail_send_task.py (4)

11-12: Import update looks good.
Including ANY keeps the new optional local_hostname argument flexible in mocks.

---

154-154: SMTP_SSL expectation updated correctly.
The call now accounts for local_hostname, matching the new config.

---

184-184: SMTP expectation updated correctly.
local_hostname=ANY keeps the test resilient to the new optional parameter.

---

216-216: Non‑TLS SMTP expectation updated correctly.
The assertion now matches the updated constructor signature.

api/configs/feature/__init__.py (1)

952-956: Config addition is clear and well‑scoped.
The new SMTP_LOCAL_HOSTNAME field is documented and optional as expected.

api/tests/integration_tests/.env.example (1)

106-107: Env example update is consistent.
Good to expose the new optional SMTP override in integration settings.

docker/docker-compose.yaml (1)

428-428: Docker env propagation looks right.
Adds the new SMTP local hostname override to the shared environment.

docker/.env.example (1)

971-972: Env example update is clear.
Nice to document the optional HELO/EHLO hostname override.

api/.env.example (1)

420-421: Env example update is consistent.
The new SMTP local hostname override is documented clearly.

api/tests/unit_tests/libs/test_smtp_client.py (3)

1-1: LGTM!

The ANY import is correctly added to support the updated mock assertions.

---

20-20: LGTM!

The assertion correctly validates that local_hostname is passed to the SMTP constructor while using ANY to decouple from specific config values.

---

41-41: LGTM!

Consistent with the other test updates for local_hostname handling.

api/libs/smtp.py (3)

6-7: LGTM!

Import correctly added to access the new SMTP_LOCAL_HOSTNAME configuration.

---

24-30: Minor redundancy in local hostname handling.

The conversion dify_config.SMTP_LOCAL_HOSTNAME or "" followed by local_host or None is slightly redundant. However, it's needed because local_host is reused on lines 34 and 36 for ehlo() calls.

The current implementation is functional and correct.

---

33-36: LGTM!

The EHLO → STARTTLS → EHLO sequence is the correct pattern for opportunistic TLS. Passing local_host (even if empty string) to ehlo() is fine since smtplib defaults to the local hostname when given an empty name.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

Warning

Tools execution failed with the following error:

Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error)

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}