Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pull Requests: Add proto and ref manager logic #8178

Merged
merged 3 commits into from
Sep 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/actions/kv_run_results_iterator.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ type KVRunResultIterator struct {
// 'after' determines the runID which we should start the scan from, used for pagination
func NewKVRunResultIterator(ctx context.Context, store kv.Store, repositoryID, branchID, commitID, after string) (*KVRunResultIterator, error) {
if branchID != "" && commitID != "" {
return nil, fmt.Errorf("can't use both branchID and CommitID: %w", ErrParamConflict)
return nil, fmt.Errorf("can't use both branchID and MergedCommitID: %w", ErrParamConflict)
}

var (
Expand Down
6 changes: 3 additions & 3 deletions pkg/actions/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ hooks:
t.Errorf("Run() manifest RunID %s, expected %s", lastManifest.Run.RunID, record.RunID)
}
if lastManifest.Run.CommitID != "" {
t.Errorf("Run() manifest CommitID %s, expected empty", lastManifest.Run.CommitID)
t.Errorf("Run() manifest MergedCommitID %s, expected empty", lastManifest.Run.CommitID)
}
lastManifest = nil

Expand All @@ -204,7 +204,7 @@ hooks:
t.Errorf("UpdateCommitID() manifest RunID %s, expected %s", lastManifest.Run.RunID, record.RunID)
}
if lastManifest.Run.CommitID != "commit1" {
t.Errorf("UpdateCommitID() manifest CommitID %s, expected 'commit1'", lastManifest.Run.CommitID)
t.Errorf("UpdateCommitID() manifest MergedCommitID %s, expected 'commit1'", lastManifest.Run.CommitID)
}

// get run result
Expand Down Expand Up @@ -236,7 +236,7 @@ hooks:
}
const expectedCommitID = "commit1"
if runResult.CommitID != expectedCommitID {
t.Errorf("GetRunResult() result CommitID=%s, expect=%s", runResult.CommitID, expectedCommitID)
t.Errorf("GetRunResult() result MergedCommitID=%s, expect=%s", runResult.CommitID, expectedCommitID)
}

require.Equal(t, 3, mockStatsCollector.Hits["pre-commit"])
Expand Down
2 changes: 2 additions & 0 deletions pkg/graveler/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ var (
ErrSkipValueUpdate = errors.New("skip value update")
ErrImport = wrapError(ErrUserVisible, "import error")
ErrReadOnlyRepository = wrapError(ErrUserVisible, "read-only repository")
ErrPullRequestNotFound = fmt.Errorf("pull request %w", ErrNotFound)
ErrPullRequestExists = fmt.Errorf("pull request already exists: %w", ErrNotUnique)
)

// wrappedError is an error for wrapping another error while ignoring its message.
Expand Down
38 changes: 37 additions & 1 deletion pkg/graveler/graveler.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ const (
// Type: Branch / Tag / Commit
// BranchID: for type ReferenceTypeBranch will hold the branch ID
// ResolvedBranchModifier: branch indicator if resolved to a branch the latest commit, staging or none was specified.
// CommitID: the commit ID of the branch head, tag or specific hash.
// MergedCommitID: the commit ID of the branch head, tag or specific hash.
// StagingToken: empty if ResolvedBranchModifier is ResolvedBranchModifierCommitted.
type ResolvedRef struct {
Type ReferenceType
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a safer approach than casting.
We use this method in many places in our code

return string(id)
}

// PullUpdateFunc Used to pass validation call back to ref manager for UpdatePullRequest flow
type PullUpdateFunc func(request *PullRequest) (*PullRequest, error)

type PullRequest struct {
CreationDate time.Time
Status PullRequestStatus
Title string
Author string
Description string
// Source - source branch of pull request
Source string
// Destination - destination branch of pull request
Destination string
// MergedCommitID - The commit ID that of the source at the time of the merge. Relevant only for merged PRs
MergedCommitID string
}

type PullRequestRecord struct {
ID PullRequestID
PullRequest
}

type ImportStatus struct {
ID ImportID
Completed bool
Expand Down Expand Up @@ -882,6 +910,14 @@ type RefManager interface {

// DeleteExpiredImports deletes expired imports on a given repository
DeleteExpiredImports(ctx context.Context, repository *RepositoryRecord) error

GetPullRequest(ctx context.Context, repository *RepositoryRecord, pullID PullRequestID) (*PullRequest, error)

CreatePullRequest(ctx context.Context, repository *RepositoryRecord, pullRequestID PullRequestID, pullRequest *PullRequest) error

DeletePullRequest(ctx context.Context, repository *RepositoryRecord, pullRequestID PullRequestID) error

UpdatePullRequest(ctx context.Context, repository *RepositoryRecord, pullRequestID PullRequestID, f PullUpdateFunc) error
}

// CommittedManager reads and applies committed snapshots
Expand Down
Loading
Loading