diff --git a/pkg/api/controller.go b/pkg/api/controller.go index 971a4d35a49..7f5c87c185b 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -1821,11 +1821,7 @@ func (c *Controller) SetBranchProtectionRules(w http.ResponseWriter, r *http.Req Value: blockedActions, } } - eTag := params.IfMatch - if swag.StringValue(eTag) == "" { - eTag = nil - } - err := c.Catalog.SetBranchProtectionRules(ctx, repository, rules, eTag) + err := c.Catalog.SetBranchProtectionRules(ctx, repository, rules, params.IfMatch) if c.handleAPIError(ctx, w, r, err) { return } @@ -3092,7 +3088,7 @@ func (c *Controller) InternalDeleteBranchProtectionRule(w http.ResponseWriter, r for p := range rules.BranchPatternToBlockedActions { if p == body.Pattern { delete(rules.BranchPatternToBlockedActions, p) - err = c.Catalog.SetBranchProtectionRules(ctx, repository, rules, swag.String(graveler.BranchProtectionSkipValidationChecksum)) + err = c.Catalog.SetBranchProtectionRules(ctx, repository, rules, nil) if c.handleAPIError(ctx, w, r, err) { return } @@ -3146,7 +3142,7 @@ func (c *Controller) InternalCreateBranchProtectionRule(w http.ResponseWriter, r Value: []graveler.BranchProtectionBlockedAction{graveler.BranchProtectionBlockedAction_STAGING_WRITE, graveler.BranchProtectionBlockedAction_COMMIT}, } rules.BranchPatternToBlockedActions[body.Pattern] = blockedActions - err = c.Catalog.SetBranchProtectionRules(ctx, repository, rules, swag.String(graveler.BranchProtectionSkipValidationChecksum)) + err = c.Catalog.SetBranchProtectionRules(ctx, repository, rules, nil) if c.handleAPIError(ctx, w, r, err) { return } diff --git a/pkg/catalog/interface.go b/pkg/catalog/interface.go index e067df88d79..b539ed06e9b 100644 --- a/pkg/catalog/interface.go +++ b/pkg/catalog/interface.go @@ -167,9 +167,9 @@ type Interface interface { // The returned checksum represents the current state of the rules, and can be passed to SetBranchProtectionRules for conditional updates. GetBranchProtectionRules(ctx context.Context, repositoryID string) (*graveler.BranchProtectionRules, *string, error) // SetBranchProtectionRules sets the branch protection rules for the given repository. - // If lastKnownChecksum doesn't match the current state, the update will fail with ErrPreconditionFailed. - // If lastKnownChecksum is nil, the update is performed only if no rules exist. - // If lastKnownChecksum is equal to BranchProtectionSkipValidationChecksum, the update is always performed. + // If lastKnownChecksum doesn't match the current state, the update fails with ErrPreconditionFailed. + // If lastKnownChecksum is the empty string, the update is performed only if no rules exist. + // If lastKnownChecksum is nil, the update is performed unconditionally. SetBranchProtectionRules(ctx context.Context, repositoryID string, rules *graveler.BranchProtectionRules, lastKnownChecksum *string) error // SetLinkAddress to validate single use limited in time of a given physical address diff --git a/pkg/graveler/branch/protection_manager.go b/pkg/graveler/branch/protection_manager.go index 8a3d8cd6fd0..ac0cd04ddd5 100644 --- a/pkg/graveler/branch/protection_manager.go +++ b/pkg/graveler/branch/protection_manager.go @@ -3,15 +3,12 @@ package branch import ( "context" "errors" - "fmt" "time" "github.com/gobwas/glob" "github.com/treeverse/lakefs/pkg/cache" "github.com/treeverse/lakefs/pkg/graveler" "github.com/treeverse/lakefs/pkg/graveler/settings" - "github.com/treeverse/lakefs/pkg/kv" - "google.golang.org/protobuf/proto" ) const ProtectionSettingKey = "protected_branches" @@ -22,10 +19,6 @@ const ( matcherCacheJitter = 1 * time.Minute ) -var ( - ErrRuleNotExists = fmt.Errorf("branch protection rule does not exist: %w", graveler.ErrNotFound) -) - type ProtectionManager struct { settingManager *settings.Manager matchers cache.Cache @@ -36,39 +29,28 @@ func NewProtectionManager(settingManager *settings.Manager) *ProtectionManager { } func (m *ProtectionManager) GetRules(ctx context.Context, repository *graveler.RepositoryRecord) (*graveler.BranchProtectionRules, *string, error) { - rulesMsg, checksum, err := m.settingManager.GetLatest(ctx, repository, ProtectionSettingKey, &graveler.BranchProtectionRules{}) - if errors.Is(err, graveler.ErrNotFound) { - return &graveler.BranchProtectionRules{}, nil, nil - } + rulesMsg := &graveler.BranchProtectionRules{} + checksum, err := m.settingManager.GetLatest(ctx, repository, ProtectionSettingKey, rulesMsg) if err != nil { return nil, nil, err } - if proto.Size(rulesMsg) == 0 { - return &graveler.BranchProtectionRules{}, nil, nil - } - return rulesMsg.(*graveler.BranchProtectionRules), checksum, nil -} -func (m *ProtectionManager) SetRules(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules) error { - return m.settingManager.Save(ctx, repository, ProtectionSettingKey, rules) + return rulesMsg, checksum, nil } -func (m *ProtectionManager) SetRulesIf(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules, lastKnownChecksum *string) error { - err := m.settingManager.SaveIf(ctx, repository, ProtectionSettingKey, rules, lastKnownChecksum) - if errors.Is(err, kv.ErrPredicateFailed) { - return graveler.ErrPreconditionFailed - } - return err +func (m *ProtectionManager) SetRules(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules, lastKnownChecksum *string) error { + return m.settingManager.Save(ctx, repository, ProtectionSettingKey, rules, lastKnownChecksum) } func (m *ProtectionManager) IsBlocked(ctx context.Context, repository *graveler.RepositoryRecord, branchID graveler.BranchID, action graveler.BranchProtectionBlockedAction) (bool, error) { - rules, err := m.settingManager.Get(ctx, repository, ProtectionSettingKey, &graveler.BranchProtectionRules{}) + rules := &graveler.BranchProtectionRules{} + err := m.settingManager.Get(ctx, repository, ProtectionSettingKey, rules) if errors.Is(err, graveler.ErrNotFound) { return false, nil } if err != nil { return false, err } - for pattern, blockedActions := range rules.(*graveler.BranchProtectionRules).BranchPatternToBlockedActions { + for pattern, blockedActions := range rules.BranchPatternToBlockedActions { pattern := pattern matcher, err := m.matchers.GetOrSet(pattern, func() (v interface{}, err error) { return glob.Compile(pattern) diff --git a/pkg/graveler/branch/protection_manager_test.go b/pkg/graveler/branch/protection_manager_test.go index 17ee1bd0017..d8c1159e622 100644 --- a/pkg/graveler/branch/protection_manager_test.go +++ b/pkg/graveler/branch/protection_manager_test.go @@ -9,6 +9,7 @@ import ( "github.com/go-openapi/swag" "github.com/go-test/deep" "github.com/golang/mock/gomock" + "github.com/stretchr/testify/require" "github.com/treeverse/lakefs/pkg/graveler" "github.com/treeverse/lakefs/pkg/graveler/branch" "github.com/treeverse/lakefs/pkg/graveler/mock" @@ -29,11 +30,8 @@ func TestSetAndGet(t *testing.T) { ctx := context.Background() bpm := prepareTest(t, ctx) rules, eTag, err := bpm.GetRules(ctx, repository) - testutil.Must(t, err) - if len(rules.BranchPatternToBlockedActions) != 0 { - t.Fatalf("expected no rules, got %d rules", len(rules.BranchPatternToBlockedActions)) - } - testutil.Must(t, bpm.SetRulesIf(ctx, repository, &graveler.BranchProtectionRules{ + require.ErrorIs(t, err, graveler.ErrNotFound) + testutil.Must(t, bpm.SetRules(ctx, repository, &graveler.BranchProtectionRules{ BranchPatternToBlockedActions: map[string]*graveler.BranchProtectionBlockedActions{ "main*": {Value: []graveler.BranchProtectionBlockedAction{ graveler.BranchProtectionBlockedAction_STAGING_WRITE}, @@ -57,7 +55,7 @@ func TestSetAndGet(t *testing.T) { func TestSetWrongETag(t *testing.T) { ctx := context.Background() bpm := prepareTest(t, ctx) - err := bpm.SetRulesIf(ctx, repository, &graveler.BranchProtectionRules{ + err := bpm.SetRules(ctx, repository, &graveler.BranchProtectionRules{ BranchPatternToBlockedActions: map[string]*graveler.BranchProtectionBlockedActions{ "main*": {Value: []graveler.BranchProtectionBlockedAction{ graveler.BranchProtectionBlockedAction_STAGING_WRITE}, @@ -106,7 +104,7 @@ func TestIsBlocked(t *testing.T) { bpm := prepareTest(t, ctx) testutil.Must(t, bpm.SetRules(ctx, repository, &graveler.BranchProtectionRules{ BranchPatternToBlockedActions: tst.patternToBlockedActions, - })) + }, nil)) for branchID, expectedBlockedActions := range tst.expectedBlockedActions { for _, action := range expectedBlockedActions.Value { diff --git a/pkg/graveler/graveler.go b/pkg/graveler/graveler.go index abd793bd489..6781780a217 100644 --- a/pkg/graveler/graveler.go +++ b/pkg/graveler/graveler.go @@ -9,7 +9,6 @@ import ( "time" "github.com/cenkalti/backoff/v4" - "github.com/go-openapi/swag" "github.com/google/uuid" "github.com/hashicorp/go-multierror" "github.com/prometheus/client_golang/prometheus" @@ -46,8 +45,6 @@ const ( RepoMetadataUpdateRandomFactor = 0.5 ) -const BranchProtectionSkipValidationChecksum = "skip-checksum-validation" - // Basic Types // DiffType represents the type of the change @@ -618,8 +615,8 @@ type VersionController interface { // SetBranchProtectionRules sets the branch protection rules for the repository. // If lastKnownChecksum doesn't match the current state, the update fails with ErrPreconditionFailed. - // If lastKnownChecksum is nil, the update is performed only if no rules exist. - // If lastKnownChecksum is equal to BranchProtectionSkipValidationChecksum, the update is always performed. + // If lastKnownChecksum is the empty string, the update is performed only if no rules exist. + // If lastKnownChecksum is nil, the update is performed unconditionally. SetBranchProtectionRules(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules, lastKnownChecksum *string) error // SetLinkAddress stores the address token under the repository. The token will be valid for addressTokenTime. @@ -1502,11 +1499,7 @@ func (g *Graveler) GetBranchProtectionRules(ctx context.Context, repository *Rep } func (g *Graveler) SetBranchProtectionRules(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules, lastKnownChecksum *string) error { - if swag.StringValue(lastKnownChecksum) == BranchProtectionSkipValidationChecksum { - // TODO(johnnyaug): remove this logic and constant once the legacy API is dropped. - return g.protectedBranchesManager.SetRules(ctx, repository, rules) - } - return g.protectedBranchesManager.SetRulesIf(ctx, repository, rules, lastKnownChecksum) + return g.protectedBranchesManager.SetRules(ctx, repository, rules, lastKnownChecksum) } // getFromStagingArea returns the most updated value of a given key in a branch staging area. @@ -3243,11 +3236,11 @@ type ProtectedBranchesManager interface { // GetRules returns all branch protection rules for the repository. // The returned checksum represents the current state of the rules, and can be passed to SetRulesIf for conditional updates. GetRules(ctx context.Context, repository *RepositoryRecord) (*BranchProtectionRules, *string, error) - SetRules(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules) error - // SetRulesIf sets the branch protection rules for the repository. + // SetRules sets the branch protection rules for the repository. // If lastKnownChecksum does not match the current checksum, returns ErrPreconditionFailed. - // If lastKnownChecksum is nil, the rules are set only if no rules are currently set. - SetRulesIf(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules, lastKnownChecksum *string) error + // If lastKnownChecksum is the empty string, the rules are set only if they are not currently set. + // If lastKnownChecksum is nil, the rules are set unconditionally. + SetRules(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules, lastKnownChecksum *string) error // IsBlocked returns whether the action is blocked by any branch protection rule matching the given branch. IsBlocked(ctx context.Context, repository *RepositoryRecord, branchID BranchID, action BranchProtectionBlockedAction) (bool, error) } diff --git a/pkg/graveler/mock/graveler.go b/pkg/graveler/mock/graveler.go index 3d8d514001b..33756dd0253 100644 --- a/pkg/graveler/mock/graveler.go +++ b/pkg/graveler/mock/graveler.go @@ -2902,29 +2902,15 @@ func (mr *MockProtectedBranchesManagerMockRecorder) IsBlocked(ctx, repository, b } // SetRules mocks base method. -func (m *MockProtectedBranchesManager) SetRules(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules) error { +func (m *MockProtectedBranchesManager) SetRules(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules, lastKnownChecksum *string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "SetRules", ctx, repository, rules) + ret := m.ctrl.Call(m, "SetRules", ctx, repository, rules, lastKnownChecksum) ret0, _ := ret[0].(error) return ret0 } // SetRules indicates an expected call of SetRules. -func (mr *MockProtectedBranchesManagerMockRecorder) SetRules(ctx, repository, rules interface{}) *gomock.Call { +func (mr *MockProtectedBranchesManagerMockRecorder) SetRules(ctx, repository, rules, lastKnownChecksum interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetRules", reflect.TypeOf((*MockProtectedBranchesManager)(nil).SetRules), ctx, repository, rules) -} - -// SetRulesIf mocks base method. -func (m *MockProtectedBranchesManager) SetRulesIf(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules, lastKnownChecksum *string) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "SetRulesIf", ctx, repository, rules, lastKnownChecksum) - ret0, _ := ret[0].(error) - return ret0 -} - -// SetRulesIf indicates an expected call of SetRulesIf. -func (mr *MockProtectedBranchesManagerMockRecorder) SetRulesIf(ctx, repository, rules, lastKnownChecksum interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetRulesIf", reflect.TypeOf((*MockProtectedBranchesManager)(nil).SetRulesIf), ctx, repository, rules, lastKnownChecksum) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetRules", reflect.TypeOf((*MockProtectedBranchesManager)(nil).SetRules), ctx, repository, rules, lastKnownChecksum) } diff --git a/pkg/graveler/settings/manager.go b/pkg/graveler/settings/manager.go index 40ab46bc2cb..66e357b6228 100644 --- a/pkg/graveler/settings/manager.go +++ b/pkg/graveler/settings/manager.go @@ -59,44 +59,47 @@ func NewManager(refManager graveler.RefManager, store kv.Store, options ...Manag return m } -// Save persists the given setting under the given repository and key. Overrides settings key in KV Store -func (m *Manager) Save(ctx context.Context, repository *graveler.RepositoryRecord, key string, setting proto.Message) error { - logSetting(logging.FromContext(ctx), repository.RepositoryID, key, setting, "saving repository-level setting") - // Write key in KV Store - return kv.SetMsg(ctx, m.store, graveler.RepoPartition(repository), []byte(graveler.SettingsPath(key)), setting) -} - -// SaveIf persists the given setting under the given repository and key. Overrides settings key in KV Store. +// Save persists the given setting under the given repository and key. Overrides settings key in KV Store. // The setting is persisted only if the current version of the setting matches the given checksum. -// If lastKnownChecksum is nil, the setting is persisted only if it does not exist. -func (m *Manager) SaveIf(ctx context.Context, repository *graveler.RepositoryRecord, key string, setting proto.Message, lastKnownChecksum *string) error { +// If lastKnownChecksum is the empty string, the setting is persisted only if it does not exist. +// If lastKnownChecksum is nil, the setting is persisted unconditionally. +func (m *Manager) Save(ctx context.Context, repository *graveler.RepositoryRecord, key string, setting proto.Message, lastKnownChecksum *string) error { logSetting(logging.FromContext(ctx), repository.RepositoryID, key, setting, "saving repository-level setting") + if lastKnownChecksum == nil { + return kv.SetMsg(ctx, m.store, graveler.RepoPartition(repository), []byte(graveler.SettingsPath(key)), setting) + } + if *lastKnownChecksum == "" { + err := kv.SetMsgIf(ctx, m.store, graveler.RepoPartition(repository), []byte(graveler.SettingsPath(key)), setting, nil) + if errors.Is(err, kv.ErrPredicateFailed) { + return graveler.ErrPreconditionFailed + } + return err + } valueWithPredicate, err := m.store.Get(ctx, []byte(graveler.RepoPartition(repository)), []byte(graveler.SettingsPath(key))) - if err != nil && !errors.Is(err, kv.ErrNotFound) { + if errors.Is(err, kv.ErrNotFound) { + return graveler.ErrPreconditionFailed + } + if err != nil { return err } - var currentChecksum *string - var currentPredicate kv.Predicate - if valueWithPredicate != nil { - if valueWithPredicate.Value != nil { - currentChecksum, err = computeChecksum(valueWithPredicate.Value) - } - if err != nil { - return err - } - currentPredicate = valueWithPredicate.Predicate + currentChecksum, err := computeChecksum(valueWithPredicate.Value) + if err != nil { + return err } - if swag.StringValue(currentChecksum) != swag.StringValue(lastKnownChecksum) { + if *currentChecksum != *lastKnownChecksum { return graveler.ErrPreconditionFailed } - err = kv.SetMsgIf(ctx, m.store, graveler.RepoPartition(repository), []byte(graveler.SettingsPath(key)), setting, currentPredicate) - if err != nil && errors.Is(err, kv.ErrPredicateFailed) { + err = kv.SetMsgIf(ctx, m.store, graveler.RepoPartition(repository), []byte(graveler.SettingsPath(key)), setting, valueWithPredicate.Predicate) + if errors.Is(err, kv.ErrPredicateFailed) { return graveler.ErrPreconditionFailed } return err } func computeChecksum(value []byte) (*string, error) { + if value == nil { + return nil, graveler.ErrInvalidValue + } h := sha256.New() _, err := h.Write(value) if err != nil { @@ -105,52 +108,53 @@ func computeChecksum(value []byte) (*string, error) { return swag.String(hex.EncodeToString(h.Sum(nil))), nil } -// GetLatest returns the latest setting under the given repository and key, without using the cache. -// The returned checksum represents the version of the setting, and can be passed to SaveIf for conditional updates. -func (m *Manager) GetLatest(ctx context.Context, repository *graveler.RepositoryRecord, key string, settingTemplate proto.Message) (proto.Message, *string, error) { +// GetLatest loads the latest setting into dst. +// It returns a checksum representing the version of the setting, which can be passed to SaveIf for conditional updates. +// The checksum of a non-existent setting is the empty string. +func (m *Manager) GetLatest(ctx context.Context, repository *graveler.RepositoryRecord, key string, dst proto.Message) (*string, error) { settings, err := m.store.Get(ctx, []byte(graveler.RepoPartition(repository)), []byte(graveler.SettingsPath(key))) if err != nil { if errors.Is(err, kv.ErrNotFound) { err = graveler.ErrNotFound } - return nil, nil, err + return nil, err } checksum, err := computeChecksum(settings.Value) if err != nil { - return nil, nil, err + return nil, err } - data := settingTemplate.ProtoReflect().Interface() - err = proto.Unmarshal(settings.Value, data) + err = proto.Unmarshal(settings.Value, dst) if err != nil { - return nil, nil, err + return nil, err } - logSetting(logging.FromContext(ctx), repository.RepositoryID, key, data, "got repository-level setting") - return data, checksum, nil + logSetting(logging.FromContext(ctx), repository.RepositoryID, key, dst, "got repository-level setting") + return checksum, nil } -// Get fetches the setting under the given repository and key, and returns the result. +// Get fetches the setting under the given repository and key, and loads it into dst. // The result is eventually consistent: it is not guaranteed to be the most up-to-date setting. The cache expiry period is 1 second. -// The settingTemplate parameter is used to determine the returned type. -// The returned checksum represents the version of the setting, and can be used in SaveIf to perform an atomic update. -func (m *Manager) Get(ctx context.Context, repository *graveler.RepositoryRecord, key string, settingTemplate proto.Message) (proto.Message, error) { +func (m *Manager) Get(ctx context.Context, repository *graveler.RepositoryRecord, key string, dst proto.Message) error { k := cacheKey{ RepositoryID: repository.RepositoryID, Key: key, } + tmp := proto.Clone(dst) setting, err := m.cache.GetOrSet(k, func() (v interface{}, err error) { - setting, _, err := m.GetLatest(ctx, repository, key, settingTemplate) + _, err = m.GetLatest(ctx, repository, key, tmp) if errors.Is(err, graveler.ErrNotFound) { + // do not return this error here, so that empty settings are cached return nil, nil } - return setting, err + return tmp, err }) if err != nil { - return nil, err + return err } if setting == nil { - return nil, graveler.ErrNotFound + return graveler.ErrNotFound } - return setting.(proto.Message), nil + proto.Merge(dst, setting.(proto.Message)) + return nil } func logSetting(logger logging.Logger, repositoryID graveler.RepositoryID, key string, setting proto.Message, logMsg string) { diff --git a/pkg/graveler/settings/manager_test.go b/pkg/graveler/settings/manager_test.go index 87a96fcd0ad..b91fd8e06a4 100644 --- a/pkg/graveler/settings/manager_test.go +++ b/pkg/graveler/settings/manager_test.go @@ -2,12 +2,12 @@ package settings_test import ( "context" - "errors" "testing" "github.com/go-openapi/swag" "github.com/go-test/deep" "github.com/golang/mock/gomock" + "github.com/stretchr/testify/require" "github.com/treeverse/lakefs/pkg/block" "github.com/treeverse/lakefs/pkg/block/mem" "github.com/treeverse/lakefs/pkg/cache" @@ -41,6 +41,15 @@ func (m *mockCache) GetOrSet(k interface{}, setFn cache.SetFn) (v interface{}, e m.c[k] = val return val, nil } +func TestNonExistent(t *testing.T) { + ctx := context.Background() + m, _ := prepareTest(t, ctx, nil, nil) + setting := &settings.ExampleSettings{} + err := m.Get(ctx, repository, "settingKey", setting) + require.ErrorIs(t, err, graveler.ErrNotFound) + _, err = m.GetLatest(ctx, repository, "settingKey", setting) + require.ErrorIs(t, err, graveler.ErrNotFound) +} func TestSaveAndGet(t *testing.T) { ctx := context.Background() @@ -48,27 +57,29 @@ func TestSaveAndGet(t *testing.T) { c: make(map[interface{}]interface{}), } m, _ := prepareTest(t, ctx, mc, nil) - emptySettings := &settings.ExampleSettings{} - firstSettings := &settings.ExampleSettings{ExampleInt: 5, ExampleStr: "hello", ExampleMap: map[string]int32{"boo": 6}} - err := m.Save(ctx, repository, "settingKey", firstSettings) + firstSettings := newSetting(5, 6, "hello") + err := m.Save(ctx, repository, "settingKey", firstSettings, nil) testutil.Must(t, err) - gotSettings, err := m.Get(ctx, repository, "settingKey", emptySettings) + gotSettings := &settings.ExampleSettings{} + err = m.Get(ctx, repository, "settingKey", gotSettings) testutil.Must(t, err) if diff := deep.Equal(firstSettings, gotSettings); diff != nil { t.Fatal("got unexpected settings:", diff) } - secondSettings := &settings.ExampleSettings{ExampleInt: 15, ExampleStr: "hi", ExampleMap: map[string]int32{"boo": 16}} - err = m.Save(ctx, repository, "settingKey", secondSettings) + secondSettings := newSetting(15, 16, "hi") + err = m.Save(ctx, repository, "settingKey", secondSettings, nil) testutil.Must(t, err) // the result should be cached, and we should get the first settings: - gotSettings, err = m.Get(ctx, repository, "settingKey", emptySettings) + gotSettings = &settings.ExampleSettings{} + err = m.Get(ctx, repository, "settingKey", gotSettings) testutil.Must(t, err) if diff := deep.Equal(firstSettings, gotSettings); diff != nil { t.Fatal("got unexpected settings:", diff) } // after clearing the mc, we should get the new settings: mc.c = make(map[interface{}]interface{}) - gotSettings, err = m.Get(ctx, repository, "settingKey", emptySettings) + gotSettings = &settings.ExampleSettings{} + err = m.Get(ctx, repository, "settingKey", gotSettings) testutil.Must(t, err) if diff := deep.Equal(secondSettings, gotSettings); diff != nil { t.Fatal("got unexpected settings:", diff) @@ -77,16 +88,12 @@ func TestSaveAndGet(t *testing.T) { func TestGetLatest(t *testing.T) { ctx := context.Background() m, _ := prepareTest(t, ctx, nil, nil) - emptySettings := &settings.ExampleSettings{} - setting, _, err := m.GetLatest(ctx, repository, "settingKey", emptySettings) - if !errors.Is(err, graveler.ErrNotFound) { - t.Fatalf("expected ErrNotFound, got %v", err) - } - err = m.Save(ctx, repository, "settingKey", &settings.ExampleSettings{ExampleInt: 5, ExampleStr: "hello", ExampleMap: map[string]int32{"boo": 6}}) + err := m.Save(ctx, repository, "settingKey", newSetting(5, 6, "hello"), nil) testutil.Must(t, err) - setting, eTag, err := m.GetLatest(ctx, repository, "settingKey", emptySettings) + setting := &settings.ExampleSettings{} + eTag, err := m.GetLatest(ctx, repository, "settingKey", setting) testutil.Must(t, err) - if diff := deep.Equal(&settings.ExampleSettings{ExampleInt: 5, ExampleStr: "hello", ExampleMap: map[string]int32{"boo": 6}}, setting); diff != nil { + if diff := deep.Equal(newSetting(5, 6, "hello"), setting); diff != nil { t.Fatal("got unexpected settings:", diff) } if eTag == nil || *eTag == "" { @@ -94,28 +101,28 @@ func TestGetLatest(t *testing.T) { } } -func TestSaveIf(t *testing.T) { +func TestConditionalUpdate(t *testing.T) { ctx := context.Background() mc := &mockCache{ c: make(map[interface{}]interface{}), } m, _ := prepareTest(t, ctx, mc, nil) - emptySettings := &settings.ExampleSettings{} - firstSettings := &settings.ExampleSettings{ExampleInt: 5, ExampleStr: "hello", ExampleMap: map[string]int32{"boo": 6}} - err := m.SaveIf(ctx, repository, "settingKey", firstSettings, nil) + firstSettings := newSetting(5, 6, "hello") + err := m.Save(ctx, repository, "settingKey", firstSettings, swag.String("WRONG_CHECKSUM")) + require.ErrorIs(t, err, graveler.ErrPreconditionFailed) + err = m.Save(ctx, repository, "settingKey", firstSettings, swag.String("")) testutil.Must(t, err) - gotSettings, checksum, err := m.GetLatest(ctx, repository, "settingKey", emptySettings) + gotSettings := &settings.ExampleSettings{} + checksum, err := m.GetLatest(ctx, repository, "settingKey", gotSettings) testutil.Must(t, err) if diff := deep.Equal(firstSettings, gotSettings); diff != nil { t.Fatal("got unexpected settings:", diff) } - secondSettings := &settings.ExampleSettings{ExampleInt: 15, ExampleStr: "hi", ExampleMap: map[string]int32{"boo": 16}} - err = m.SaveIf(ctx, repository, "settingKey", secondSettings, checksum) + secondSettings := newSetting(15, 16, "hi") + err = m.Save(ctx, repository, "settingKey", secondSettings, checksum) testutil.Must(t, err) - err = m.SaveIf(ctx, repository, "settingKey", secondSettings, swag.String("WRONG_CHECKSUM")) - if !errors.Is(err, graveler.ErrPreconditionFailed) { - t.Fatalf("expected ErrPreconditionFailed, got %v", err) - } + err = m.Save(ctx, repository, "settingKey", secondSettings, swag.String("WRONG_CHECKSUM")) + require.ErrorIs(t, err, graveler.ErrPreconditionFailed) } func prepareTest(t *testing.T, ctx context.Context, refCache cache.Cache, branchLockCallback func(context.Context, *graveler.RepositoryRecord, graveler.BranchID, func() (interface{}, error)) (interface{}, error)) (*settings.Manager, block.Adapter) { @@ -142,3 +149,11 @@ func prepareTest(t *testing.T, ctx context.Context, refCache cache.Cache, branch refManager.EXPECT().GetRepository(ctx, gomock.Eq(repository.RepositoryID)).AnyTimes().Return(repository, nil) return m, blockAdapter } + +func newSetting(a int32, b int32, c string) *settings.ExampleSettings { + return &settings.ExampleSettings{ + ExampleInt: a, + ExampleStr: c, + ExampleMap: map[string]int32{"boo": b}, + } +} diff --git a/pkg/samplerepo/samplecontent.go b/pkg/samplerepo/samplecontent.go index 314507df93c..80a07cda66e 100644 --- a/pkg/samplerepo/samplecontent.go +++ b/pkg/samplerepo/samplecontent.go @@ -118,19 +118,9 @@ func PopulateSampleRepo(ctx context.Context, repo *catalog.Repository, cat catal } func AddBranchProtection(ctx context.Context, repo *catalog.Repository, cat catalog.Interface) error { - // Set branch protection on the main branch - rules, checksum, err := cat.GetBranchProtectionRules(ctx, repo.Name) - if err != nil { - return err - } - if rules == nil { - rules = &graveler.BranchProtectionRules{} - } - if rules.BranchPatternToBlockedActions == nil { - rules.BranchPatternToBlockedActions = make(map[string]*graveler.BranchProtectionBlockedActions) - } - rules.BranchPatternToBlockedActions[repo.DefaultBranch] = &graveler.BranchProtectionBlockedActions{ - Value: []graveler.BranchProtectionBlockedAction{graveler.BranchProtectionBlockedAction_COMMIT}, - } - return cat.SetBranchProtectionRules(ctx, repo.Name, rules, checksum) + return cat.SetBranchProtectionRules(ctx, repo.Name, &graveler.BranchProtectionRules{ + BranchPatternToBlockedActions: map[string]*graveler.BranchProtectionBlockedActions{ + repo.DefaultBranch: {Value: []graveler.BranchProtectionBlockedAction{graveler.BranchProtectionBlockedAction_COMMIT}}, + }, + }, swag.String("")) } diff --git a/webui/src/lib/api/index.js b/webui/src/lib/api/index.js index 6365239fe02..71364b93b6f 100644 --- a/webui/src/lib/api/index.js +++ b/webui/src/lib/api/index.js @@ -1003,9 +1003,8 @@ class BranchProtectionRules { } async setRules(repoID, rules, lastKnownChecksum) { - let additionalHeaders = {} + const additionalHeaders = {} if (lastKnownChecksum) { - console.log(`setting branch protection rules with checksum ${lastKnownChecksum}`) additionalHeaders['If-Match'] = lastKnownChecksum } const response = await apiRequest(`/repositories/${encodeURIComponent(repoID)}/settings/branch_protection`, {