Merge pull request #51 from stakater/reconcile-bug-fix

Fixed repetitive reconciliation issue on users

.gitignore

@@ -23,4 +23,5 @@ bin
 *.swo
 *~
 .local
-testbin
+testbin
+.vscode

controllers/channel_controller.go

@@ -29,6 +29,7 @@ import (
 	reconcilerUtil "github.com/stakater/operator-utils/util/reconciler"
 	slackv1alpha1 "github.com/stakater/slack-operator/api/v1alpha1"
 	slack "github.com/stakater/slack-operator/pkg/slack"
+	pkgutil "github.com/stakater/slack-operator/pkg/util"
 )
 
 var (
@@ -184,10 +185,10 @@ func (r *ChannelReconciler) updateSlackChannel(ctx context.Context, channel *sla
 		return reconcilerUtil.ManageError(r.Client, channel, err, false)
 	}
 
-	err = r.SlackService.InviteUsers(channelID, users)
-	if err != nil {
+	errorlist := r.SlackService.InviteUsers(channelID, users)
+	if len(errorlist) > 0 {
 		log.Error(err, "Error inviting users to channel")
-		return reconcilerUtil.ManageError(r.Client, channel, err, false)
+		return pkgutil.ManageError(ctx, r.Client, channel, pkgutil.MapErrorListToError(errorlist))
 	}
 
 	err = r.SlackService.RemoveUsers(channelID, users)

controllers/channel_controller_test.go

@@ -2,6 +2,7 @@ package controllers
 
 import (
 	"context"
+	"fmt"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -84,13 +85,13 @@ var _ = Describe("ChannelController", func() {
 			})
 
 			It("should set error condition when user does not exists", func() {
-
-				_ = util.CreateChannel(channelName, true, "", "", []string{"[email protected]"}, ns)
+				emailList := []string{"[email protected]"}
+				_ = util.CreateChannel(channelName, true, "", "", emailList, ns)
 				channel := util.GetChannel(channelName, ns)
 
 				Expect(len(channel.Status.Conditions)).To(Equal(1))
 				Expect(channel.Status.Conditions[0].Reason).To(Equal("Failed"))
-				Expect(channel.Status.Conditions[0].Message).To(Equal("users_not_found"))
+				Expect(channel.Status.Conditions[0].Message).To(Equal(fmt.Sprintf("Error fetching user by Email %s", emailList[0])))
 			})
 		})
 	})

pkg/config/config.go

@@ -3,6 +3,7 @@ package config
 import (
 	"io/ioutil"
 	"os"
+	"time"
 
 	util "github.com/stakater/operator-utils/util"
 	secretsUtil "github.com/stakater/operator-utils/util/secrets"
@@ -13,6 +14,8 @@ import (
 )
 
 const (
+	ErrorRequeueTime = 15 * time.Minute
+
 	SlackDefaultSecretName string = "slack-secret"
 	SlackAPITokenSecretKey string = "APIToken"
 )

pkg/slack/service.go

@@ -21,7 +21,7 @@ type Service interface {
 	SetTopic(string, string) (*slack.Channel, error)
 	RenameChannel(string, string) (*slack.Channel, error)
 	ArchiveChannel(string) error
-	InviteUsers(string, []string) error
+	InviteUsers(string, []string) []error
 	RemoveUsers(string, []string) error
 	GetChannel(string) (*slack.Channel, error)
 	GetUsersInChannel(channelID string) ([]string, error)
@@ -176,27 +176,29 @@ func (s *SlackService) GetUsersInChannel(channelID string) ([]string, error) {
 }
 
 // InviteUsers invites users to the slack channel
-func (s *SlackService) InviteUsers(channelID string, userEmails []string) error {
+func (s *SlackService) InviteUsers(channelID string, userEmails []string) []error {
 	log := s.log.WithValues("channelID", channelID)
 
+	var errorlist []error
+
 	for _, email := range userEmails {
 		user, err := s.api.GetUserByEmail(email)
 
 		if err != nil {
-			log.Error(err, "Error getting user by email")
-			return err
+			errorlist = append(errorlist, fmt.Errorf(fmt.Sprintf("Error fetching user by Email %s", email)))
+			continue
 		}
 
 		log.V(1).Info("Inviting user to Slack Channel", "userID", user.ID)
 		_, err = s.api.InviteUsersToConversation(channelID, user.ID)
 
 		if err != nil && err.Error() != "already_in_channel" {
 			log.Error(err, "Error Inviting user to channel", "userID", user.ID)
-			return err
+			errorlist = append(errorlist, err)
 		}
 	}
 
-	return nil
+	return errorlist
 }
 
 // RemoveUsers remove users from the slack channel
@@ -285,8 +287,8 @@ func (s *SlackService) IsChannelUpdated(channel *slackv1alpha1.Channel) (bool, e
 	for _, email := range userEmails {
 		user, err := s.api.GetUserByEmail(email)
 		if err != nil {
-			log.Error(err, "Error fetching user by Email")
-			return false, err
+			log.Error(err, fmt.Sprintf("Error fetching user by Email %s", email))
+			continue
 		}
 
 		found := false

pkg/slack/service_test.go

@@ -1,6 +1,7 @@
 package slack
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stakater/slack-operator/pkg/slack/mock"
@@ -78,12 +79,14 @@ func TestSlackService_ArchiveChannel_shouldThrowError_whenChannelNotFound(t *tes
 
 func TestSlackService_InviteUsers_shouldSendUserInvites_whenUserExists(t *testing.T) {
 	s := NewMockService(log)
-	err := s.InviteUsers(mock.PublicConversationID, []string{mock.ExistingUserEmail})
-	assert.NoError(t, err)
+	errs := s.InviteUsers(mock.PublicConversationID, []string{mock.ExistingUserEmail})
+	assert.Equal(t, 0, len(errs))
 }
 
 func TestSlackService_InviteUsers_shouldThowError_whenUserDoesNotExists(t *testing.T) {
 	s := NewMockService(log)
-	err := s.InviteUsers(mock.PublicConversationID, []string{"[email protected]"})
-	assert.EqualError(t, err, "users_not_found")
+	emailList := []string{"[email protected]"}
+	errs := s.InviteUsers(mock.PublicConversationID, emailList)
+	assert.Equal(t, 1, len(errs))
+	assert.EqualError(t, errs[0], fmt.Sprintf("Error fetching user by Email %s", emailList[0]))
 }

pkg/util/util.go

@@ -0,0 +1,58 @@
+package pkgutil
+
+import (
+	"context"
+	"fmt"
+	"strings"
+	"time"
+
+	"github.com/stakater/slack-operator/pkg/config"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	ctrl "sigs.k8s.io/controller-runtime"
+
+	k8sClient "sigs.k8s.io/controller-runtime/pkg/client"
+
+	reconcilerUtil "github.com/stakater/operator-utils/util/reconciler"
+	slackv1alpha1 "github.com/stakater/slack-operator/api/v1alpha1"
+)
+
+// MapErrorListToError maps multiple errors into a single error
+func MapErrorListToError(errs []error) error {
+
+	if len(errs) == 0 {
+		return nil
+	}
+
+	errMsg := []string{}
+	for _, err := range errs {
+		errMsg = append(errMsg, err.Error())
+	}
+
+	return fmt.Errorf(strings.Join(errMsg, "\n"))
+}
+
+func ManageError(ctx context.Context, client k8sClient.Client, channelInstance *slackv1alpha1.Channel, issue error) (ctrl.Result, error) {
+
+	// Base object for patch, which patches using the merge-patch strategy with the given object as base.
+	channelInstancePatchBase := k8sClient.MergeFrom(channelInstance.DeepCopy())
+
+	// Update status
+	channelInstance.Status.Conditions = []metav1.Condition{
+		{
+			Type:               "ReconcileError",
+			LastTransitionTime: metav1.Date(time.Now().Year(), time.Now().Month(), time.Now().Day(), time.Now().Hour(), time.Now().Minute(), 0, 0, time.Now().Location()),
+			Message:            issue.Error(),
+			Reason:             reconcilerUtil.FailedReason,
+			Status:             metav1.ConditionTrue,
+		},
+	}
+
+	// Patch status
+	err := client.Status().Patch(ctx, channelInstance, channelInstancePatchBase)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	return reconcilerUtil.RequeueAfter(config.ErrorRequeueTime)
+}