Conversation
vigo
commented
Mar 18, 2026
vigo
commented
- remove jsonparser
- seperate migrations
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbbe66c708
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| DROP INDEX IF EXISTS "cauldron"."idx_gitlab_event_created_at"; | ||
| DROP INDEX IF EXISTS "cauldron"."idx_gitlab_created_at"; |
There was a problem hiding this comment.
This split breaks rollback semantics for databases that had the old version 9 already applied. Those installs already created the GitLab created_at indexes in the former 000009_add_more_indexes.up.sql, so 000010_add_indexes_for_gitlab.up.sql becomes a no-op, but this down migration still drops them. After migrate goto 9 or rolling back just this release, version 9 no longer matches the historical schema and GitLab queries lose the date/object_kind indexes unexpectedly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR refactors webhook handlers to remove the jsonparser dependency by switching to encoding/json unmarshalling, and adds dedicated GitLab DB indexes via a new migration.
Changes:
- Add GitLab-specific indexes in a new migration (
000010) and remove GitLab index statements from the GitHub migration (000009). - Update GitHub/GitLab webhook handlers to parse payload fields via
json.Unmarshalinstead ofjsonparser. - Adjust a GitHub webhook handler test to avoid potential goroutine/race issues when reading from the message queue.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| migrations/000010_add_indexes_for_gitlab.up.sql | Adds indexes for GitLab table to optimize created_at and (object_kind, created_at) queries. |
| migrations/000010_add_indexes_for_gitlab.down.sql | Drops the GitLab indexes on rollback. |
| migrations/000009_add_indexes_for_github.up.sql | Removes GitLab index creation from the GitHub index migration. |
| migrations/000009_add_indexes_for_github.down.sql | Removes GitLab index drops from the GitHub rollback migration. |
| internal/transport/http/gitlabwebhookhandler/gitlabwebhookhandler.go | Switches GitLab webhook payload parsing to encoding/json with field fallbacks. |
| internal/transport/http/githubwebhookhandler/githubwebhookhandler.go | Switches GitHub webhook payload parsing to encoding/json for sender fields. |
| internal/transport/http/githubwebhookhandler/githubwebhookhandler_test.go | Makes message-queue assertion deterministic with a timeout-based select. |
| go.mod | Removes github.com/buger/jsonparser dependency. |
| go.sum | Removes jsonparser checksums. |
| .golangci.yml | Disables revive’s nested-structs rule (globally). |
Comments suppressed due to low confidence (1)
migrations/000009_add_indexes_for_github.up.sql:7
- This PR modifies an existing migration (000009) to remove GitLab indexes that previously would have been created at version 9. That means two databases at the same migration version (000009) can now have different schema/index state depending on when they were created/updated, which can be operationally confusing and can affect performance in environments pinned at version 9. Safer approach is to keep 000009 immutable and add 000010 for GitLab indexes (using IF NOT EXISTS for forward-compat), or otherwise create a new migration that explicitly transitions state without changing historical versions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| disabled: true | ||
|
|
| select { | ||
| case msg := <-messageQueue: | ||
| assert.NotNil(t, msg) | ||
| assert.Equal(t, string(msg.Value.(sarama.ByteEncoder)), body) | ||
| }() | ||
|
|
||
| assert.Equal(t, fasthttp.StatusAccepted, ctx.Response.StatusCode()) | ||
| assert.NotEmpty(t, <-messageQueue) | ||
| case <-time.After(time.Second): | ||
| t.Fatal("timed out waiting for message in queue") | ||
| } |
| DROP INDEX IF EXISTS "cauldron"."idx_gitlab_event_created_at"; | ||
| DROP INDEX IF EXISTS "cauldron"."idx_gitlab_created_at"; |
