Fix quickstart bug: branch protection #6682

johnnyaug · 2023-10-02T14:35:23Z

Resolves #6674.

Fix nil check in quickstart.
Fix delete branch protection rule in UI.
Do not use checksum from the KV store as the ETag.

N-o-Z

Some comments.
In addition:
Can we add an esti test for S3 which checks the creation of the sample repo?

N-o-Z · 2023-10-02T15:38:34Z

pkg/graveler/graveler.go

 	return g.protectedBranchesManager.GetRules(ctx, repository)
 }

 func (g *Graveler) SetBranchProtectionRules(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules, lastKnownChecksum *string) error {
 	if lastKnownChecksum == nil {
 		return g.protectedBranchesManager.SetRules(ctx, repository, rules)
 	}
-	return g.protectedBranchesManager.SetRulesIf(ctx, repository, rules, *lastKnownChecksum)
+	if *lastKnownChecksum == "" {


Seem like the logic should be reversed?:

lastKnownChecksum is nil -> SetIf

lastKnownChecksum is "" -> Set
And of course change the logic in the invoking methods accordingly

Agree that this is ugly. I argue that "" as the "skip validation checksum" is just as problematic. Added a dedicated skip-validation constant to overcome this. This will be removed once the legacy API is dropped.

N-o-Z · 2023-10-02T15:39:28Z

pkg/graveler/graveler.go


 	// 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 the empty string, the update is performed only if no rules exist.


Should be nil and not empty

johnnyaug · 2023-10-02T16:42:14Z

@N-o-Z, added an Esti test for sample repo creation.

N-o-Z

LGTM

nopcoder · 2023-10-02T17:06:53Z

webui/src/pages/repositories/repository/settings/branches.jsx

+            setRefresh(!refresh)
+            setDeleteButtonDisabled(false)
+        }).catch(err => {
+            setDeleteButtonDisabled(false)
+            setActionError(err)


can move setDeleteButtonDisabled(false) to finally

nopcoder · 2023-10-02T17:07:46Z

webui/src/lib/api/index.js

@@ -1003,28 +1003,19 @@ class BranchProtectionRules {
    }

    async setRules(repoID, rules, lastKnownChecksum) {
+        let additionalHeaders = {}
+        if (lastKnownChecksum) {
+            console.log(`setting branch protection rules with checksum ${lastKnownChecksum}`)


console log

nopcoder · 2023-10-02T17:08:02Z

webui/src/lib/api/index.js

@@ -1003,28 +1003,19 @@ class BranchProtectionRules {
    }

    async setRules(repoID, rules, lastKnownChecksum) {
+        let additionalHeaders = {}


can use const

nopcoder · 2023-10-02T17:11:11Z

pkg/api/controller.go

@@ -1788,7 +1788,7 @@ func (c *Controller) GetBranchProtectionRules(w http.ResponseWriter, r *http.Req
 			Pattern: pattern,
 		})
 	}
-	w.Header().Set("ETag", eTag)
+	w.Header().Set("ETag", swag.StringValue(eTag))


if etag is nil it means there is no data - so I guess we should not send empty etag, right? as it is valid value as indicator and not a value for a content that is not there.

nopcoder · 2023-10-02T17:13:19Z

pkg/api/controller.go

@@ -1816,6 +1816,9 @@ func (c *Controller) SetBranchProtectionRules(w http.ResponseWriter, r *http.Req
 		}
 	}
 	eTag := params.IfMatch
+	if swag.StringValue(eTag) == "" {
+		eTag = nil


Thought we kept the kv way:
As the client call without eTag - no check to be done.
If eTag is empty - check no previous value is set.

This API does not allow skipping the validation.

nopcoder · 2023-10-03T06:40:34Z

pkg/graveler/settings/manager.go

-	decodedChecksum, err := base64.StdEncoding.DecodeString(lastKnownChecksum)
-	if err != nil {
-		return fmt.Errorf("decode checksum: %w", err)
+	valueWithPredicate, err := m.store.Get(ctx, []byte(graveler.RepoPartition(repository)), []byte(graveler.SettingsPath(key)))


Case lastKnownChecksum is nil we don't need to Get the current value, we can just return from SetMsg - it will also remove the check if valueWithPredicate is nil later as successful call should return non-nil.

nopcoder · 2023-10-03T06:49:11Z

pkg/graveler/settings/manager.go

+		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) {


Simplify

Suggested change

if err != nil && errors.Is(err, kv.ErrPredicateFailed) {

if errors.Is(err, kv.ErrPredicateFailed) {

nopcoder · 2023-10-03T06:52:46Z

pkg/graveler/settings/manager.go

+	var currentPredicate kv.Predicate
+	if valueWithPredicate != nil {
+		if valueWithPredicate.Value != nil {
+			currentChecksum, err = computeChecksum(valueWithPredicate.Value)
+		}
+		if err != nil {
+			return err
+		}
+		currentPredicate = valueWithPredicate.Predicate
+	}
+	if swag.StringValue(currentChecksum) != swag.StringValue(lastKnownChecksum) {
+		return graveler.ErrPreconditionFailed
+	}
+	err = kv.SetMsgIf(ctx, m.store, graveler.RepoPartition(repository), []byte(graveler.SettingsPath(key)), setting, currentPredicate)


Main concern here is just the flow where we call SetMsgIf while using currentPredicate value that was not part of valueWithPredicate.
The promise from kv is to support SetIf only with predicate that was returned from Get or explicit call with nil to indicate that the value should not be exists.

Suggest to check if lastKnownChecksum is "" and call SetIf with nil - for all the rest we can compare the checksum as above and call SetMsgIf

nopcoder · 2023-10-03T06:56:46Z

pkg/graveler/settings/manager.go

-func (m *Manager) GetLatest(ctx context.Context, repository *graveler.RepositoryRecord, key string, settingTemplate proto.Message) (proto.Message, string, error) {
-	data := settingTemplate.ProtoReflect().Interface()
-	pred, err := m.getWithPredicate(ctx, repository, key, data)
+func (m *Manager) GetLatest(ctx context.Context, repository *graveler.RepositoryRecord, key string, settingTemplate proto.Message) (proto.Message, *string, error) {


Accept protoreflect.ProtoMessage as settingMsg and unmarshal the data into it instead of template+reflect+create and return.

Fix quickstart bug: branch protection

da7ff3a

johnnyaug added the exclude-changelog PR description should not be included in next release changelog label Oct 2, 2023

johnnyaug added 2 commits October 2, 2023 17:44

flitzenmoch

e65087e

fix set on empty

2a35db0

johnnyaug requested review from nopcoder and N-o-Z October 2, 2023 14:57

N-o-Z requested changes Oct 2, 2023

View reviewed changes

johnnyaug added 3 commits October 2, 2023 19:03

cr fix

a911f44

add sample repo esti test

e7c0e61

do not create repo twice!

d60be14

johnnyaug requested a review from N-o-Z October 2, 2023 16:41

N-o-Z approved these changes Oct 2, 2023

View reviewed changes

johnnyaug merged commit d96af68 into master Oct 3, 2023
34 checks passed

johnnyaug deleted the fix/branch_protection_empty_state branch October 3, 2023 00:21

nopcoder suggested changes Oct 3, 2023

View reviewed changes

johnnyaug mentioned this pull request Oct 3, 2023

Simplify branch protect interface #6688

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix quickstart bug: branch protection #6682

Fix quickstart bug: branch protection #6682

johnnyaug commented Oct 2, 2023 •

edited

Loading

N-o-Z left a comment

N-o-Z Oct 2, 2023

johnnyaug Oct 2, 2023

N-o-Z Oct 2, 2023

johnnyaug commented Oct 2, 2023

N-o-Z left a comment

nopcoder Oct 2, 2023

nopcoder Oct 2, 2023

nopcoder Oct 2, 2023

nopcoder Oct 2, 2023

nopcoder Oct 2, 2023

johnnyaug Oct 3, 2023

nopcoder Oct 3, 2023

nopcoder Oct 3, 2023

nopcoder Oct 3, 2023

nopcoder Oct 3, 2023

	if err != nil && errors.Is(err, kv.ErrPredicateFailed) {
	if errors.Is(err, kv.ErrPredicateFailed) {

Fix quickstart bug: branch protection #6682

Fix quickstart bug: branch protection #6682

Conversation

johnnyaug commented Oct 2, 2023 • edited Loading

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug commented Oct 2, 2023

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug commented Oct 2, 2023 •

edited

Loading