refactor(sns): move configuration types into sns package - #5385
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSNS configuration ownership moves from ChangesSNS configuration migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
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.
Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
1 participant
Pull Request Checklist
Which user-facing changes does this PR introduce?