feat: allow pass hostname in docker env#6
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for configuring a custom local hostname for SMTP HELO/EHLO commands through a new SMTP_LOCAL_HOSTNAME environment variable. This is useful for deployments behind NAT or when the default hostname causes SMTP server rejections.
Changes:
- Added
SMTP_LOCAL_HOSTNAMEconfiguration field to support custom hostname in SMTP HELO/EHLO - Refactored SMTP client to use ternary operator for TLS mode selection and pass local_hostname parameter
- Updated tests to accommodate the new local_hostname parameter using ANY matcher
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docker/docker-compose.yaml | Added SMTP_LOCAL_HOSTNAME environment variable mapping |
| docker/.env.example | Added SMTP_LOCAL_HOSTNAME configuration with documentation |
| api/.env.example | Added SMTP_LOCAL_HOSTNAME configuration with documentation |
| api/tests/integration_tests/.env.example | Added SMTP_LOCAL_HOSTNAME configuration with documentation |
| api/configs/feature/init.py | Added SMTP_LOCAL_HOSTNAME field to MailConfig with description |
| api/libs/smtp.py | Refactored SMTP client to use local_hostname parameter and simplified TLS mode handling |
| api/tests/unit_tests/tasks/test_mail_send_task.py | Updated test assertions to use ANY matcher for local_hostname parameter |
| api/tests/unit_tests/libs/test_smtp_client.py | Updated test assertions to use ANY matcher for local_hostname parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client.send(_mail()) | ||
|
|
||
| mock_smtp_cls.assert_called_once_with("smtp.example.com", 587, timeout=10) | ||
| mock_smtp_cls.assert_called_once_with("smtp.example.com", 587, timeout=10, local_hostname=ANY) |
There was a problem hiding this comment.
The test changes only verify that local_hostname parameter is passed to SMTP constructors using ANY matcher, but don't actually test the new SMTP_LOCAL_HOSTNAME configuration feature. Consider adding tests that:
- Verify that when SMTP_LOCAL_HOSTNAME is configured (e.g., "custom.hostname.com"), it's correctly passed to smtplib.SMTP/SMTP_SSL constructors and ehlo() calls
- Verify that when SMTP_LOCAL_HOSTNAME is None/empty, the default behavior (None) is used
This would ensure the feature works as intended and catches regressions.
| self.server, self.port, timeout=10, local_hostname=local_host or None | ||
| ) | ||
|
|
||
| assert smtp is not None |
There was a problem hiding this comment.
The assert statement on line 32 is unnecessary. The ternary expression on lines 28-30 always returns either an SMTP or SMTP_SSL instance, so smtp can never be None at this point. This assertion serves no functional purpose and should be removed.
| assert smtp is not None |
|
|
||
| # Assert | ||
| mock_smtp_ssl.assert_called_once_with("smtp.example.com", 465, timeout=10) | ||
| mock_smtp_ssl.assert_called_once_with("smtp.example.com", 465, timeout=10, local_hostname=ANY) |
There was a problem hiding this comment.
The test changes only verify that local_hostname parameter is passed to SMTP constructors using ANY matcher, but don't actually test the new SMTP_LOCAL_HOSTNAME configuration feature. Consider adding tests that:
- Verify that when SMTP_LOCAL_HOSTNAME is configured (e.g., "custom.hostname.com"), it's correctly passed to smtplib.SMTP/SMTP_SSL constructors and ehlo() calls
- Verify that when SMTP_LOCAL_HOSTNAME is None/empty, the default behavior (None) is used
This would ensure the feature works as intended and catches regressions.
| smtp.ehlo(local_host) | ||
| smtp.starttls() | ||
| smtp.ehlo(local_host) |
There was a problem hiding this comment.
Breaking change in EHLO hostname behavior. The original code called ehlo(self.server) which used the SMTP server address for the HELO/EHLO command. The new code calls ehlo(local_host), where local_host is "" when SMTP_LOCAL_HOSTNAME is not configured (line 25).
When ehlo("") is called, Python's smtplib uses socket.getfqdn() to determine the local machine's fully qualified domain name.
This changes the default behavior for existing users:
- Original: ehlo("smtp.example.com") - identifies as the SMTP server address
- New: ehlo("") → uses socket.getfqdn() - identifies as the local machine's FQDN
This is a breaking change that could affect SMTP delivery. Some SMTP servers may have whitelisted the old behavior or could reject connections from hosts identifying with different FQDNs. To maintain backward compatibility while adding the new feature, consider using the server address as fallback: local_host = dify_config.SMTP_LOCAL_HOSTNAME or self.server
Benchmark PR from agentic-review-benchmarks#6