Skip to content

Commit

Permalink
Refactor handleNotifications + solve bug (#135)
Browse files Browse the repository at this point in the history 
- Refactors handleNotifications function
- Solves a bug that was causing an issue when adding/updating/deleting notifications in a IR resource previously created with no notification within it.
- Updates integration tests
  • Loading branch information
@mavaras
mavaras authored Jul 11, 2024
1 parent 948ce25 commit cec0ccb
Show file tree
Hide file tree
Showing 2 changed files with 130 additions and 138 deletions.
164 changes: 83 additions & 81 deletions controllers/imagerepository_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -590,28 +590,11 @@ var _ = Describe("Image repository controller", func() {
return &quay.Repository{Name: expectedImageName}, nil
}

isCreateNotificationInvoked := false
quay.CreateNotificationFunc = func(organization, repository string, notification quay.Notification) (*quay.Notification, error) {
isCreateNotificationInvoked = true
Expect(organization).To(Equal(quay.TestQuayOrg))
return &quay.Notification{UUID: "uuid"}, nil
}

createImageRepository(imageRepositoryConfig{
Notifications: []imagerepositoryv1alpha1.Notifications{
{
Title: "test-notification",
Event: imagerepositoryv1alpha1.NotificationEventRepoPush,
Method: imagerepositoryv1alpha1.NotificationMethodWebhook,
Config: imagerepositoryv1alpha1.NotificationConfig{
Url: "http://test-url",
},
},
},
Notifications: []imagerepositoryv1alpha1.Notifications{},
})

Eventually(func() bool { return isCreateRepositoryInvoked }, timeout, interval).Should(BeTrue())
Eventually(func() bool { return isCreateNotificationInvoked }, timeout, interval).Should(BeTrue())

waitImageRepositoryFinalizerOnImageRepository(resourceKey)

Expand All @@ -626,27 +609,17 @@ var _ = Describe("Image repository controller", func() {
Expect(imageRepository.Status.Credentials.PushRobotAccountName).To(HavePrefix(expectedRobotAccountPrefix))
Expect(imageRepository.Status.Credentials.PushSecretName).To(Equal(imageRepository.Name + "-image-push"))
Expect(imageRepository.Status.Credentials.GenerationTimestamp).ToNot(BeNil())
Expect(imageRepository.Status.Notifications).To(HaveLen(1))
Expect(imageRepository.Status.Notifications).To(HaveLen(0))
})

It("should add notification", func() {
notifications := []quay.Notification{
{
UUID: "uuid",
Title: "test-notification",
Event: string(imagerepositoryv1alpha1.NotificationEventRepoPush),
Method: string(imagerepositoryv1alpha1.NotificationMethodWebhook),
Config: quay.NotificationConfig{
Url: "http://test-url",
},
},
}
notifications := []quay.Notification{}
isCreateNotificationInvoked := false
quay.CreateNotificationFunc = func(organization, repository string, notification quay.Notification) (*quay.Notification, error) {
notifications = append(
notifications,
quay.Notification{
UUID: "uuid2",
UUID: "uuid",
Title: notification.Title,
Event: notification.Event,
Method: notification.Method,
Expand All @@ -655,7 +628,7 @@ var _ = Describe("Image repository controller", func() {
)
isCreateNotificationInvoked = true
Expect(organization).To(Equal(quay.TestQuayOrg))
return &quay.Notification{UUID: "uuid2", Title: notification.Title}, nil
return &quay.Notification{UUID: "uuid", Title: notification.Title}, nil
}
isGetNotificationsInvoked := false
quay.GetNotificationsFunc = func(organization, repository string) ([]quay.Notification, error) {
Expand All @@ -665,11 +638,11 @@ var _ = Describe("Image repository controller", func() {
}

newNotification := imagerepositoryv1alpha1.Notifications{
Title: "test-notification-2",
Title: "test-notification",
Event: imagerepositoryv1alpha1.NotificationEventRepoPush,
Method: imagerepositoryv1alpha1.NotificationMethodWebhook,
Config: imagerepositoryv1alpha1.NotificationConfig{
Url: "http://test-url-2",
Url: "http://test-url",
},
}
imageRepository := getImageRepository(resourceKey)
Expand All @@ -680,28 +653,64 @@ var _ = Describe("Image repository controller", func() {
Eventually(func() bool { return isGetNotificationsInvoked }, timeout, interval).Should(BeTrue())
Eventually(func() bool {
imageRepository := getImageRepository(resourceKey)
return len(imageRepository.Status.Notifications) == 2
return len(imageRepository.Status.Notifications) == 1
}, timeout, interval).Should(BeTrue())
})

It("should delete notification", func() {
notifications := []quay.Notification{
{
UUID: "uuid",
It("should update notification", func() {
updatedUrl := "http://test-url_new"
isUpdateNotificationInvoked := false
quay.UpdateNotificationFunc = func(organization, repository string, notificationUuid string, notification quay.Notification) (*quay.Notification, error) {
isUpdateNotificationInvoked = true
Expect(organization).To(Equal(quay.TestQuayOrg))
return &quay.Notification{
UUID: "uuid_new",
Title: "test-notification",
Event: string(imagerepositoryv1alpha1.NotificationEventRepoPush),
Method: string(imagerepositoryv1alpha1.NotificationMethodWebhook),
Config: quay.NotificationConfig{
Url: "http://test-url",
Url: updatedUrl,
},
},
}, nil
}
isGetNotificationsInvoked := false
quay.GetNotificationsFunc = func(organization, repository string) ([]quay.Notification, error) {
isGetNotificationsInvoked = true
Expect(organization).To(Equal(quay.TestQuayOrg))
notifications := []quay.Notification{
{
UUID: "uuid",
Title: "test-notification",
Event: string(imagerepositoryv1alpha1.NotificationEventRepoPush),
Method: string(imagerepositoryv1alpha1.NotificationMethodWebhook),
Config: quay.NotificationConfig{
Url: "http://test-url",
},
},
}
return notifications, nil
}
imageRepository := getImageRepository(resourceKey)
imageRepository.Spec.Notifications[0].Config.Url = updatedUrl
Expect(k8sClient.Update(ctx, imageRepository)).To(Succeed())

Eventually(func() bool { return isUpdateNotificationInvoked }, timeout, interval).Should(BeTrue())
Eventually(func() bool { return isGetNotificationsInvoked }, timeout, interval).Should(BeTrue())
Eventually(func() bool {
imageRepository := getImageRepository(resourceKey)
return len(imageRepository.Status.Notifications) == 1 && imageRepository.Spec.Notifications[0].Config.Url == updatedUrl && imageRepository.Status.Notifications[0].UUID == "uuid_new"
}, timeout, interval).Should(BeTrue())
})

It("should delete notification", func() {
notifications := []quay.Notification{
{
UUID: "uuid2",
Title: "test-notification-2",
UUID: "uuid_new",
Title: "test-notification",
Event: string(imagerepositoryv1alpha1.NotificationEventRepoPush),
Method: string(imagerepositoryv1alpha1.NotificationMethodWebhook),
Config: quay.NotificationConfig{
Url: "http://test-url-2",
Url: "http://test-url_new",
},
},
}
Expand All @@ -720,61 +729,54 @@ var _ = Describe("Image repository controller", func() {
}

imageRepository := getImageRepository(resourceKey)
Expect(imageRepository.Status.Notifications).To(HaveLen(2))
Expect(imageRepository.Status.Notifications).To(HaveLen(1))
imageRepository.Spec.Notifications = imageRepository.Spec.Notifications[:len(imageRepository.Spec.Notifications)-1]
Expect(k8sClient.Update(ctx, imageRepository)).To(Succeed())

Eventually(func() bool { return isDeleteNotificationInvoked }, timeout, interval).Should(BeTrue())
Eventually(func() bool { return isGetNotificationsInvoked }, timeout, interval).Should(BeTrue())
Eventually(func() bool {
imageRepository := getImageRepository(resourceKey)
return len(imageRepository.Status.Notifications) == 1
return len(imageRepository.Status.Notifications) == 0
}, timeout, interval).Should(BeTrue())
})

It("should update notification", func() {
updatedUrl := "http://test-url_new"
isUpdateNotificationInvoked := false
quay.UpdateNotificationFunc = func(organization, repository string, notificationUuid string, notification quay.Notification) (*quay.Notification, error) {
isUpdateNotificationInvoked = true
It("should provision image repository with some notifications to create", func() {
deleteImageRepository(resourceKey)
isCreateNotificationInvoked := false
quay.CreateNotificationFunc = func(organization, repository string, notification quay.Notification) (*quay.Notification, error) {
isCreateNotificationInvoked = true
Expect(organization).To(Equal(quay.TestQuayOrg))
return &quay.Notification{
UUID: "uuid_new",
Title: "test-notification",
Event: string(imagerepositoryv1alpha1.NotificationEventRepoPush),
Method: string(imagerepositoryv1alpha1.NotificationMethodWebhook),
Config: quay.NotificationConfig{
Url: updatedUrl,
},
}, nil
return &quay.Notification{UUID: "uuid"}, nil
}
isGetNotificationsInvoked := false
quay.GetNotificationsFunc = func(organization, repository string) ([]quay.Notification, error) {
isGetNotificationsInvoked = true
Expect(organization).To(Equal(quay.TestQuayOrg))
notifications := []quay.Notification{
createImageRepository(imageRepositoryConfig{
Notifications: []imagerepositoryv1alpha1.Notifications{
{
UUID: "uuid",
Title: "test-notification",
Event: string(imagerepositoryv1alpha1.NotificationEventRepoPush),
Method: string(imagerepositoryv1alpha1.NotificationMethodWebhook),
Config: quay.NotificationConfig{
Event: imagerepositoryv1alpha1.NotificationEventRepoPush,
Method: imagerepositoryv1alpha1.NotificationMethodWebhook,
Config: imagerepositoryv1alpha1.NotificationConfig{
Url: "http://test-url",
},
},
}
return notifications, nil
}
imageRepository := getImageRepository(resourceKey)
imageRepository.Spec.Notifications[0].Config.Url = updatedUrl
Expect(k8sClient.Update(ctx, imageRepository)).To(Succeed())
{
Title: "test-notification-2",
Event: imagerepositoryv1alpha1.NotificationEventRepoPush,
Method: imagerepositoryv1alpha1.NotificationMethodWebhook,
Config: imagerepositoryv1alpha1.NotificationConfig{
Url: "http://test-url-2",
},
},
},
})
Eventually(func() bool { return isCreateNotificationInvoked }, timeout, interval).Should(BeTrue())

Eventually(func() bool { return isUpdateNotificationInvoked }, timeout, interval).Should(BeTrue())
Eventually(func() bool { return isGetNotificationsInvoked }, timeout, interval).Should(BeTrue())
Eventually(func() bool {
imageRepository := getImageRepository(resourceKey)
return len(imageRepository.Status.Notifications) == 1 && imageRepository.Spec.Notifications[0].Config.Url == updatedUrl && imageRepository.Status.Notifications[0].UUID == "uuid_new"
}, timeout, interval).Should(BeTrue())
waitImageRepositoryFinalizerOnImageRepository(resourceKey)

imageRepository := getImageRepository(resourceKey)
Expect(imageRepository.Status.Notifications).To(HaveLen(2))
Expect(imageRepository.Status.Notifications[0].Title).To(Equal("test-notification"))
Expect(imageRepository.Status.Notifications[1].Title).To(Equal("test-notification-2"))
})
})

Expand Down
104 changes: 47 additions & 57 deletions controllers/imagerepository_notifications.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,14 @@ func (r *ImageRepositoryReconciler) HandleNotifications(ctx context.Context, ima
if !r.checkNotificationChangesExists(imageRepository, allNotifications) {
return nil
}
for index := 0; index < len(imageRepository.Status.Notifications); index++ {
statusNotification := &imageRepository.Status.Notifications[index] // This way we can update the UUID if notification gets updated
existsInSpec := false
for _, notification := range imageRepository.Spec.Notifications {

log.Info("Starting to handle notifications")
for _, notification := range imageRepository.Spec.Notifications {
existsInStatus := false
for index := 0; index < len(imageRepository.Status.Notifications); index++ {
statusNotification := &imageRepository.Status.Notifications[index] // This way we can update the UUID if notification gets updated
if notification.Title == statusNotification.Title {
existsInSpec = true
existsInStatus = true
quayNotification, err := r.notificationExistsInQuayByUUID(statusNotification.UUID, imageRepository)
if err != nil {
return r.handleError(ctx, imageRepository, err, "Couldn't retrieve all Quay notifications")
Expand Down Expand Up @@ -101,46 +103,50 @@ func (r *ImageRepositoryReconciler) HandleNotifications(ctx context.Context, ima
statusNotification.Title = updatedNotification.Title
break
}
} else {
exists, err := r.notificationExistsInQuayByTitle(notification.Title, imageRepository)
if err != nil {
return r.handleError(ctx, imageRepository, err, "Couldn't retrieve all Quay notifications")
}
if !exists {
// New item in Spec.Notifications: add notification to Quay and Status.Notifications
log.Info("Adding new notification to Quay", "Title", notification.Title, "Event", notification.Event, "Method", notification.Method)
resStatusNotification, err := r.AddNotification(notification, imageRepository)
if err != nil {
log.Error(err, "failed to add notification", "Title", notification.Title, "Event", notification.Event, "Method", notification.Method)
return r.handleError(ctx, imageRepository, err, "Error while adding a notification ("+notification.Title+") to Quay")
}
existsInStatus := false
for _, statusNotificationAux := range imageRepository.Status.Notifications {
if resStatusNotification.UUID == statusNotificationAux.UUID {
existsInStatus = true
break
}
}
if !existsInStatus {
imageRepository.Status.Notifications = append(imageRepository.Status.Notifications, resStatusNotification)
}
}
}
}
if !existsInSpec {
// Deleted item from Spec.Notifications: delete notification from Quay and Status.Notifications
log.Info("Deleting notification in Quay", "Title", statusNotification.Title, "UUID", statusNotification.UUID)
_, err := r.QuayClient.DeleteNotification(
r.QuayOrganization,
imageRepository.Spec.Image.Name,
statusNotification.UUID)
if !existsInStatus {
log.Info("Adding new notification to Quay", "Title", notification.Title, "Event", notification.Event, "Method", notification.Method)
resStatusNotification, err := r.AddNotification(notification, imageRepository)
if err != nil {
log.Error(err, "failed to delete notification", "Title", statusNotification.Title, "UUID", statusNotification.UUID)
return r.handleError(ctx, imageRepository, err, "Error while deleting a notification ("+statusNotification.Title+") to Quay")
log.Error(err, "failed to add notification", "Title", notification.Title, "Event", notification.Event, "Method", notification.Method)
return r.handleError(ctx, imageRepository, err, "Error while adding a notification ("+notification.Title+") to Quay")
}
alreadyInStatus := false
for _, statusNotificationAux := range imageRepository.Status.Notifications {
if resStatusNotification.UUID == statusNotificationAux.UUID {
alreadyInStatus = true
break
}
}
if !alreadyInStatus {
imageRepository.Status.Notifications = append(imageRepository.Status.Notifications, resStatusNotification)
}
}
}
if len(imageRepository.Status.Notifications) > len(imageRepository.Spec.Notifications) {
// There are notifications to be deleted
for index, statusNotification := range imageRepository.Status.Notifications {
existsInSpec := false
for _, notification := range imageRepository.Spec.Notifications {
if notification.Title == statusNotification.Title {
existsInSpec = true
break
}
}
if !existsInSpec {
log.Info("Deleting notification in Quay", "Title", statusNotification.Title, "UUID", statusNotification.UUID)
_, err := r.QuayClient.DeleteNotification(
r.QuayOrganization,
imageRepository.Spec.Image.Name,
statusNotification.UUID)
if err != nil {
log.Error(err, "failed to delete notification", "Title", statusNotification.Title, "UUID", statusNotification.UUID)
return r.handleError(ctx, imageRepository, err, "Error while deleting a notification ("+statusNotification.Title+") to Quay")
}
// Remove notification from CR status
imageRepository.Status.Notifications = append(imageRepository.Status.Notifications[:index], imageRepository.Status.Notifications[index+1:]...)
}
// Remove notification from CR status
imageRepository.Status.Notifications = append(imageRepository.Status.Notifications[:index], imageRepository.Status.Notifications[index+1:]...)
index--
}
}

Expand Down Expand Up @@ -193,22 +199,6 @@ func (r *ImageRepositoryReconciler) notificationExistsInQuayByUUID(UUID string,
return notification, nil
}

func (r *ImageRepositoryReconciler) notificationExistsInQuayByTitle(title string, imageRepository *imagerepositoryv1alpha1.ImageRepository) (bool, error) {
exists := false
allNotifications, err := r.QuayClient.GetNotifications(r.QuayOrganization, imageRepository.Spec.Image.Name)
if err != nil {
return exists, err
}
for _, quayNotification := range allNotifications {
if quayNotification.Title == title {
exists = true
break
}
}

return exists, nil
}

func isNotificationChanged(notification imagerepositoryv1alpha1.Notifications, quayNotification quay.Notification) bool {
return quayNotification.UUID != "" && (quayNotification.Title != notification.Title || quayNotification.Event != string(notification.Event) || quayNotification.Method != string(notification.Method) || quayNotification.Config.Url != notification.Config.Url)
}
Expand Down

0 comments on commit cec0ccb

Please sign in to comment.