fix: pre-merge is missing commit ID (#7750)

treeverse · May 13, 2024 · 3877c20 · 3877c20
1 parent 071120b
commit 3877c20
Show file tree

Hide file tree

Showing 7 changed files with 18 additions and 15 deletions.
diff --git a/.github/workflows/docs-pr.yaml b/.github/workflows/docs-pr.yaml
@@ -61,7 +61,7 @@ jobs:
         id: lychee
         uses: lycheeverse/[email protected]
         with:
-          args: docs/_site --no-progress --exclude-file=docs/.lycheeignore
+          args: docs/_site --no-progress --exclude-mail --exclude-file=docs/.lycheeignore
           fail: true
           jobSummary: true
           format: markdown

diff --git a/docs/.lycheeignore b/docs/.lycheeignore
@@ -20,4 +20,6 @@ https://www.linkedin.com/company/treeverse
 https://your.domain.io
 lakefs://
 local://
-s3://
+s3://
+https://lakefs.io/
+https://lakefs.acme.com/
diff --git a/docs/howto/hooks/webhooks.md b/docs/howto/hooks/webhooks.md
@@ -73,11 +73,13 @@ Upon execution, a webhook will send a request containing a JSON object with the
 | commit_message[^2]  | The message for the commit (or merge) that is taking place        | string |
 | committer[^2]       | Name of the committer                                             | string |
 | commit_metadata[^2] | The metadata for the commit that is taking place                  | string |
+| commit_id[^2,^4]    | The ID of the commit that is being created              | string |
 | tag_id[^3]          | The ID of the created/deleted tag                                 | string |
 
 [^1]: N\A for Tag events  
 [^2]: N\A for Tag and Create/Delete Branch events  
 [^3]: Applicable only for Tag events
+[^4]: Applicable to commit/merge events. For merges, this represents the merge commit ID to be created if the merge operation succeeds.
 
 Example:
 ```json
@@ -90,6 +92,7 @@ Example:
   "branch_id": "feature-1",
   "source_ref": "feature-1",
   "commit_message": "merge commit message",
+  "commit_id": "5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03",
   "committer": "committer",
   "commit_metadata": {
     "key": "value"

diff --git a/esti/hooks_test.go b/esti/hooks_test.go
@@ -194,6 +194,7 @@ func testCommitMerge(t *testing.T, ctx context.Context, repo string) {
 		BranchID:      mainBranch,
 		Committer:     commitRecord.Committer,
 		CommitMessage: fmt.Sprintf("Merge '%s' into '%s'", branch, mainBranch),
+		CommitID:      mergeRef,
 	}, preMergeEvent)
 
 	// Testing post-merge hook response
@@ -212,7 +213,7 @@ func testCommitMerge(t *testing.T, ctx context.Context, repo string) {
 		HookID:        "test_webhook",
 		RepositoryID:  repo,
 		BranchID:      mainBranch,
-		CommitID:      mergeResp.JSON200.Reference,
+		CommitID:      mergeRef,
 		Committer:     commitRecord.Committer,
 		CommitMessage: fmt.Sprintf("Merge '%s' into '%s'", branch, mainBranch),
 	}, postMergeEvent)

diff --git a/pkg/actions/service.go b/pkg/actions/service.go
@@ -539,12 +539,6 @@ func (s *StoreService) PreMergeHook(ctx context.Context, record graveler.HookRec
 }
 
 func (s *StoreService) PostMergeHook(ctx context.Context, record graveler.HookRecord) error {
-	// update pre-merge with commit ID if needed
-	err := s.UpdateCommitID(ctx, record.RepositoryID.String(), record.StorageNamespace.String(), record.PreRunID, record.CommitID.String())
-	if err != nil {
-		return err
-	}
-
 	s.asyncRun(ctx, record)
 	return nil
 }

diff --git a/pkg/graveler/graveler.go b/pkg/graveler/graveler.go
@@ -2777,6 +2777,10 @@ func (g *Graveler) Merge(ctx context.Context, repository *RepositoryRecord, dest
 		}
 		metadata[MergeStrategyMetadataKey] = mergeStrategyString[mergeStrategy]
 		commit.Metadata = metadata
+		commitID, err = g.RefManager.AddCommit(ctx, repository, commit)
+		if err != nil {
+			return nil, fmt.Errorf("add commit: %w", err)
+		}
 		if !repository.ReadOnly {
 			preRunID = g.hooks.NewRunID()
 			err = g.hooks.PreMergeHook(ctx, HookRecord{
@@ -2787,6 +2791,7 @@ func (g *Graveler) Merge(ctx context.Context, repository *RepositoryRecord, dest
 				BranchID:         destination,
 				SourceRef:        fromCommit.CommitID.Ref(),
 				Commit:           commit,
+				CommitID:         commitID,
 			})
 			if err != nil {
 				return nil, &HookAbortError{
@@ -2796,11 +2801,6 @@ func (g *Graveler) Merge(ctx context.Context, repository *RepositoryRecord, dest
 				}
 			}
 		}
-		commitID, err = g.RefManager.AddCommit(ctx, repository, commit)
-		if err != nil {
-			return nil, fmt.Errorf("add commit: %w", err)
-		}
-
 		tokensToDrop = branch.SealedTokens
 		branch.SealedTokens = []StagingToken{}
 		branch.CommitID = commitID

diff --git a/pkg/graveler/graveler_test.go b/pkg/graveler/graveler_test.go
@@ -1872,7 +1872,7 @@ func TestGraveler_PreMergeHook(t *testing.T) {
 				repo = repositoryRO
 			}
 			// call merge
-			_, err := g.Merge(ctx, repo, mergeDestination, expectedCommitID.Ref(), graveler.CommitParams{
+			mergeCommitID, err := g.Merge(ctx, repo, mergeDestination, expectedCommitID.Ref(), graveler.CommitParams{
 				Committer: commitCommitter,
 				Message:   mergeMessage,
 				Metadata:  mergeMetadata,
@@ -1914,6 +1914,9 @@ func TestGraveler_PreMergeHook(t *testing.T) {
 			if h.Commit.Message != mergeMessage {
 				t.Errorf("Hook merge message '%s', expected '%s'", h.Commit.Message, mergeMessage)
 			}
+			if h.CommitID != mergeCommitID {
+				t.Errorf("Hook merge commit ID '%s', expected '%s'", h.CommitID, mergeCommitID)
+			}
 			if diff := deep.Equal(h.Commit.Metadata, expectedMergeMetadata); diff != nil {
 				t.Error("Hook merge metadata diff:", diff)
 			}