feat: hook pre/post merge include merge source #8703
Conversation
The new field `merge_source` include source branch/tag/commit as requested. The current event include `source_ref` which resolved to the specific commit the merge operation will merge.
nopcoder
added
area/hooks
|
♻️ PR Preview 6c09f27 has been successfully destroyed since this PR has been closed.
🤖 By surge-preview |
E2E Test Results - DynamoDB Local - Local Block Adapter |
E2E Test Results - Quickstart |
docs/howto/hooks/webhooks.md
Outdated
| | tag_id[^3] | The ID of the created/deleted tag | string | | ||
| | merge_source[^5] | The source branch/tag/ref on merge operation | string | |
There was a problem hiding this comment.
This renders as 4? (see image with purple arrow)
There was a problem hiding this comment.
Fixed this issue in the current docs - seems you can't compose
| @@ -37,6 +38,7 @@ func marshalEventInformation(actionName, hookID string, record graveler.HookReco | |||
| CommitMessage: record.Commit.Message, | |||
| Committer: record.Commit.Committer, | |||
| CommitMetadata: record.Commit.Metadata, | |||
| MergeSource: record.MergeSource.String(), | |||
There was a problem hiding this comment.
Is this left out from lua type on purpose? (https://github.com/treeverse/lakeFS/blob/master/pkg/actions/lua.go#L47)
There was a problem hiding this comment.
No, It is a bug - will fix it
| @@ -20,6 +20,7 @@ type EventInfo struct { | |||
| CommitMessage string `json:"commit_message,omitempty"` | |||
| Committer string `json:"committer,omitempty"` | |||
| CommitMetadata map[string]string `json:"commit_metadata,omitempty"` | |||
| MergeSource string `json:"merge_source,omitempty"` | |||
There was a problem hiding this comment.
I was having trouble understanding the difference between MergeSource and SourceRef. I think documenting these fields will greatly improve readability
There was a problem hiding this comment.
Documented in the HookRecord struct and part of the invoke
There was a problem hiding this comment.
I think it's better to document it on the struct itself rather than where it is being used
There was a problem hiding this comment.
HookRecord is the structure that is used when we trigger hooks. The event is used for marshal the data for webhook and airflow.
There was a problem hiding this comment.
Sorry I thought you meant on the usage
pkg/graveler/graveler.go
Outdated
| RunID: postRunID, | ||
| Repository: repository, | ||
| BranchID: destination, | ||
| MergeSource: source, |
There was a problem hiding this comment.
nit: align field location in both pre and post hooks
| CommitID: "123456789", | ||
| PreRunID: "3498032432", | ||
| TagID: "tag1", | ||
| MergeSource: "merge-source", |
There was a problem hiding this comment.
Do we have any place in the test where we verify the field? If not - what is the point of adding it?
There was a problem hiding this comment.
the test uses a golden file which I updated
The new field
merge_sourceinclude source branch/tag/commit as requested.The current event include
source_refwhich resolved to the specific commit the merge operation will merge.Close #8707