Skip to content

Commit

Permalink
fixes+tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@johnnyaug
johnnyaug committed Sep 26, 2023
1 parent fb2cbd1 commit eff3038
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 53 deletions.
2 changes: 1 addition & 1 deletion pkg/api/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1789,7 +1789,7 @@ func (c *Controller) SetBranchProtectionRules(w http.ResponseWriter, r *http.Req
// https://github.com/deepmap/oapi-codegen/issues/954
eTag = swag.String(base64.StdEncoding.EncodeToString([]byte("EMPTY")))
}
err := c.Catalog.SetBranchProtectionRules(ctx, repository, rules, params.IfMatch)
err := c.Catalog.SetBranchProtectionRules(ctx, repository, rules, eTag)
if c.handleAPIError(ctx, w, r, err) {
return
}
Expand Down
19 changes: 0 additions & 19 deletions pkg/graveler/branch/protection_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"time"

"github.com/gobwas/glob"
"github.com/gobwas/glob/syntax"
"github.com/treeverse/lakefs/pkg/cache"
"github.com/treeverse/lakefs/pkg/graveler"
"github.com/treeverse/lakefs/pkg/graveler/settings"
Expand Down Expand Up @@ -36,24 +35,6 @@ func NewProtectionManager(settingManager *settings.Manager) *ProtectionManager {
return &ProtectionManager{settingManager: settingManager, matchers: cache.NewCache(matcherCacheSize, matcherCacheExpiry, cache.NewJitterFn(matcherCacheJitter))}
}

func (m *ProtectionManager) Add(ctx context.Context, repository *graveler.RepositoryRecord, branchNamePattern string, blockedActions []graveler.BranchProtectionBlockedAction) error {
_, err := syntax.Parse(branchNamePattern)
if err != nil {
return fmt.Errorf("invalid branch pattern syntax: %w", err)
}
return m.settingManager.Update(ctx, repository, ProtectionSettingKey, &graveler.BranchProtectionRules{}, func(message proto.Message) (proto.Message, error) {
rules := message.(*graveler.BranchProtectionRules)
if rules.BranchPatternToBlockedActions == nil {
rules.BranchPatternToBlockedActions = make(map[string]*graveler.BranchProtectionBlockedActions)
}
if _, ok := rules.BranchPatternToBlockedActions[branchNamePattern]; ok {
return nil, ErrRuleAlreadyExists
}
rules.BranchPatternToBlockedActions[branchNamePattern] = &graveler.BranchProtectionBlockedActions{Value: blockedActions}
return rules, nil
})
}

func (m *ProtectionManager) Delete(ctx context.Context, repository *graveler.RepositoryRecord, branchNamePattern string) error {
return m.settingManager.Update(ctx, repository, ProtectionSettingKey, &graveler.BranchProtectionRules{}, func(message proto.Message) (proto.Message, error) {
rules := message.(*graveler.BranchProtectionRules)
Expand Down
70 changes: 37 additions & 33 deletions pkg/graveler/branch/protection_manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,14 @@ func TestGet(t *testing.T) {
if rule != nil {
t.Fatalf("expected nil rule, got %v", rule)
}
testutil.Must(t, bpm.Add(ctx, repository, "main*", []graveler.BranchProtectionBlockedAction{graveler.BranchProtectionBlockedAction_STAGING_WRITE}))
testutil.Must(t, bpm.SetRules(ctx, repository, &graveler.BranchProtectionRules{
BranchPatternToBlockedActions: map[string]*graveler.BranchProtectionBlockedActions{
"main*": {Value: []graveler.BranchProtectionBlockedAction{
graveler.BranchProtectionBlockedAction_STAGING_WRITE},
},
},
}, nil))

rule, err = bpm.Get(ctx, repository, "main*")
testutil.Must(t, err)
if diff := deep.Equal([]graveler.BranchProtectionBlockedAction{graveler.BranchProtectionBlockedAction_STAGING_WRITE}, rule); diff != nil {
Expand All @@ -44,24 +51,20 @@ func TestGet(t *testing.T) {
}
}

func TestAddAlreadyExists(t *testing.T) {
ctx := context.Background()
bpm := prepareTest(t, ctx)
testutil.Must(t, bpm.Add(ctx, repository, "main*", []graveler.BranchProtectionBlockedAction{graveler.BranchProtectionBlockedAction_STAGING_WRITE}))
err := bpm.Add(ctx, repository, "main*", []graveler.BranchProtectionBlockedAction{graveler.BranchProtectionBlockedAction_COMMIT})
if !errors.Is(err, branch.ErrRuleAlreadyExists) {
t.Fatalf("expected ErrRuleAlreadyExists, got %v", err)
}
}

func TestDelete(t *testing.T) {
ctx := context.Background()
bpm := prepareTest(t, ctx)
err := bpm.Delete(ctx, repository, "main*")
if !errors.Is(err, branch.ErrRuleNotExists) {
t.Fatalf("expected ErrRuleNotExists, got %v", err)
}
testutil.Must(t, bpm.Add(ctx, repository, "main*", []graveler.BranchProtectionBlockedAction{graveler.BranchProtectionBlockedAction_STAGING_WRITE}))
testutil.Must(t, bpm.SetRules(ctx, repository, &graveler.BranchProtectionRules{
BranchPatternToBlockedActions: map[string]*graveler.BranchProtectionBlockedActions{
"main*": {Value: []graveler.BranchProtectionBlockedAction{
graveler.BranchProtectionBlockedAction_STAGING_WRITE},
},
},
}, nil))
rule, err := bpm.Get(ctx, repository, "main*")
testutil.Must(t, err)
if diff := deep.Equal([]graveler.BranchProtectionBlockedAction{graveler.BranchProtectionBlockedAction_STAGING_WRITE}, rule); diff != nil {
Expand All @@ -78,44 +81,45 @@ func TestDelete(t *testing.T) {

func TestIsBlocked(t *testing.T) {
ctx := context.Background()
const (
var (
action1 = graveler.BranchProtectionBlockedAction_STAGING_WRITE
action2 = graveler.BranchProtectionBlockedAction_COMMIT
action3 = 2
action4 = 3
action3 = graveler.BranchProtectionBlockedAction(2)
action4 = graveler.BranchProtectionBlockedAction(3)
)
tests := map[string]struct {
patternToBlockedActions map[string][]graveler.BranchProtectionBlockedAction
expectedBlockedActions map[string][]graveler.BranchProtectionBlockedAction
expectedAllowedActions map[string][]graveler.BranchProtectionBlockedAction
patternToBlockedActions map[string]*graveler.BranchProtectionBlockedActions
expectedBlockedActions map[string]*graveler.BranchProtectionBlockedActions
expectedAllowedActions map[string]*graveler.BranchProtectionBlockedActions
}{
"two_rules": {
patternToBlockedActions: map[string][]graveler.BranchProtectionBlockedAction{"main*": {action1}, "dev": {action2}},
expectedBlockedActions: map[string][]graveler.BranchProtectionBlockedAction{"main": {action1}, "main2": {action1}, "dev": {action2}},
expectedAllowedActions: map[string][]graveler.BranchProtectionBlockedAction{"main": {action2}, "main2": {action2}, "dev": {action1}, "dev1": {action1, action2}},
patternToBlockedActions: map[string]*graveler.BranchProtectionBlockedActions{"main*": {Value: []graveler.BranchProtectionBlockedAction{action1}}, "dev": {Value: []graveler.BranchProtectionBlockedAction{action2}}},
expectedBlockedActions: map[string]*graveler.BranchProtectionBlockedActions{"main": {Value: []graveler.BranchProtectionBlockedAction{action1}}, "main2": {Value: []graveler.BranchProtectionBlockedAction{action1}}, "dev": {Value: []graveler.BranchProtectionBlockedAction{action2}}},
expectedAllowedActions: map[string]*graveler.BranchProtectionBlockedActions{"main": {Value: []graveler.BranchProtectionBlockedAction{action2}}, "main2": {Value: []graveler.BranchProtectionBlockedAction{action2}}, "dev": {Value: []graveler.BranchProtectionBlockedAction{action1}}, "dev1": {Value: []graveler.BranchProtectionBlockedAction{action1, action2}}},
},
"multiple_blocked": {
patternToBlockedActions: map[string][]graveler.BranchProtectionBlockedAction{"main*": {action1, action2, action3}, "stable_*": {action3, action4}},
expectedBlockedActions: map[string][]graveler.BranchProtectionBlockedAction{"main": {action1, action2, action3}, "main2": {action1, action2, action3}, "stable_branch": {action3, action4}},
expectedAllowedActions: map[string][]graveler.BranchProtectionBlockedAction{"main": {action4}, "main2": {action4}, "stable_branch": {action1, action2}},
patternToBlockedActions: map[string]*graveler.BranchProtectionBlockedActions{"main*": {Value: []graveler.BranchProtectionBlockedAction{action1, action2, action3}}, "stable_*": {Value: []graveler.BranchProtectionBlockedAction{action3, action4}}},
expectedBlockedActions: map[string]*graveler.BranchProtectionBlockedActions{"main": {Value: []graveler.BranchProtectionBlockedAction{action1, action2, action3}}, "main2": {Value: []graveler.BranchProtectionBlockedAction{action1, action2, action3}}, "stable_branch": {Value: []graveler.BranchProtectionBlockedAction{action3, action4}}},
expectedAllowedActions: map[string]*graveler.BranchProtectionBlockedActions{"main": {Value: []graveler.BranchProtectionBlockedAction{action4}}, "main2": {Value: []graveler.BranchProtectionBlockedAction{action4}}, "stable_branch": {Value: []graveler.BranchProtectionBlockedAction{action1, action2}}},
},
"overlapping_patterns": {
patternToBlockedActions: map[string][]graveler.BranchProtectionBlockedAction{"main*": {action1}, "mai*": {action2}, "ma*": {action2, action3}},
expectedBlockedActions: map[string][]graveler.BranchProtectionBlockedAction{"main": {action1, action2, action3}},
expectedAllowedActions: map[string][]graveler.BranchProtectionBlockedAction{"main": {action4}},
patternToBlockedActions: map[string]*graveler.BranchProtectionBlockedActions{"main*": {Value: []graveler.BranchProtectionBlockedAction{action1}}, "mai*": {Value: []graveler.BranchProtectionBlockedAction{action2}}, "ma*": {Value: []graveler.BranchProtectionBlockedAction{action2, action3}}},
expectedBlockedActions: map[string]*graveler.BranchProtectionBlockedActions{"main": {Value: []graveler.BranchProtectionBlockedAction{action1, action2, action3}}},
expectedAllowedActions: map[string]*graveler.BranchProtectionBlockedActions{"main": {Value: []graveler.BranchProtectionBlockedAction{action4}}},
},
"no_rules": {
expectedAllowedActions: map[string][]graveler.BranchProtectionBlockedAction{"main": {action1, action2}},
expectedAllowedActions: map[string]*graveler.BranchProtectionBlockedActions{"main": {Value: []graveler.BranchProtectionBlockedAction{action1, action2}}},
},
}
for name, tst := range tests {
t.Run(name, func(t *testing.T) {
bpm := prepareTest(t, ctx)
for pattern, blockedActions := range tst.patternToBlockedActions {
testutil.Must(t, bpm.Add(ctx, repository, pattern, blockedActions))
}
testutil.Must(t, bpm.SetRules(ctx, repository, &graveler.BranchProtectionRules{
BranchPatternToBlockedActions: tst.patternToBlockedActions,
}, nil))

for branchID, expectedBlockedActions := range tst.expectedBlockedActions {
for _, action := range expectedBlockedActions {
for _, action := range expectedBlockedActions.Value {
res, err := bpm.IsBlocked(ctx, repository, graveler.BranchID(branchID), action)
testutil.Must(t, err)
if !res {
Expand All @@ -124,7 +128,7 @@ func TestIsBlocked(t *testing.T) {
}
}
for branchID, expectedAllowedActions := range tst.expectedAllowedActions {
for _, action := range expectedAllowedActions {
for _, action := range expectedAllowedActions.Value {
res, err := bpm.IsBlocked(ctx, repository, graveler.BranchID(branchID), action)
testutil.Must(t, err)
if res {
Expand Down

0 comments on commit eff3038

Please sign in to comment.