feat: Add SNS publisher module #463
Conversation
|
""" WalkthroughThis change introduces support for AWS SNS publishing in the CLI tooling. It adds CLI flags to enable SNS publishing and configure endpoint overrides, updates configuration structures, and integrates a new SNS publisher module using Uber Fx for dependency injection. The SNS module handles credentials, endpoint overrides, and resource cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant FX
participant SNSPublisher
participant AWS
User->>CLI: Run command with --publisher-sns-enabled
CLI->>FX: Initialize modules with SNS options
FX->>SNSPublisher: Construct with config, endpoint, credentials
SNSPublisher->>AWS: Load config, resolve endpoint/credentials
SNSPublisher->>SNSPublisher: Create publisher, manage lifecycle
SNSPublisher-->>FX: Provide publisher instance
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
laouji
force-pushed
the
BAN-13
branch
2 times, most recently
from
bdb968d to
ecc659a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #463 +/- ##
==========================================
- Coverage 33.55% 33.07% -0.49%
==========================================
Files 130 131 +1
Lines 5507 5587 +80
==========================================
Hits 1848 1848
- Misses 3550 3630 +80
Partials 109 109 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
publish/sns.go (1)
26-29: Use a constant for the default account ID.The hardcoded account ID should be defined as a constant for better maintainability and clarity.
+const defaultDevAccountID = "000000000000" + func NewSnsPublisher(cmd *cobra.Command, logger watermill.LoggerAdapter, config aws.Config, optFns []func(*snsservice.Options)) (*sns.Publisher, error) { credentials, err := config.Credentials.Retrieve(cmd.Context()) if err != nil { return nil, fmt.Errorf("failed to fetch credentials: %w", err) } - accountID := "000000000000" // if we are using a static credentials provider in a dev env, it may be empty + accountID := defaultDevAccountID // if we are using a static credentials provider in a dev env, it may be empty if credentials.AccountID != "" { accountID = credentials.AccountID }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (2)
publish/cli.go(6 hunks)publish/sns.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
publish/sns.go (1)
service/context.go (1)
IsDebug(54-60)
🪛 GitHub Check: codecov/patch
publish/sns.go
[warning] 21-29: publish/sns.go#L21-L29
Added lines #L21 - L29 were not covered by tests
[warning] 31-34: publish/sns.go#L31-L34
Added lines #L31 - L34 were not covered by tests
[warning] 36-41: publish/sns.go#L36-L41
Added lines #L36 - L41 were not covered by tests
[warning] 44-55: publish/sns.go#L44-L55
Added lines #L44 - L55 were not covered by tests
[warning] 59-64: publish/sns.go#L59-L64
Added lines #L59 - L64 were not covered by tests
[warning] 68-72: publish/sns.go#L68-L72
Added lines #L68 - L72 were not covered by tests
[warning] 74-83: publish/sns.go#L74-L83
Added lines #L74 - L83 were not covered by tests
[warning] 87-95: publish/sns.go#L87-L95
Added lines #L87 - L95 were not covered by tests
[warning] 97-97: publish/sns.go#L97
Added line #L97 was not covered by tests
[warning] 100-102: publish/sns.go#L100-L102
Added lines #L100 - L102 were not covered by tests
publish/cli.go
[warning] 157-160: publish/cli.go#L157-L160
Added lines #L157 - L160 were not covered by tests
[warning] 221-221: publish/cli.go#L221
Added line #L221 was not covered by tests
[warning] 247-254: publish/cli.go#L247-L254
Added lines #L247 - L254 were not covered by tests
🔇 Additional comments (2)
publish/cli.go (1)
157-160: Add test coverage for the new SNS flags.The static analysis indicates these lines lack test coverage. Please add tests to verify that the SNS flags are properly registered and can be retrieved from the command.
publish/sns.go (1)
44-104: Well-implemented Fx module with proper lifecycle management.The SNS module implementation is well-structured with:
- Appropriate handling of development environments via endpoint overrides
- Proper lifecycle management with cleanup on shutdown
- Good separation of concerns across providers
However, the static analysis indicates this entire module lacks test coverage. Please add comprehensive tests to verify the module behavior in both production and development scenarios.
publish/cli.go
Outdated
| fallthrough // we might have both a sqs listener and a sns publisher | ||
| case snsEnabled: | ||
| snsEndpointOverride, _ := cmd.Flags().GetString(PublisherSnsEndpointOverrideFlag) | ||
|
|
||
| options = append(options, | ||
| fx.Supply(fx.Annotate(iam.LoadOptionFromCommand(cmd), fx.ResultTags(`name:"publish-sns-enabled"`))), | ||
| snsModule(cmd, snsEndpointOverride), | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify the fallthrough pattern and potential duplicate IAM loading.
The fallthrough from the SQS case to the SNS case allows both modules to be enabled simultaneously. This design has potential issues:
- When both SQS and SNS are enabled, the IAM loading options will be supplied twice with potentially conflicting tag names (
name:"publish-sqs-enabled"andname:"publish-sns-enabled"). - The comment "we might have both a sqs listener and a sns publisher" suggests this is intentional, but the implementation could cause issues with dependency injection.
Consider separating the cases or using a different approach to handle when both are enabled.
Additionally, please add test coverage for this configuration logic.
🏁 Script executed:
#!/bin/bash
# Description: Check if there are any existing tests for the FXModuleFromFlags function
# and verify how other messaging systems handle multiple enabled flags
# Search for existing tests of FXModuleFromFlags
rg -A 10 "TestFXModuleFromFlags|Test.*FromFlags" --glob "**/*test*.go"
# Check how IAM loading is used in other parts of the codebase
ast-grep --pattern 'iam.LoadOptionFromCommand($$$)'
# Look for other uses of fx.Supply with tag annotations
rg -A 2 'fx\.Supply.*fx\.Annotate.*fx\.ResultTags'Length of output: 1279
Publish CLI: clarify fallthrough logic and add missing tests
The fallthrough from the SQS case into the SNS case is intentional (per the inline comment), and because we annotate the IAM options with distinct tags (publish-sqs-enabled vs. publish-sns-enabled), there’s no collision at runtime. However, this pattern can be hard to read and isn’t covered by existing tests. Please:
- Refactor the switch/fallthrough in publish/cli.go into two independent
ifblocks (one for SQS, one for SNS) to make the intent clearer. - Verify that each call to
iam.LoadOptionFromCommand(cmd)uses a uniquefx.ResultTagsvalue to avoid any ambiguity. - Add unit tests for
FXModuleFromFlagscovering:- SQS-only enabled
- SNS-only enabled
- Both SQS and SNS enabled
Tests can live alongside the existing TestFXModuleFromFlags in licence/cli_test.go or in a new publish/cli_test.go.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 247-254: publish/cli.go#L247-L254
Added lines #L247 - L254 were not covered by tests
🤖 Prompt for AI Agents
In publish/cli.go around lines 247 to 254, refactor the switch statement with
fallthrough into two separate if blocks, one checking if SQS is enabled and the
other if SNS is enabled, to improve code clarity. Ensure each call to
iam.LoadOptionFromCommand(cmd) is annotated with a unique fx.ResultTags value to
prevent runtime collisions. Then, add unit tests for the FXModuleFromFlags
function covering three scenarios: SQS-only enabled, SNS-only enabled, and both
enabled. Place these tests either in licence/cli_test.go alongside
TestFXModuleFromFlags or create a new publish/cli_test.go file.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
publish/cli.go
Outdated
| @@ -232,6 +244,14 @@ func FXModuleFromFlags(cmd *cobra.Command, debug bool) fx.Option { | |||
| fx.Supply(fx.Annotate(iam.LoadOptionFromCommand(cmd), fx.ResultTags(`name:"publish-sqs-enabled"`))), | |||
| sqsModule(cmd, sqsEndpointOverride), | |||
| ) | |||
| fallthrough // we might have both a sqs listener and a sns publisher | |||
There was a problem hiding this comment.
we can have SQS listener & publisher, likewise for SNS right?
Also I might be wrong, but I think the fallthrough will execute even if the condition is false -- so if you have sqsEnabled, you'll always setup sns -- is it what we want?
There was a problem hiding this comment.
yes cursor actually caught the same bug. I guess I need to come up with something a bit more robust so that it's possible to specify just SQS or SQS + SNS
There was a problem hiding this comment.
Please see my most recent commit!
Fixes: BAN-13