Add retries to sendgrid emailer (#618) #6164
Conversation
Code Review Agent Run #63207aActionable Suggestions - 2
Review Details
AI Code Review powered by |
Signed-off-by: Katrina Rogan <katroganGH@gmail.com>
katrogan
force-pushed
the
union/upstream-6b28174a9b7cefe9e612668e8d9cbb85b93ee43a
branch
from
95b4554 to
665ec44
Compare
Changelist by BitoThis pull request implements the following key changes.
|
| @@ -79,5 +98,6 @@ func NewSendGridEmailer(config runtimeInterfaces.NotificationsConfig, scope prom | |||
| return &SendgridEmailer{ | |||
| client: sendgrid.NewSendClient(getAPIKey(config.NotificationsEmailerConfig.EmailerConfig)), | |||
| systemMetrics: newEmailMetrics(scope.NewSubScope("sendgrid")), | |||
| cfg: &config, | |||
There was a problem hiding this comment.
Consider initializing the cfg field with just the required NotificationsEmailerConfig instead of storing the entire config struct. This would reduce memory usage and coupling.
Code suggestion
Check the AI-generated fix before applying
| cfg: &config, | |
| cfg: &config.NotificationsEmailerConfig, |
Code Review Run #63207a
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| t.Run("succeed within allowed retry attempts", func(t *testing.T) { | ||
| t.Run("exhaust all retry attempts", func(t *testing.T) { |
There was a problem hiding this comment.
Consider removing the nested t.Run() with identical description. The outer and inner test cases have the same name 'exhaust all retry attempts'.
Code suggestion
Check the AI-generated fix before applying
- t.Run("exhaust all retry attempts", func(t *testing.T) {
ctx := context.TODO()
sendgridClient := &mocks.SendgridClient{}
expectedEmail := getSendgridEmail(emailNotification)
@@ -165,7 +164,6 @@
# TYPE goodemailer:send_error counter
goodemailer:send_error 0
`)))
- })
})
Code Review Run #63207a
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6164 +/- ##
==========================================
+ Coverage 37.02% 37.04% +0.02%
==========================================
Files 1317 1318 +1
Lines 132534 132568 +34
==========================================
+ Hits 49067 49107 +40
+ Misses 79221 79211 -10
- Partials 4246 4250 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Code Review Agent Run #255086Actionable Suggestions - 1
Review Details
AI Code Review powered by |
| var ( | ||
| emailNotification = &admin.EmailMessage{ |
There was a problem hiding this comment.
Consider keeping test data within test functions rather than as package-level variables to maintain test isolation and clarity. Each test should be self-contained with its own test data.
Code suggestion
Check the AI-generated fix before applying
-var (
- emailNotification = &admin.EmailMessage{
- SubjectLine: "Notice: Execution \"name\" has succeeded in \"domain\".",
- SenderEmail: "no-reply@example.com",
- RecipientsEmail: []string{
- "my@example.com",
- "john@example.com",
- },
- Body: "Execution \"name\" has succeeded in \"domain\". View details at " +
- "<a href=\"https://example.com/executions/T/B/D\">" +
- "https://example.com/executions/T/B/D</a>.",
- }
-)
@@ -43,1 +43,14 @@
func TestGetEmail(t *testing.T) {
+ emailNotification := &admin.EmailMessage{
+ SubjectLine: "Notice: Execution \"name\" has succeeded in \"domain\".",
+ SenderEmail: "no-reply@example.com",
+ RecipientsEmail: []string{
+ "my@example.com",
+ "john@example.com",
+ },
+ Body: "Execution \"name\" has succeeded in \"domain\". View details at " +
+ "<a href=\"https://example.com/executions/T/B/D\">" +
+ "https://example.com/executions/T/B/D</a>.",
+ }
Code Review Run #255086
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Future-Outlier
deleted the
union/upstream-6b28174a9b7cefe9e612668e8d9cbb85b93ee43a
branch
Why are the changes needed?
Nice to have for transient network failures.
What changes were proposed in this pull request?
See PR title
How was this patch tested?
Unit tests
Labels
Done
This is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
Implementation of a retry mechanism for Sendgrid email sender to handle transient network failures, featuring configurable retry attempts and delay settings. The changes introduce a SendgridClient interface and enhanced error logging capabilities. The implementation includes comprehensive test coverage for various retry scenarios to ensure reliable email notifications.Unit tests added: True
Estimated effort to review (1-5, lower is better): 2