Simplify branch protect interface #6688
Conversation
johnnyaug
added
the
exclude-changelog
| if proto.Size(rulesMsg) == 0 { | ||
| return &graveler.BranchProtectionRules{}, nil, nil | ||
| } |
There was a problem hiding this comment.
In this case we have data, so we should return the checksum. Even if the data is an empty slice.
pkg/graveler/settings/manager.go
Outdated
| 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 swag.String(""), err |
There was a problem hiding this comment.
I understood why we would like to return a value in case of an error, specific not found. But I hope that the caller will work based on the error and the convention of the API and not use the checksum value in case of an error.
There was a problem hiding this comment.
Done - not returning empty string.
pkg/graveler/settings/manager.go
Outdated
| if errors.Is(err, graveler.ErrNotFound) { | ||
| 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 | ||
| } |
There was a problem hiding this comment.
About graveler.ErrNotFound: we check for the specific error inside the callback and also have if settings == nil to return the same error.
There was a problem hiding this comment.
That's because it may arrive from the cache, which doesn't return this error.
Following @nopcoder's comments on #6682.
Resolves #6674.