feat(stovepipe): add commit ingestion abstractions#200
Conversation
gsturges
requested review from
a team,
behinddwalls and
sbalabanov
as code owners
|
|
|
|
||
| // PushEvent represents a git push to a branch detected from a webhook event. | ||
| // Fields are immutable after construction. | ||
| type PushEvent struct { |
There was a problem hiding this comment.
do we need to be git aware? @mnoah1
SQ currently is not git aware in general for most part, so my general thought here would be to keep that semantics similar and define abstractions that can allow us to work with git or some other vcs, specific impl could be left to the implementer to decide
There was a problem hiding this comment.
This case seems very close to SubmitQueue's ChangeInfo struct, so maybe we could leverage that:
- Internal extension subscribes to the events and translates them into a shape similar to
ChangeInfo- maybe even reuse the existing entity altogether. - Similarly to the
github.com://schema, add a rawgit://schema that identifies the path to the change on a git remote or other vcs - at this point, it doesn't matter which code review system was used, just where to find the change on remote. - Will work well as an identifier in the db like the existing change table - easy to provide two uri's and compare them.
@behinddwalls does this make sense?
There was a problem hiding this comment.
Yes, this seems fine. We can think about whether to re-use the Change entity...we might have to move it out of SQ domain and into the top level
gsturges
force-pushed
the
gsturges/codem-101-stovepipe-commit-ingestion-abstractions
branch
3 times, most recently
from
64a6c14 to
9bbb8f9
Compare
| // Example: "git://github.com/uber/go-code/refs/heads/main/c3a4d5e6f789..." | ||
| // | ||
| // Fields are immutable after construction. | ||
| type CommitEvent struct { |
There was a problem hiding this comment.
Let's use the ChangeInfo naming from our earlier chat (and can use Change similarly throughout the PR where it says Commit currently).
|
|
||
| "github.com/uber/submitqueue/stovepipe/entity" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Add a New() function to enforce interface compliance here (and you can accept the logger as a param)
| func (LoggingHandler) HandlePushEvent(ctx context.Context, event entity.PushEvent) error { | ||
| // Caller (the Kafka consumer in go-code) is responsible for actual logging. | ||
| // This stub is intentionally a no-op at the OSS layer. | ||
| return nil |
There was a problem hiding this comment.
Add an actual log line here - once we get this wired into the internal service we can deploy and confirm it emits real events.
|
|
||
| // CommitEventHandler processes a single commit event received from the ingester. | ||
| type CommitEventHandler interface { | ||
| HandleCommitEvent(ctx context.Context, event entity.CommitEvent) error |
There was a problem hiding this comment.
I think naming has diverged a bit here - we have HandleCommitEvent, HandlePushEvent. Let's go with something that mirrors our "Change" terminology - how about IngestChange
Introduce ChangeInfo, ChangeIngester/ChangeHandler, a no-op logging handler stub, and URI watch filtering for downstream ingestion.
gsturges
force-pushed
the
gsturges/codem-101-stovepipe-commit-ingestion-abstractions
branch
from
9bbb8f9 to
9bfce3e
Compare
4 participants
Summary
entity.PushEventto represent a branch push (repo, branch, SHAs, pusher) for webhook/Kafka-driven ingestion.CommitIngesterandPushEventHandlerinstovepipe/extensionso a consumer can subscribe to push events and dispatch work.stovepipe/core/filterwithConfig/ShouldProcessto gate processing by watched repo and optional branch list (empty list = all branches for that repo).commitingester.LoggingHandleras a no-opPushEventHandlerstub until persistence/pipeline wiring lands.Test plan
make test(or targeted tests if added in a follow-up)make lintMade with Cursor