webhook: custom payloads allow any type#5130
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR changes webhook payload handling by making Changes
Sequence DiagramsequenceDiagram
participant Webhook as Webhook Notifier
participant Config as Payload Config
participant Template as Template Engine
participant HTTP as HTTP Client
Webhook->>Config: Read Payload (any)
activate Config
Config-->>Webhook: Return payload object
deactivate Config
Webhook->>Template: DeepCopyWithTemplate(payload)
activate Template
Template->>Template: Recursively render & template values
Template-->>Webhook: Return rendered payload
deactivate Template
Webhook->>Webhook: Marshal rendered payload to JSON
Webhook->>HTTP: POST JSON payload
activate HTTP
HTTP-->>Webhook: Response
deactivate HTTP
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@notify/webhook/webhook_test.go`:
- Around line 375-380: The YAML unmarshal in the test currently ignores the
error; change the call to capture the returned error and assert it with
require.NoError(t, err) so test fixtures failing to parse fail the test. Locate
the yaml.Unmarshal call that fills the payload variable
(yaml.Unmarshal([]byte(...), &payload)), store its error (err :=
yaml.Unmarshal(...)) and replace the ignored result with require.NoError(t, err)
using the existing testing variable t.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34bb2cfb-be7e-469f-87cb-010860cfcb80
📒 Files selected for processing (3)
config/notifiers.gonotify/webhook/webhook.gonotify/webhook/webhook_test.go
1407602 to
fbc6fae
Compare
do not require payload top level to be a map deepCopyWithTemplate can handle any type, so we can allow users to specify any type of payload. This allows for more flexibility in the payload structure. Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
fbc6fae to
8375938
Compare
do not require payload top level to be a map
deepCopyWithTemplate can handle any type, so we
can allow users to specify any type of payload.
This allows for more flexibility in the payload
structure.
I have set CHANGELOG to
NONEwhich is true if this goes in before the next release as this updates #5011 which is not released yet.Pull Request Checklist
Please check all the applicable boxes.
Which user-facing changes does this PR introduce?
Summary by CodeRabbit
New Features
Tests