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.

Sign up for GitHub

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

Jump to bottom

Introduce Notifications CRUD Service #38327

Merged

rudream merged 1 commit into master from yassine/notifications/service

Mar 1, 2024

Contributor

rudream commented Feb 16, 2024 •

edited

Loading

Purpose

Part of #37704

This PR introduces the backend CRUD service for creating, updating, deleting and listing Notifications-related resources.

Note: The logic for fetching notifications for a user will be in a separate PR as that implementation is more complex.

rudream added no-changelog backport/branch/v15 labels

rudream commented

View reviewed changes

api/proto/teleport/notifications/v1/notifications_service.proto Show resolved Hide resolved

rudream force-pushed the yassine/notifications/service branch 6 times, most recently from 3eb723a to 61fb59b Compare

February 20, 2024 20:17

rudream marked this pull request as ready for review

February 20, 2024 20:18

rudream requested review from zmb3 and rosstimothy

February 20, 2024 20:18

github-actions bot added the size/lg label

github-actions bot requested review from gzdunek and ravicious

February 20, 2024 20:19

rosstimothy reviewed

View reviewed changes

api/proto/teleport/notifications/v1/notifications_service.proto Outdated Show resolved Hide resolved

api/proto/teleport/notifications/v1/notifications_service.proto Outdated Show resolved Hide resolved

lib/services/local/notifications.go Outdated Show resolved Hide resolved

lib/services/local/notifications.go Outdated Show resolved Hide resolved

lib/services/local/notifications.go Outdated Show resolved Hide resolved

lib/services/notifications.go Show resolved Hide resolved

lib/services/notifications.go Outdated Show resolved Hide resolved

lib/services/notifications.go Outdated Show resolved Hide resolved

lib/services/notifications.go Show resolved Hide resolved

lib/services/notifications.go Outdated Show resolved Hide resolved

rudream mentioned this pull request

Implement Notifications System #37704

Closed

rudream requested a review from rosstimothy

February 22, 2024 02:03

rosstimothy reviewed

View reviewed changes

lib/services/local/notifications.go Outdated Show resolved Hide resolved

lib/services/local/notifications.go Outdated Show resolved Hide resolved

lib/services/local/notifications.go Outdated Show resolved Hide resolved

lib/services/local/notifications.go Outdated Show resolved Hide resolved

lib/services/notifications.go

Comment on lines +81 to +86

+              	if !cfg.PreserveResourceID {
+              		notification = proto.Clone(notification).(*notificationsv1.Notification)
+              		//nolint:staticcheck // SA1019. Deprecated, but still needed.
+              		notification.Metadata.Id = 0
+              		notification.Metadata.Revision = ""
+              	}

Contributor

rosstimothy Feb 22, 2024

We could probably improve maybeResetProtoResourceID to use types.SetRevision and add an equivalent types.SetResource so that it could be used. Not something you need to do here but a good example of where we fell slightly short in making these helpers support legacy and new resources.

lib/services/local/notifications.go Show resolved Hide resolved

lib/services/local/notifications.go Outdated Show resolved Hide resolved

lib/services/local/notifications.go Show resolved Hide resolved

lib/services/local/notifications.go Show resolved Hide resolved

lib/services/local/notifications.go

+              // DeleteGlobalNotification deletes a global notification.
+              func (s *NotificationsService) DeleteGlobalNotification(ctx context.Context, notificationId string) error {
+              	err := s.globalNotificationService.DeleteResource(ctx, notificationId)

Contributor

rosstimothy Feb 22, 2024

Do we need to clean up any associated resources for the notification as well?

Contributor Author

rudream Feb 23, 2024

Hmm, we would want to delete the notification states for this notification, but I don't think there is a performant way to do it here. We would need to list all notification states for all users, filter them for ones that are for this notification, and then loop through them and delete each one. I think this might be better if this was done during a periodic cleanup, what do you think?

rudream requested a review from rosstimothy

February 23, 2024 01:24

rosstimothy reviewed

View reviewed changes

lib/services/local/notifications.go Outdated Show resolved Hide resolved

lib/services/local/generic/generic_wrapper.go Outdated Show resolved Hide resolved

lib/services/local/notifications.go Outdated Show resolved Hide resolved

rudream requested a review from rosstimothy

February 26, 2024 20:19

rosstimothy reviewed

View reviewed changes

api/proto/teleport/notifications/v1/notifications.proto Outdated Show resolved Hide resolved

lib/services/local/notifications.go Show resolved Hide resolved

lib/services/local/notifications.go Outdated Show resolved Hide resolved

rudream force-pushed the yassine/notifications/service branch from 23819f9 to 62e7db7 Compare

February 27, 2024 22:01

ravicious reviewed

View reviewed changes

lib/services/local/notifications.go Outdated Show resolved Hide resolved

lib/services/local/generic/generic_wrapper.go Show resolved Hide resolved

lib/services/local/notifications.go Outdated Show resolved Hide resolved

rudream requested review from ravicious and rosstimothy

February 28, 2024 17:36

rosstimothy reviewed

View reviewed changes

Contributor

rosstimothy left a comment

Overall looks pretty good, just a few small things I hadn't noticed before.

lib/services/notifications.go Show resolved Hide resolved

lib/services/notifications.go Outdated Show resolved Hide resolved

web/packages/teleport/pkg/sumdb/sum.golang.org/latest Outdated Show resolved Hide resolved

lib/services/local/notifications.go Outdated Show resolved Hide resolved

ravicious approved these changes

View reviewed changes

rudream force-pushed the yassine/notifications/service branch from 1718af6 to ca536aa Compare

February 29, 2024 19:09

rudream requested a review from rosstimothy

February 29, 2024 19:17

rosstimothy approved these changes

View reviewed changes

Contributor

rosstimothy left a comment

Nice work @rudream!!

public-teleport-github-review-bot bot removed request for zmb3 and gzdunek

February 29, 2024 23:52

rudream force-pushed the yassine/notifications/service branch from 229f064 to 2595958 Compare

March 1, 2024 16:23

rudream enabled auto-merge

March 1, 2024 16:26


          add notifications backend service

fdf95dc

rudream force-pushed the yassine/notifications/service branch from 2595958 to fdf95dc Compare

March 1, 2024 16:55

rudream added this pull request to the merge queue

Merged via the queue into master with commit e5246f9

37 checks passed

rudream deleted the yassine/notifications/service branch

March 1, 2024 19:05

public-teleport-github-review-bot bot commented Mar 1, 2024

@rudream See the table below for backport results.

Branch	Result
branch/v15	Create PR

rudream mentioned this pull request

[v15] Introduce Notifications CRUD Service #38888

Merged

rosstimothy mentioned this pull request

Add initial access monitoring rule resource #36745

Merged

rosstimothy mentioned this pull request

Add access monitoring rules services and backend #39143

Merged

This was referenced Mar 25, 2024

[v14] Generic service adapter for RFD 153-style resources #39780

Merged

[v13] Generic service adapter for RFD 153-style resources #39818

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/branch/v15 no-changelog size/lg