- Notifications
You must be signed in to change notification settings - Fork1k
feat(cli): add mock SMTP server for testing scaletest notifications#20221
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
This stack of pull requests is managed byGraphite. Learn more aboutstacking. |
5830a76 to20790fcCompare| iferr:=s.smtpServer.Start();err!=nil { | ||
| returnxerrors.Errorf("start SMTP server: %w",err) | ||
| } | ||
| s.smtpPort=s.smtpServer.PortNumber() |
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.
I think you are doing this so that you can leave the port initially set to 0 and then later query the running port. It feels like a sharp edge, though, that s.SMTPAddress() will show you a 0 port until you start the server. Is there any reason why we don't want to just start the server immediately onNew()?
| NotificationID uuid.UUID`json:"notification_id,omitempty"` | ||
| } | ||
| varnotificationIDRegex=regexp.MustCompile(`notifications\?disabled=3D([a-f0-9-]+)`) |
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.
It seems the=3D is a result of "quoted printable" encoding. We should be decoding the email and then matching. Trying to do a regex match on the encoded content is fragile.
c.f.import "mime/quotedprintable"
| // <p><a href=3D"http://127.0.0.1:3000/settings/notifications?disabled=3D | ||
| // =3D4e19c0ac-94e1-4532-9515-d1801aa283b2" style=3D"color: #2563eb; text-deco= | ||
| // ration: none;">Stop receiving emails like this</a></p> | ||
| replacer:=strings.NewReplacer("=\n","","=\r\n","") |
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.
I think that if you actually decode from "quoted printable" encoding, you will not need this replacement.
| typeEmailSummarystruct { | ||
| Subjectstring`json:"subject"` | ||
| Date time.Time`json:"date"` | ||
| NotificationID uuid.UUID`json:"notification_id,omitempty"` |
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 the notification template ID. For quite a while I thought this was the notification message ID and was very confused, so we need to get the names of things right.
| returntrue | ||
| } | ||
| returnslices.ContainsFunc(recipients,func(rcptPair []string)bool { | ||
| returnlen(rcptPair)>0&&strings.Contains(rcptPair[0],email) |
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.
just usingContains means that a request for "ike@example.com" will also return mail for "spike@example.com"
| Coder deployment values required: | ||
| - CODER_EMAIL_FROM=noreply@coder.com | ||
| - CODER_EMAIL_SMARTHOST=localhost:33199 | ||
| - CODER_EMAIL_HELLO=localhost`, |
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.
these values only apply when testing with Coderd running in the same environment as the SMTP mock, which won't be the case for the majority of our scale testing.
| typeServerstruct { | ||
| smtpServer*smtpmocklib.Server | ||
| httpServer*http.Server | ||
| listener net.Listener |
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.
httpListener?
| listener net.Listener | ||
| logger slog.Logger | ||
| hoststring |
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.
host is ambiguous. This should be hostAddress (as opposed to host name). Also applies to the CLI args.
| func (*RootCmd)scaletestSMTP()*serpent.Command { | ||
| var ( | ||
| hoststring | ||
| portint64 |
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.
smtpPort?
| for { | ||
| select { | ||
| case<-ctx.Done(): | ||
| _,_=fmt.Fprintf(inv.Stdout,"\nTotal messages received: %d\n",srv.MessageCount()) |
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.
misleading. This is messages since last purge, or number of messages in memory, not the total we've ever received.
In another increment or PR it would be useful to have Prometheus metrics for the current number in memory and the total we've ever received.
Uh oh!
There was an error while loading.Please reload this page.
This PR adds a fake SMTP server for scale testing. It collects emails sent during tests, which you can then check using the HTTP API.
Changes
coder scaletest smtpCLI commandHTTP API Endpoints
GET /messages?email=<email>– Get messages sent to an email addressPOST /purge– Clear all messages from memoryThe HTTP API parses raw email messages to extract thedate,subject, andnotification ID.
Notification IDs are sent in emails like this:
CLI
Flags:
--host: Host for the mock SMTP and API server (default: localhost)--port: Port for the mock SMTP server (random if not specified)--api-port: Port for the HTTP API server (random if not specified)--purge-at-count: Max number of messages before auto-purging (default: 100000)