Batch access list review reminders and provide link (slack) #43782
Conversation
| @@ -192,12 +225,12 @@ func (a *App) notifyForAccessListReviews(ctx context.Context, accessList *access | |||
| // If the current time before the notification start time, skip notifications. | |||
| if now.Before(notificationStart) { | |||
| log.Debugf("Access list %s is not ready for notifications, notifications start at %s", accessList.GetName(), notificationStart.Format(time.RFC3339)) | |||
| return nil | |||
| return nil, nil | |||
There was a problem hiding this comment.
| return nil, nil | |
| return []common.Recipient,, nil |
I would assume that:
val, err := getRecipientsRequiringReminders
if err != nil {
return err
}
val = append(val, common.Recipient{...}) will not panic
There was a problem hiding this comment.
if i understood you correctly, iterating over a nil slice and spreading a nil slice, doesn't result in panic: https://go.dev/play/p/VbdQF2X6s5f
There was a problem hiding this comment.
My bad. I always confusing when I'm seeing return nil, nil
| SendReviewReminders(ctx context.Context, recipient common.Recipient, accessList *accesslist.AccessList) error | ||
| // SendBatchedReviewReminder will send a batched review reminder. | ||
| SendBatchedReviewReminder(ctx context.Context, recipient common.Recipient, accessLists []*accesslist.AccessList) error |
There was a problem hiding this comment.
Does it make sens to reduce interface and instead of having 2 methods
have only one:
SendReviewReminders(ctx context.Context, recipient common.Recipient, accessLists []*accesslist.AccessList) errorand distinguish the flow internally depending on len(accessLists) ?
| remindersLookup := make(map[string][]*accesslist.AccessList) | ||
| recipientLookup := make(map[string]common.Recipient) |
There was a problem hiding this comment.
why not
| remindersLookup := make(map[string][]*accesslist.AccessList) | |
| recipientLookup := make(map[string]common.Recipient) | |
| remindersLookup := make(map[common.Recipient][]*accesslist.AccessList) |
where the recipientLookup can be removed because remindersLookup already have all necessary information
There was a problem hiding this comment.
that's wild... made the change 👍
| } | ||
|
|
||
| return trace.Wrap(a.sendMessages(ctx, accessList, allRecipients, now, notificationStart)) | ||
| return recipients, err |
There was a problem hiding this comment.
| return recipients, err | |
| return recipients, nil |
kimlisa
force-pushed
the
lisa/batch-access-list-review-reminder
branch
2 times, most recently
from
f72c524 to
667a6b6
Compare
kimlisa
force-pushed
the
lisa/batch-access-list-review-reminder
branch
from
667a6b6 to
00ae5a0
Compare
| if _, ok := remindersLookup[recipient]; !ok { | ||
| remindersLookup[recipient] = []*accesslist.AccessList{} | ||
| } | ||
| remindersLookup[recipient] = append(remindersLookup[recipient], accessList) |
There was a problem hiding this comment.
append will return a new list if the first argument is nil 👍
| if _, ok := remindersLookup[recipient]; !ok { | |
| remindersLookup[recipient] = []*accesslist.AccessList{} | |
| } | |
| remindersLookup[recipient] = append(remindersLookup[recipient], accessList) | |
| remindersLookup[recipient] = append(remindersLookup[recipient], accessList) |
| log.WithError(err).Warn("Error notifying for access list reviews") | ||
| recipients, err := a.getRecipientsRequiringReminders(ctx, accessList) | ||
| if err != nil { | ||
| log.WithError(err).Warn("Error getting recipients to notify for access list reviews") |
There was a problem hiding this comment.
Would it be helpful to log the access list name?
| } | ||
| } | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
This err is not relevant to the current operation. I think you meant to check for errs instead.
| if err != nil { | |
| if len(errs) > 0 { |
public-teleport-github-review-bot
bot
removed the request for review
from tigrato
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
part of https://github.com/gravitational/teleport.e/issues/3571
Instead of sending a reminder PER access list review due, it now only sends one (batched or singular) and includes link to the access list page. This required to gather all access list upfront (instead of processing reminders per page), putting it into a lookup map by recipients.
If a recipient has >1 access list reviews due, the messaging is batched, with the earliest due date mentioned, and links out to the access list listing page:
If a recipient has only one review due, it keeps the previous messaging, including a link out to the specific access list:
changelog: For slack integration, Access List reminders are batched into 1 message and provides link out to the web UI.