refactor(slack): move configuration types into slack package#5239
Open
santiagofn wants to merge 1 commit into
Open
refactor(slack): move configuration types into slack package#5239santiagofn wants to merge 1 commit into
santiagofn wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughExtracts Slack notifier configuration into a new notify/slack package (types, defaults, and validation), updates the Slack notifier implementation and tests to use the new local Config/Field/Action types, and removes the deprecated Slack types from the global config package. ChangesSlack Configuration Module Migration
Sequence Diagram(s) sequenceDiagram
participant Receiver
participant notify_slack_New as notify/slack.New
participant SlackAPI as Slack API
Receiver->>notify_slack_New: passes *slack.Config (receiver.SlackConfigs)
notify_slack_New->>SlackAPI: sends rendered message using Config (APIURL / token)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
santiagofn
force-pushed
the
refactor/slack-move-config
branch
from
cbbb050 to
bbb4c8b
Compare
This was referenced May 19, 2026
santiagofn
force-pushed
the
refactor/slack-move-config
branch
5 times, most recently
from
3fae459 to
f559570
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@notify/slack/config_test.go`:
- Around line 92-137: Update TestSlackAuthMethodConfigValidation to include a
new test case where the YAML input contains "update_message: true" but omits
"api_url" (e.g. in: "`update_message: true\n`"). Use the same table-driven
structure (tests slice) and set expectedErr to the validation error string
produced by Config's strict unmarshal/validation for missing api_url when
update_message is true; run yaml.UnmarshalStrict into a Config and assert that
err is non-nil and matches that expectedErr (ensuring it does not panic).
Reference the existing test function TestSlackAuthMethodConfigValidation, the
Config type, update_message, api_url, and yaml.UnmarshalStrict so the new case
follows the same pattern as other entries.
In `@notify/slack/config.go`:
- Around line 190-191: The validation uses c.APIURL.String() without ensuring
c.APIURL is non-nil which can panic; update the check in the validation (where
c.UpdateMessage is inspected) to first guard c.APIURL (e.g., if c.UpdateMessage
{ if c.APIURL == nil { return errors.New("update_message requires api_url to be
set to https://slack.com/api/chat.postMessage") } if c.APIURL.String() !=
"https://slack.com/api/chat.postMessage" { return errors.New("update_message can
only be used with bot tokens. api_url must be set to
https://slack.com/api/chat.postMessage") } }) so you return a config error
instead of dereferencing a nil pointer (referencing c.UpdateMessage, c.APIURL
and APIURL.String()).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e6baa8e2-ad66-4324-8620-7c045c901d1b
📒 Files selected for processing (8)
config/config.goconfig/notifiers.goconfig/notifiers_test.gonotify/slack/config.gonotify/slack/config_test.gonotify/slack/slack.gonotify/slack/slack_test.gonotify/slack/types.go
💤 Files with no reviewable changes (2)
- config/notifiers_test.go
- config/notifiers.go
santiagofn
force-pushed
the
refactor/slack-move-config
branch
from
f559570 to
b70e44b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/config.go`:
- Around line 414-417: Loop variable sc is being defaulted but not written back
into the slice, leaving nil entries in rcv.SlackConfigs that later cause a panic
in BuildReceiverIntegrations -> notify/slack.New; change the loop that iterates
rcv.SlackConfigs to assign the defaulted value back into the slice (e.g., set
rcv.SlackConfigs[i] = &slack.Config{} when sc == nil) so the slice contains
non-nil *slack.Config elements before they are passed to notify/slack.New.
In `@notify/slack/config_test.go`:
- Around line 238-255: Add explicit length assertions before iterating
element-wise so missing/unmarshalled entries fail the test: check
len(cfg.Fields) == len(expected) (and the analogous slices at the second
location covering lines 302-338) and call t.Fatalf or t.Fatalf-style assertion
when they differ, then proceed with the existing index-based loops; reference
the cfg.Fields and expected slices used in the for index, field := range
cfg.Fields loops to locate both spots to update.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7a186bfb-2f89-4705-8929-ef2597921e38
📒 Files selected for processing (8)
config/config.goconfig/notifiers.goconfig/notifiers_test.gonotify/slack/config.gonotify/slack/config_test.gonotify/slack/slack.gonotify/slack/slack_test.gonotify/slack/types.go
💤 Files with no reviewable changes (2)
- config/notifiers.go
- config/notifiers_test.go
Comment on lines
+238
to
+255
| for index, field := range cfg.Fields { | ||
| exp := expected[index] | ||
| if field.Title != exp.Title { | ||
| t.Errorf("\nexpected:\n%v\ngot:\n%v", exp.Title, field.Title) | ||
| } | ||
| if field.Value != exp.Value { | ||
| t.Errorf("\nexpected:\n%v\ngot:\n%v", exp.Value, field.Value) | ||
| } | ||
| if exp.Short == nil && field.Short != nil { | ||
| t.Errorf("\nexpected:\n%v\ngot:\n%v", exp.Short, *field.Short) | ||
| } | ||
| if exp.Short != nil && field.Short == nil { | ||
| t.Errorf("\nexpected:\n%v\ngot:\n%v", *exp.Short, field.Short) | ||
| } | ||
| if exp.Short != nil && *exp.Short != *field.Short { | ||
| t.Errorf("\nexpected:\n%v\ngot:\n%v", *exp.Short, *field.Short) | ||
| } | ||
| } |
There was a problem hiding this comment.
Assert slice lengths before element-wise checks.
At Line 238 and Line 302, both loops only iterate over parsed output. If unmarshalling drops entries, these tests can pass without detecting missing items.
Proposed fix
func TestFieldConfigUnmarshaling(t *testing.T) {
@@
- for index, field := range cfg.Fields {
+ if len(cfg.Fields) != len(expected) {
+ t.Fatalf("\nexpected:\n%d\ngot:\n%d", len(expected), len(cfg.Fields))
+ }
+ for index, field := range cfg.Fields {
exp := expected[index]
@@
func TestActionsValidation(t *testing.T) {
@@
- for index, action := range cfg.Actions {
+ if len(cfg.Actions) != len(expected) {
+ t.Fatalf("\nexpected:\n%d\ngot:\n%d", len(expected), len(cfg.Actions))
+ }
+ for index, action := range cfg.Actions {
exp := expected[index]Also applies to: 302-338
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@notify/slack/config_test.go` around lines 238 - 255, Add explicit length
assertions before iterating element-wise so missing/unmarshalled entries fail
the test: check len(cfg.Fields) == len(expected) (and the analogous slices at
the second location covering lines 302-338) and call t.Fatalf or t.Fatalf-style
assertion when they differ, then proceed with the existing index-based loops;
reference the cfg.Fields and expected slices used in the for index, field :=
range cfg.Fields loops to locate both spots to update.
Contributor
Author
There was a problem hiding this comment.
this is a pre-existing pattern that comes from config/notifiers_test.go. the risk (silent unmarshal entry-drop) is low for these tests: the inputs are short YAMLs, not user data. so i'll leave it as it is.
There was a problem hiding this comment.
@santiagofn, understood — hardcoded YAML literals carry negligible risk of silent entry-drops, and keeping the pattern consistent with the original config/notifiers_test.go is a reasonable call. Thanks for the context!
(ˆ·ˆ) 🐇
Moves SlackConfig (and the related SlackAction, SlackConfirmationField, SlackField types plus DefaultSlackConfig) from config/notifiers.go into a new notify/slack/config.go, following the same pattern used recently for the mattermost, jira, and incidentio notifiers. While moving them, the Slack prefix is dropped per the package-naming guidance in https://go.dev/blog/package-names ("Avoid repetition"): SlackConfig -> Config, DefaultSlackConfig -> DefaultConfig, SlackAction -> Action, SlackConfirmationField -> ConfirmationField, SlackField -> Field. External callers now refer to slack.Config etc., reading better than the previous slack.SlackConfig stutter. The Slack-specific unmarshal tests are moved alongside them into notify/slack/config_test.go. config.Receiver now refers to *slack.Config, and notify/slack no longer needs to import the top-level config package. No behavioral changes. Signed-off-by: Santiago Fernández Núñez <santiago.nunez@nubank.com.br> Co-authored-by: Cursor <cursoragent@cursor.com>
santiagofn
force-pushed
the
refactor/slack-move-config
branch
from
b70e44b to
14a0060
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
notify/slack/config_test.go (2)
238-255: ⚡ Quick winAssert slice length before element-wise comparison.
The loop iterates over
cfg.Fieldsand accessesexpected[index]. If unmarshalling silently drops entries, the test would pass without detecting missing items.Proposed fix
+ if len(cfg.Fields) != len(expected) { + t.Fatalf("expected %d fields, got %d", len(expected), len(cfg.Fields)) + } for index, field := range cfg.Fields { exp := expected[index]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notify/slack/config_test.go` around lines 238 - 255, The test iterates cfg.Fields and indexes into expected without checking lengths, so missing or dropped entries can be ignored; before the element-wise loop in the test (the block that iterates "for index, field := range cfg.Fields"), add an assertion that len(cfg.Fields) == len(expected) (using t.Fatalf or t.Fatalf-like failure) to fail fast on mismatched slice lengths, and only then proceed with the existing per-index comparisons of cfg.Fields and expected.
302-338: ⚡ Quick winAssert slice length before element-wise comparison.
Same issue as
TestFieldConfigUnmarshaling: if unmarshalling drops action entries, the test would silently pass without detecting the missing items.Proposed fix
+ if len(cfg.Actions) != len(expected) { + t.Fatalf("expected %d actions, got %d", len(expected), len(cfg.Actions)) + } for index, action := range cfg.Actions { exp := expected[index]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notify/slack/config_test.go` around lines 302 - 338, The test iterates cfg.Actions and compares elements to expected but never asserts that len(cfg.Actions) == len(expected), so missing or extra actions would be undetected; add an explicit length assertion before the loop (compare len(cfg.Actions) and len(expected)) in the test that uses cfg.Actions and expected (same pattern as in TestFieldConfigUnmarshaling) so the test fails when unmarshalling drops or adds action entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@notify/slack/config_test.go`:
- Around line 238-255: The test iterates cfg.Fields and indexes into expected
without checking lengths, so missing or dropped entries can be ignored; before
the element-wise loop in the test (the block that iterates "for index, field :=
range cfg.Fields"), add an assertion that len(cfg.Fields) == len(expected)
(using t.Fatalf or t.Fatalf-like failure) to fail fast on mismatched slice
lengths, and only then proceed with the existing per-index comparisons of
cfg.Fields and expected.
- Around line 302-338: The test iterates cfg.Actions and compares elements to
expected but never asserts that len(cfg.Actions) == len(expected), so missing or
extra actions would be undetected; add an explicit length assertion before the
loop (compare len(cfg.Actions) and len(expected)) in the test that uses
cfg.Actions and expected (same pattern as in TestFieldConfigUnmarshaling) so the
test fails when unmarshalling drops or adds action entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b0ebc29b-6dbd-439e-941a-685a02980bf4
📒 Files selected for processing (8)
config/config.goconfig/notifiers.goconfig/notifiers_test.gonotify/slack/config.gonotify/slack/config_test.gonotify/slack/slack.gonotify/slack/slack_test.gonotify/slack/types.go
💤 Files with no reviewable changes (2)
- config/notifiers_test.go
- config/notifiers.go
✅ Files skipped from review due to trivial changes (1)
- notify/slack/slack_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 2 of a series implementing threaded message support for the Slack notifier. Full picture (it might be outdated): #5150.
Summary
Moves
SlackConfigand related types fromconfig/notifiers.gointo a newnotify/slack/config.go, following the same pattern used recently for themattermost,jira, andincidentionotifiers.The Slack-specific unmarshal tests are moved alongside them into
notify/slack/config_test.go.config.Receivernow refers to*slack.Config, andnotify/slackno longer needs to import the top-levelconfigpackage.Naming
While moving the types, the
Slackprefix is dropped from each:SlackConfigConfigSlackActionActionSlackFieldFieldSlackConfirmationFieldConfirmationFieldDefaultSlackConfigDefaultConfigThis follows the Go team's guidance against stuttering identifiers, so call sites read
slack.Configinstead ofslack.SlackConfig:Extra changes driven by CodeRabbit review
Config.UnmarshalYAML: guardc.APIURL == nilbefore callingc.APIURL.String()in theupdate_messagevalidation. Previouslyupdate_message: truewithoutapi_urlpanicked instead of returning the validation error the message string already promised.update_message: truewith noapi_urlinnotify/slack/config_test.go.slack.Configtorcv.SlackConfigs: when a receiver had anilentry inslack_configs(e.g.- ~in YAML),Config.UnmarshalYAMLreboundscto a fresh&slack.Config{}locally but never assigned it back to the slice; the nil entry then flowed intoBuildReceiverIntegrations→slack.New, which dereferencedc.HTTPConfigand panicked. Now the newConfigis written back torcv.SlackConfigs[i].No behavioral changes apart from the panic-to-error fixes.
Made with Cursor
Summary by CodeRabbit