-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(smtp): Add hostname to SMTP #1076
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
8c733b8
feat(smtp): Add hostname to SMTP
BYK 7dc3a4c
Fix comment
BYK 8889df7
review feedback
BYK 0ee22d5
fix one last _from
BYK 19fe4b1
fix invalid python
BYK 68192ec
another _from to _host
BYK 5fe3361
fix tests
BYK File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks right and wrong to me
I was thinking SENTRY_MAIL_HOST was used for the HELO command, but it seems like not
The value of the HELO command can be first.server.company.tld but send mails as domain2.tld
This is what did happen at phpMyAdmin, we send Sentry reports as xxx@phpmyadmin.net and use a HELO value of server.hostingcompany.tld
Could you please link me the actual uses of this two variables ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, they don't need to be in sync. We are just setting sensible defaults for a good out-of-the-box experience. One can change these as freely as they can.
This is the code that uses them: https://github.com/getsentry/sentry/blob/2103815733832c1e463142f225f6bd2652b7e87e/src/sentry/utils/email.py#L277-L283
SENTRY_MAIL_HOSTis not used, it is something I just made up for easy configuration in self-hosted. The mail server settings are here: https://github.com/getsentry/onpremise/blob/674a600770acad37ab8560e323e38ead49b0cf97/sentry/config.example.yml#L10-L16By default we use the
smtpservice based onexim4but it is also fully configurable.