[webhooks] implement webhooks management commands #13
Conversation
WalkthroughThis pull request introduces modifications to the SMS gateway CLI application, primarily focusing on webhook functionality. Changes include adding commands for managing webhooks (registering, listing, and deleting), updating the dependency for Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Application
participant Client as Webhook Client
participant Renderer as Output Renderer
CLI->>Client: RegisterWebhook(context, webhook)
Client-->>CLI: Webhook Registration Response
CLI->>Renderer: Render Webhook
Renderer-->>CLI: Formatted Output
CLI->>CLI: Display Result
sequenceDiagram
participant CLI as CLI Application
participant Client as Webhook Client
participant Renderer as Output Renderer
CLI->>Client: ListWebhooks(context)
Client-->>CLI: List of Webhooks
CLI->>Renderer: Render Webhooks
Renderer-->>CLI: Formatted Webhook List
CLI->>CLI: Display Results
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
go.mod (1)
Line range hint
3-3: Fix invalid Go version.
go 1.23.2is not a valid version as Go 1.23 hasn't been released yet. The latest stable version is Go 1.22.-go 1.23.2 +go 1.22
🧹 Nitpick comments (6)
internal/commands/webhooks/list.go (1)
25-29: Consider handling empty webhook list case.While the error handling is good, consider adding specific handling for an empty webhook list to improve user experience.
res, err := client.ListWebhooks(c.Context) if err != nil { return cli.Exit(err.Error(), codes.ClientError) } +if len(res) == 0 { + fmt.Println("No webhooks found") + return nil +}internal/core/output/json.go (1)
37-39: Consider returning a structured success response.The
Success()method returns an empty string, which might be confusing for JSON output. Consider returning a structured success message.func (o *JSONOutput) Success() (string, error) { - return "", nil + return o.marshaler(struct { + Status string `json:"status"` + }{ + Status: "success", + }) }internal/commands/webhooks/delete.go (2)
Line range hint
17-22: Enhance ID validation.While basic empty check is present, consider adding more robust ID validation.
id := c.Args().Get(0) if id == "" { return cli.Exit("ID is empty", codes.ParamsError) } +// Add UUID validation if IDs are UUIDs +if !isValidUUID(id) { + return cli.Exit("Invalid ID format", codes.ParamsError) +}🧰 Tools
🪛 GitHub Check: build
[failure] 27-27:
client.DeleteWebhook undefined (type *smsgateway.Client has no field or method DeleteWebhook)
32-37: Consider adding user confirmation for deletion.Add a confirmation prompt before deleting the webhook to prevent accidental deletions.
+if !c.Bool("force") { + confirmed, err := promptForConfirmation(fmt.Sprintf("Are you sure you want to delete webhook %s?", id)) + if err != nil { + return cli.Exit(err.Error(), codes.OutputError) + } + if !confirmed { + return nil + } +} err := client.DeleteWebhook(c.Context, id)Also, consider adding a
--forceflag to skip confirmation:var delete = &cli.Command{ // ... existing fields ... + Flags: []cli.Flag{ + &cli.BoolFlag{ + Name: "force", + Aliases: []string{"f"}, + Usage: "Skip deletion confirmation", + }, + }, Action: func(c *cli.Context) error {internal/core/output/text.go (1)
84-105: Consider pre-allocating string builder capacity.For better performance, consider pre-allocating the string builder capacity based on the number of webhooks and average webhook string length.
func (o *TextOutput) Webhooks(src []smsgateway.Webhook) (string, error) { builder := strings.Builder{} + // Pre-allocate capacity assuming average webhook string length of 100 chars + // plus separators (5 chars each) + builder.Grow(len(src) * (100 + 5)).gitignore (1)
128-128: Consolidate duplicate Go workspace entries.There's already an entry for
go.workon line 47. Consider consolidating these entries by updating the existing one to use the wildcard pattern for better maintainability.Apply this diff to consolidate the entries:
# Go workspace file -go.work +go.work* # Custom rules (everything added below won't be overriden by 'Generate .gitignore File' if you use 'Update' option) -go.work*
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.gitignore(1 hunks)cmd/smsgate/smsgate.go(1 hunks)go.mod(1 hunks)internal/commands/webhooks/delete.go(2 hunks)internal/commands/webhooks/list.go(2 hunks)internal/commands/webhooks/register.go(3 hunks)internal/core/output/json.go(1 hunks)internal/core/output/output.go(1 hunks)internal/core/output/text.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: build
internal/commands/webhooks/list.go
[failure] 20-20:
client.ListWebhooks undefined (type *smsgateway.Client has no field or method ListWebhooks)
internal/commands/webhooks/delete.go
[failure] 27-27:
client.DeleteWebhook undefined (type *smsgateway.Client has no field or method DeleteWebhook)
internal/commands/webhooks/register.go
[failure] 30-30:
undefined: smsgateway.WebhookEventTypes
[failure] 58-58:
client.RegisterWebhook undefined (type *smsgateway.Client has no field or method RegisterWebhook)
🪛 GitHub Actions: test
internal/commands/webhooks/register.go
[error] 30-30: undefined: smsgateway.WebhookEventTypes
🔇 Additional comments (7)
internal/core/output/output.go (1)
19-21: LGTM! Well-structured interface extensions.The new webhook-related methods follow consistent patterns with existing interface methods, maintaining good API design principles.
internal/core/output/json.go (1)
41-41: LGTM! Good practice with interface verification.The interface implementation verification using
var _ Renderer = (*JSONOutput)(nil)is a good practice to catch any interface implementation issues at compile time.go.mod (1)
7-7: Consider using a released version of go-helpers.The dependency is currently pinned to a specific commit. Consider waiting for a stable release or vendoring the code if the features are essential.
cmd/smsgate/smsgate.go (1)
94-94: LGTM!Good practice to pre-allocate the commands slice capacity and maintain proper error handling.
internal/core/output/text.go (1)
70-82: LGTM!Well-documented implementation with proper string builder usage and error handling.
internal/commands/webhooks/register.go (2)
Line range hint
34-40: LGTM: Event validation implementation.The event validation is well implemented with proper error handling and user feedback.
🧰 Tools
🪛 GitHub Check: build
[failure] 30-30:
undefined: smsgateway.WebhookEventTypes🪛 GitHub Actions: test
[error] 30-30: undefined: smsgateway.WebhookEventTypes
52-56: LGTM: Request structure.The webhook request structure is properly initialized with all required fields.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/core/output/text.go (3)
70-82: LGTM! Consider adding input validation.The implementation is clean and efficient, using
strings.Builderfor performance.Consider adding validation for empty or invalid webhook fields:
func (*TextOutput) Webhook(src smsgateway.Webhook) (string, error) { + if src.ID == "" || src.Event == "" || src.URL == "" { + return "", fmt.Errorf("invalid webhook: missing required fields") + } builder := strings.Builder{} builder.WriteString("ID: ")
84-109: Consider defining the separator as a constant.The implementation looks good with proper error handling and empty slice handling.
Consider defining the separator as a constant for better maintainability:
package output import ( "strings" "time" "github.com/android-sms-gateway/client-go/smsgateway" ) +const webhookSeparator = "\n---\n" // ... rest of the code ... func (o *TextOutput) Webhooks(src []smsgateway.Webhook) (string, error) { // ... rest of the code ... - builder.WriteString("\n---\n") + builder.WriteString(webhookSeparator) // ... rest of the code ... }
111-114: Consider making success messages more specific and consistent.While the implementation is simple and clear, it could be improved for better maintainability and user experience.
Consider these improvements:
- Define success messages as constants
- Make messages more specific to the operation
package output +const ( + successMessageGeneric = "Success" + successMessageWebhook = "Webhook operation completed successfully" +) // ... rest of the code ... -func (*TextOutput) Success() (string, error) { - return "Success", nil +func (*TextOutput) Success(operation ...string) (string, error) { + if len(operation) > 0 && operation[0] == "webhook" { + return successMessageWebhook, nil + } + return successMessageGeneric, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(1 hunks)internal/core/output/text.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
internal/core/output/text.go (1)
116-116: Great use of compile-time interface check!The blank assignment ensures that
TextOutputimplements theRendererinterface at compile time.
capcom6
force-pushed
the
webhooks/add-commands
branch
from
34d9483 to
e545dae
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/commands/webhooks/register.go (1)
Line range hint
44-48: Consider adding URL format validation.While the code checks for empty URL, it would be beneficial to validate the URL format to ensure it's a valid HTTP/HTTPS URL before making the API call.
Here's a suggested implementation:
url := c.Args().Get(0) if url == "" { return cli.Exit("URL is empty", codes.ParamsError) } +if !strings.HasPrefix(url, "http://") && !strings.HasPrefix(url, "https://") { + return cli.Exit("URL must start with http:// or https://", codes.ParamsError) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
.gitignore(1 hunks)README.md(2 hunks)cmd/smsgate/smsgate.go(1 hunks)go.mod(1 hunks)internal/commands/webhooks/delete.go(2 hunks)internal/commands/webhooks/list.go(2 hunks)internal/commands/webhooks/register.go(3 hunks)internal/core/output/json.go(1 hunks)internal/core/output/output.go(1 hunks)internal/core/output/text.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- .gitignore
- internal/commands/webhooks/list.go
- internal/core/output/output.go
- cmd/smsgate/smsgate.go
- go.mod
- README.md
- internal/core/output/text.go
- internal/core/output/json.go
🔇 Additional comments (7)
internal/commands/webhooks/register.go (3)
4-11: LGTM! Imports are appropriate for the functionality.The imports include all necessary packages for string manipulation, CLI functionality, and webhook management.
Line range hint
23-40: Verify the client version supports these methods.The code uses
WebhookEventTypes()andIsValidWebhookEvent()methods which were previously undefined. Please ensure you're using the latest version ofgithub.com/android-sms-gateway/client-gothat includes these methods.#!/bin/bash # Description: Check the client version and available methods # Check the imported version cat go.mod | grep "github.com/android-sms-gateway/client-go" # Check if the methods exist in the client package rg -A 5 "func (.*WebhookEventTypes|func (.*IsValidWebhookEvent"
49-69: Verify the client version supports RegisterWebhook method.The implementation looks good with proper error handling for both API calls and response rendering. However, please ensure you're using the latest version of the client that includes the
RegisterWebhookmethod.#!/bin/bash # Description: Check if RegisterWebhook method exists in the client # Check if the method exists in the client package rg -A 5 "func \(.*\)RegisterWebhook"internal/commands/webhooks/delete.go (4)
4-7: LGTM!The added imports are necessary and properly used in the code.
24-25: LGTM!The client and renderer are correctly initialized from the app metadata.
32-37: LGTM!The success handling is well-implemented with proper error handling for the renderer and correct output printing.
27-30: Verify that the build error is resolved.The error handling is correct, but there was a previous build error indicating that
DeleteWebhookwas undefined. Let's verify that this has been resolved.Run the following script to check if the method exists in the client package:
✅ Verification successful
Build error is resolved - DeleteWebhook is provided by external client package
The
DeleteWebhookmethod is correctly implemented in the externalgithub.com/android-sms-gateway/client-go/smsgatewaypackage, and the client is properly initialized ininternal/core/client/client.go. The code is working as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if DeleteWebhook method is implemented in the client package. # Search for DeleteWebhook method definition ast-grep --pattern $'func \($_\) DeleteWebhook($_, $_) $_' # Search for DeleteWebhook interface definition ast-grep --pattern $'interface { $$$ DeleteWebhook($_, $_) $_ $$$ }'Length of output: 135
Script:
#!/bin/bash # Search for any occurrence of DeleteWebhook with context rg -l "DeleteWebhook" --type go # Show the content of files containing DeleteWebhook rg "DeleteWebhook" -B 2 -A 2 --type goLength of output: 458
Script:
#!/bin/bash # Find files in client package fd -t f "^.*\.go$" "client" # Search for webhook-related methods in go files rg "func.*Webhook" -B 1 -A 3 --type goLength of output: 1729
Script:
#!/bin/bash # Search for smsgateway imports and package declarations rg "smsgateway" -B 2 -A 2 --type go # Look for delete-related methods or interfaces rg "Delete.*Webhook|RemoveWebhook" -B 2 -A 2 --type goLength of output: 7435
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/go.yml (3)
1-8: Consider optimizing workflow triggers and branch naming.
- Consider using
maininstead ofmasteras the default branch name to align with modern Git practices.- Add path filters to optimize workflow execution by running only when Go files are modified.
on: push: branches: [master] + paths: + - '**.go' + - 'go.mod' + - 'go.sum' pull_request: branches: [master] + paths: + - '**.go' + - 'go.mod' + - 'go.sum'
10-30: Enhance linting job performance and configuration.Consider the following improvements:
- Add caching to speed up builds
- Configure golangci-lint settings file
golangci: name: Lint runs-on: ubuntu-latest steps: - name: Checkout code into workspace directory uses: actions/checkout@v4 - name: Set up Go uses: actions/setup-go@v5 with: go-version: stable + cache: true - name: Run golangci-lint uses: golangci/golangci-lint-action@v6 with: version: latest args: --timeout=5m + config-path: .golangci.yml
31-51: Enhance test job configuration and coverage reporting.Consider the following improvements:
- Add caching to speed up builds
- Add coverage report upload
- Configure test timeout and output formatting
test: name: Test runs-on: ubuntu-latest steps: - name: Checkout code into workspace directory uses: actions/checkout@v4 - name: Set up Go uses: actions/setup-go@v5 with: go-version: stable + cache: true - name: Install all Go dependencies run: go mod download - name: Run coverage - run: go test -race -coverprofile=coverage.out -covermode=atomic ./... + run: | + go test -race -json -timeout=10m -coverprofile=coverage.out -covermode=atomic ./... | tee test-report.json + + - name: Upload coverage reports + uses: codecov/codecov-action@v4 + with: + file: ./coverage.out + fail_ci_if_error: true + token: ${{ secrets.CODECOV_TOKEN }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/go.yml(1 hunks).github/workflows/test.yml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/test.yml
capcom6
force-pushed
the
webhooks/add-commands
branch
from
7414ce6 to
ea7e3d0
Compare
Summary by CodeRabbit
Release Notes
New Features
Improvements
Dependencies
github.com/android-sms-gateway/client-goto a newer versiongithub.com/capcom6/go-helpersOther Changes
.gitignoreto exclude Go workspace files