refactor(sns): move configuration types into sns package#5385
Conversation
📝 WalkthroughWalkthroughSNS configuration ownership moves from ChangesSNS configuration migration
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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/sns/config_test.go`:
- Around line 59-60: Add a test case in the notification configuration
validation tests covering simultaneous target_arn, topic_arn, and phone_number
values. Assert that validation fails for all three targets, guarding the
three-variable XOR behavior while preserving existing cases.
In `@notify/sns/config.go`:
- Around line 61-63: Update the SNS validation in the config validation method
around TargetARN, TopicARN, and PhoneNumber to count the non-empty targets and
return the existing error unless exactly one is provided. Remove the chained
boolean XOR condition and preserve the current error message.
🪄 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: e4e97394-bcaf-44f4-bb7f-4cebab72d509
📒 Files selected for processing (7)
config/config.goconfig/notifiers.goconfig/notifiers_test.gonotify/sns/config.gonotify/sns/config_test.gonotify/sns/sns.gonotify/sns/sns_test.go
💤 Files with no reviewable changes (2)
- config/notifiers_test.go
- config/notifiers.go
| if (c.TargetARN == "") != (c.TopicARN == "") != (c.PhoneNumber == "") { | ||
| return errors.New("must provide either a Target ARN, Topic ARN, or Phone Number for SNS config") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the boolean logic to correctly enforce that exactly one target is provided.
Chaining != operators on booleans acts as an XOR operation. A three-variable XOR (A != B != C) evaluates to false when exactly one variable is false (correct), but it also evaluates to false when all three variables are false.
Therefore, if a user provides target_arn, topic_arn, and phone_number at the same time, this condition will evaluate to false and incorrectly accept the configuration without returning an error.
Replace it with a simple counter for readability and correctness.
🐛 Proposed fix
- if (c.TargetARN == "") != (c.TopicARN == "") != (c.PhoneNumber == "") {
- return errors.New("must provide either a Target ARN, Topic ARN, or Phone Number for SNS config")
+ targets := 0
+ if c.TargetARN != "" {
+ targets++
+ }
+ if c.TopicARN != "" {
+ targets++
+ }
+ if c.PhoneNumber != "" {
+ targets++
+ }
+ if targets != 1 {
+ return errors.New("must provide exactly one of Target ARN, Topic ARN, or Phone Number for SNS config")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (c.TargetARN == "") != (c.TopicARN == "") != (c.PhoneNumber == "") { | |
| return errors.New("must provide either a Target ARN, Topic ARN, or Phone Number for SNS config") | |
| } | |
| targets := 0 | |
| if c.TargetARN != "" { | |
| targets++ | |
| } | |
| if c.TopicARN != "" { | |
| targets++ | |
| } | |
| if c.PhoneNumber != "" { | |
| targets++ | |
| } | |
| if targets != 1 { | |
| return errors.New("must provide exactly one of Target ARN, Topic ARN, or Phone Number for SNS config") | |
| } |
🤖 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/sns/config.go` around lines 61 - 63, Update the SNS validation in the
config validation method around TargetARN, TopicARN, and PhoneNumber to count
the non-empty targets and return the existing error unless exactly one is
provided. Remove the chained boolean XOR condition and preserve the current
error message.
Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Line 989: Restore the exported config.SNSConfig symbol by adding a type alias
to sns.SNSConfig in the config package, while keeping SNSConfigs’ existing field
type and tags unchanged.
🪄 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: 092e6112-609d-4ec9-ac93-5a61a46c371b
📒 Files selected for processing (7)
config/config.goconfig/notifiers.goconfig/notifiers_test.gonotify/sns/config.gonotify/sns/config_test.gonotify/sns/sns.gonotify/sns/sns_test.go
💤 Files with no reviewable changes (2)
- config/notifiers.go
- config/notifiers_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- notify/sns/config.go
- notify/sns/sns.go
- notify/sns/sns_test.go
- notify/sns/config_test.go
| PushoverConfigs []*PushoverConfig `yaml:"pushover_configs,omitempty" json:"pushover_configs,omitempty"` | ||
| VictorOpsConfigs []*VictorOpsConfig `yaml:"victorops_configs,omitempty" json:"victorops_configs,omitempty"` | ||
| SNSConfigs []*SNSConfig `yaml:"sns_configs,omitempty" json:"sns_configs,omitempty"` | ||
| SNSConfigs []*sns.SNSConfig `yaml:"sns_configs,omitempty" json:"sns_configs,omitempty"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files mentioning config.SNSConfig:\n'
rg -n --hidden --glob '!**/vendor/**' '\bconfig\.SNSConfig\b' .
printf '\nDefinitions of SNSConfig in config package:\n'
rg -n --hidden --glob '!**/vendor/**' 'type\s+SNSConfig\b|SNSConfig\s*=' config
printf '\nNearby context in config/config.go around line 989 and any SNSConfig definitions:\n'
python3 - <<'PY'
from pathlib import Path
p = Path('config/config.go')
lines = p.read_text().splitlines()
for start in [930, 970, 980, 990, 1000]:
if start <= len(lines):
end = min(len(lines), start+35)
print(f"\n--- {p}:{start}-{end} ---")
for i in range(start-1, end):
print(f"{i+1:4d}: {lines[i]}")
PYRepository: prometheus/alertmanager
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'SNSConfig declarations/aliases in config package:\n'
rg -n --hidden --glob '!**/vendor/**' 'type\s+SNSConfig\b|SNSConfig\s*=' config
printf '\nReceiver and related SNS snippets from config/config.go:\n'
python3 - <<'PY'
from pathlib import Path
p = Path('config/config.go')
lines = p.read_text().splitlines()
targets = []
for i, line in enumerate(lines, 1):
if 'SNSConfig' in line or 'SNSConfigs' in line:
targets.append(i)
for i in targets[:20]:
start = max(1, i-8)
end = min(len(lines), i+8)
print(f"\n--- {p}:{start}-{end} ---")
for j in range(start, end+1):
print(f"{j:4d}: {lines[j-1]}")
PYRepository: prometheus/alertmanager
Length of output: 211
Preserve the exported config.SNSConfig alias.
This removes the config.SNSConfig symbol and breaks downstream code that still imports config. If config is a supported API, add a compatibility alias such as type SNSConfig = sns.SNSConfig.
🤖 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 `@config/config.go` at line 989, Restore the exported config.SNSConfig symbol
by adding a type alias to sns.SNSConfig in the config package, while keeping
SNSConfigs’ existing field type and tags unchanged.
1 participant
Pull Request Checklist
Which user-facing changes does this PR introduce?