Pull Requests: Add proto and ref manager logic #8178
Conversation
E2E Test Results - DynamoDB Local - Local Block Adapter |
E2E Test Results - Quickstart |
N-o-Z
added
the
exclude-changelog
| @@ -260,6 +260,34 @@ type RangeID string | |||
| // ImportID represents an import process id in the ref-store | |||
| type ImportID string | |||
|
|
|||
| type PullRequestID string | |||
|
|
|||
| func (id PullRequestID) String() string { | |||
There was a problem hiding this comment.
It's a safer approach than casting.
We use this method in many places in our code
pkg/graveler/graveler.go
Outdated
| type PullUpdateFunc func(request *PullRequest) (*PullRequest, error) | ||
|
|
||
| type PullRequest struct { | ||
| CreatedAt time.Time |
There was a problem hiding this comment.
For the other objects in this package (Repo and Commit) this is called CreationDate.
Please align with this.
pkg/graveler/graveler.go
Outdated
| // Destination - destination branch of pull request | ||
| Destination string | ||
| // CommitID - The commit ID that of the source at the time of the merge. Relevant only for merged PRs | ||
| CommitID string |
There was a problem hiding this comment.
Maybe MergedCommitID or something like this, to be more explicit?
| importsPrefix = "imports" | ||
| repoMetadataPrefix = "repo-metadata" | ||
| pullRequestsPrefix = "pulls" |
| @@ -69,10 +69,6 @@ func SettingsPath(key string) string { | |||
| return kv.FormatPath(settingsPrefix, key) | |||
| } | |||
|
|
|||
| func LinkedAddressPath(key string) string { | |||
| t.Run("update_pull_request_exists", func(t *testing.T) { | ||
| require.NoError(t, r.CreatePullRequest(ctx, repository, expected.ID, &expected.PullRequest)) | ||
|
|
||
| err := r.UpdatePullRequest(ctx, repository, expected.ID, func(request *graveler.PullRequest) (*graveler.PullRequest, error) { |
There was a problem hiding this comment.
Generally, prefer creating a new object instead of modifying an existing one.
In this case, the one that will add a new test after this will expect to have "some title" as the title of expected, but it'll be different.
There was a problem hiding this comment.
True, considering this is a unit test let's defer this to later
| err = graveler.ErrPullRequestNotFound | ||
| } | ||
| if err != nil { | ||
| return nil, nil, err |
There was a problem hiding this comment.
I guess we also do it in other places, but note that this means that the manager might return kv errors (where I would expect it to return graveler errors).
There was a problem hiding this comment.
That's correct, but should be handled in higher layers
Closes #8177
Change Description
Create proto data models and logic in ref manager