Merge pull request #1156 from mumoshu/fix-describe-locks-bug

Fix describe locks bug
kufu-ai · Nov 11, 2024 · 7e72b34 · 7e72b34
2 parents 0688c62 + 5c29e37
commit 7e72b34
Show file tree

Hide file tree

Showing 4 changed files with 202 additions and 80 deletions.
diff --git a/deploy/configmap.go b/deploy/configmap.go
@@ -63,13 +63,13 @@ const (
 
 // configMapKey is a helper function that returns the key within the ConfigMap for the project and the environment
 // which is either locked or unlocked.
-func (c *Coordinator) configMapKey(project, environment string) string {
+func configMapKey(project, environment string) string {
 	return project + Sep + environment
 }
 
 func splitConfigMapKey(key string) (string, string) {
-	parts := strings.Split(key, Sep)
-	return parts[0], parts[1]
+	i := strings.LastIndex(key, Sep)
+	return key[:i], key[i+1:]
 }
 
 func (c *Coordinator) getOrCreateConfigMap(ctx context.Context) (*corev1.ConfigMap, error) {

diff --git a/deploy/lock.go b/deploy/lock.go
@@ -88,31 +88,8 @@ func (c *Coordinator) lock(ctx context.Context, project, environment, user, reas
 		return fmt.Errorf("unable to get or create configmap: %w", err)
 	}
 
-	key := c.configMapKey(project, environment)
-	value, err := strToConfigMapValue(configMap.Data[key])
-	if err != nil {
-		return fmt.Errorf("unable to unmarshal str into value: %w", err)
-	}
-
-	if value.Locked {
-		return ErrAlreadyLocked
-	}
-
-	if n := len(value.LockHistory); n >= MaxHistoryItems {
-		value.LockHistory = value.LockHistory[n-MaxHistoryItems+1:]
-	}
-
-	value.LockHistory = append(value.LockHistory, LockHistoryItem{
-		User:   user,
-		Action: LockActionLock,
-		At:     c.Now(),
-		Reason: reason,
-	})
-
-	value.Locked = true
-
-	configMap.Data[key], err = configMapValueToStr(value)
-	if err != nil {
+	enc := &keysAndValuesEncoding{data: configMap.Data}
+	if err := enc.lock(project, environment, user, reason, c.Now()); err != nil {
 		return err
 	}
 
@@ -156,37 +133,8 @@ func (c *Coordinator) unlock(ctx context.Context, project, environment, user str
 		return err
 	}
 
-	key := c.configMapKey(project, environment)
-	value, err := strToConfigMapValue(configMap.Data[key])
-	if err != nil {
-		return err
-	}
-
-	if !value.Locked {
-		return ErrAlreadyUnlocked
-	}
-
-	if force {
-		value.Locked = false
-	} else {
-		if len(value.LockHistory) == 0 || value.LockHistory[len(value.LockHistory)-1].User != user {
-			return newNotAllowedToUnlockError(user)
-		}
-
-		if n := len(value.LockHistory); n >= MaxHistoryItems {
-			value.LockHistory = value.LockHistory[n-MaxHistoryItems+1:]
-		}
-
-		value.Locked = false
-		value.LockHistory = append(value.LockHistory, LockHistoryItem{
-			User:   user,
-			Action: LockActionUnlock,
-			At:     c.Now(),
-		})
-	}
-
-	configMap.Data[key], err = configMapValueToStr(value)
-	if err != nil {
+	enc := &keysAndValuesEncoding{data: configMap.Data}
+	if err := enc.unlock(project, environment, user, force, c.Now()); err != nil {
 		return err
 	}
 
@@ -264,28 +212,11 @@ func (c *Coordinator) FetchLocks(ctx context.Context, projectFilter, phaseFilter
 		return nil, err
 	}
 
-	locks := make(map[string]map[string]Phase)
-	for k, v := range configMap.Data {
-		value, err := strToConfigMapValue(v)
-		if err != nil {
-			return nil, err
-		}
-
-		project, environment := splitConfigMapKey(k)
-
-		if projectFilter != "" && project != projectFilter {
-			continue
-		}
-
-		if phaseFilter != "" && environment != phaseFilter {
-			continue
-		}
+	enc := &keysAndValuesEncoding{data: configMap.Data}
 
-		if locks[project] == nil {
-			locks[project] = make(map[string]Phase)
-		}
-
-		locks[project][environment] = value
+	locks, err := enc.describeLocks(projectFilter, phaseFilter)
+	if err != nil {
+		return nil, err
 	}
 
 	return locks, nil

diff --git a/deploy/lock_encoding.go b/deploy/lock_encoding.go
@@ -0,0 +1,109 @@
+package deploy
+
+import (
+	"fmt"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+type keysAndValuesEncoding struct {
+	data map[string]string
+}
+
+func (e *keysAndValuesEncoding) lock(project, environment, user, reason string, at metav1.Time) error {
+	key := configMapKey(project, environment)
+	value, err := strToConfigMapValue(e.data[key])
+	if err != nil {
+		return fmt.Errorf("unable to unmarshal str into value: %w", err)
+	}
+
+	if value.Locked {
+		return ErrAlreadyLocked
+	}
+
+	if n := len(value.LockHistory); n >= MaxHistoryItems {
+		value.LockHistory = value.LockHistory[n-MaxHistoryItems+1:]
+	}
+
+	value.LockHistory = append(value.LockHistory, LockHistoryItem{
+		User:   user,
+		Action: LockActionLock,
+		At:     at,
+		Reason: reason,
+	})
+
+	value.Locked = true
+
+	e.data[key], err = configMapValueToStr(value)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (e *keysAndValuesEncoding) unlock(project, environment, user string, force bool, at metav1.Time) error {
+	key := configMapKey(project, environment)
+	value, err := strToConfigMapValue(e.data[key])
+	if err != nil {
+		return err
+	}
+
+	if !value.Locked {
+		return ErrAlreadyUnlocked
+	}
+
+	if force {
+		value.Locked = false
+	} else {
+		if len(value.LockHistory) == 0 || value.LockHistory[len(value.LockHistory)-1].User != user {
+			return newNotAllowedToUnlockError(user)
+		}
+
+		if n := len(value.LockHistory); n >= MaxHistoryItems {
+			value.LockHistory = value.LockHistory[n-MaxHistoryItems+1:]
+		}
+
+		value.Locked = false
+		value.LockHistory = append(value.LockHistory, LockHistoryItem{
+			User:   user,
+			Action: LockActionUnlock,
+			At:     at,
+		})
+	}
+
+	e.data[key], err = configMapValueToStr(value)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (e *keysAndValuesEncoding) describeLocks(projectFilter, phaseFilter string) (map[string]map[string]Phase, error) {
+	locks := make(map[string]map[string]Phase)
+	for k, v := range e.data {
+		value, err := strToConfigMapValue(v)
+		if err != nil {
+			return nil, err
+		}
+
+		project, environment := splitConfigMapKey(k)
+
+		if projectFilter != "" && project != projectFilter {
+			continue
+		}
+
+		if phaseFilter != "" && environment != phaseFilter {
+			continue
+		}
+
+		if locks[project] == nil {
+			locks[project] = make(map[string]Phase)
+		}
+
+		locks[project][environment] = value
+	}
+
+	return locks, nil
+}
diff --git a/deploy/lock_encoding_test.go b/deploy/lock_encoding_test.go
@@ -0,0 +1,82 @@
+package deploy
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestKeysAndValuesEncoding(t *testing.T) {
+	data := map[string]string{}
+	enc := &keysAndValuesEncoding{
+		data: data,
+	}
+
+	now, err := time.Parse(time.RFC3339, "2021-09-01T00:00:00Z")
+	require.NoError(t, err)
+
+	kNow := metav1.NewTime(now.Local())
+
+	require.NoError(t, enc.lock("myproject1", "prod", "user1", "for deployment of revision 1a", kNow))
+	require.ErrorIs(t, enc.lock("myproject1", "prod", "user1", "for deployment of revision 1b", kNow), ErrAlreadyLocked)
+	require.ErrorIs(t, enc.lock("myproject1", "prod", "user2", "for deployment of revision 1b", kNow), ErrAlreadyLocked)
+
+	require.ErrorIs(t, enc.unlock("myproject2", "prod", "user1", false, kNow), ErrAlreadyUnlocked)
+	require.NoError(t, enc.lock("myproject2", "prod", "user1", "for deployment of revision 2a", kNow))
+
+	require.NoError(t, enc.lock("myproject2-api", "prod", "user1", "for deployment of revision 3a", kNow))
+
+	locks, err := enc.describeLocks("", "")
+	require.NoError(t, err)
+
+	assert.Equal(t, map[string]map[string]Phase{
+		"myproject1": {
+			"prod": {
+				Locked: true,
+				LockHistory: []LockHistoryItem{
+					{
+						User:   "user1",
+						Action: LockActionLock,
+						At:     kNow,
+						Reason: "for deployment of revision 1a",
+					},
+				},
+			},
+		},
+		"myproject2": {
+			"prod": {
+				Locked: true,
+				LockHistory: []LockHistoryItem{
+					{
+						User:   "user1",
+						Action: LockActionLock,
+						At:     kNow,
+						Reason: "for deployment of revision 2a",
+					},
+				},
+			},
+		},
+		"myproject2-api": {
+			"prod": {
+				Locked: true,
+				LockHistory: []LockHistoryItem{
+					{
+						User:   "user1",
+						Action: LockActionLock,
+						At:     kNow,
+						Reason: "for deployment of revision 3a",
+					},
+				},
+			},
+		},
+	}, locks)
+
+	assert.Equal(t, map[string]string{
+		"myproject1-prod":     `{"locked":true,"lockHistory":[{"user":"user1","action":"lock","at":"2021-09-01T00:00:00Z","reason":"for deployment of revision 1a"}]}`,
+		"myproject2-prod":     `{"locked":true,"lockHistory":[{"user":"user1","action":"lock","at":"2021-09-01T00:00:00Z","reason":"for deployment of revision 2a"}]}`,
+		"myproject2-api-prod": `{"locked":true,"lockHistory":[{"user":"user1","action":"lock","at":"2021-09-01T00:00:00Z","reason":"for deployment of revision 3a"}]}`,
+	}, data)
+}