feat: add --tags flag to start and stop commands #104
Conversation
Add the ability to start and stop multiple templates by tag filter. Changes: - Add GetByTags() function to retrieve templates matching given tags - Add --tags/-t flag to start command for starting multiple templates - Add --tags/-t flag to stop command for stopping multiple templates - Tags support case-insensitive and substring matching - --id and --tags are mutually exclusive - Continue on failure and report failed templates at end Usage: vt start --tags sqli,xss # Start all templates with sqli OR xss tags vt stop --tags web # Stop all templates with web tag Closes #101
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds tag-based selection to the start and stop CLI commands and a new template lookup by tags in the template package, with tests for tag matching and adjustments to per-template start/stop control flow and logging. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (start/stop)"
participant TPL as "Template pkg (GetByTags / GetByID)"
participant Provider as "Provider (Start/Stop)"
CLI->>TPL: parse flags (--id or --tags)
alt id provided
CLI->>TPL: GetByID(id)
TPL-->>CLI: template
CLI->>Provider: lookup provider for template
Provider-->>CLI: provider instance
CLI->>Provider: Start/Stop(template)
Provider-->>CLI: result
else tags provided
CLI->>TPL: GetByTags(tags)
TPL-->>CLI: []*templates
loop for each template
CLI->>Provider: lookup provider for template
Provider-->>CLI: provider instance
CLI->>Provider: Start/Stop(template)
Provider-->>CLI: result
CLI->>CLI: log per-template post-install / errors
end
CLI-->>CLI: aggregate and log summary of failures
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/template/template.go (1)
223-238: Minor: Redundant exact-match check.The
strings.EqualFoldcheck on line 231 is redundant because the subsequentstrings.Contains(strings.ToLower(...), strings.ToLower(...))on line 232 already covers exact matches. However, the explicit check may improve readability by showing intent, so this is purely optional.🔎 Optional simplification
func templateMatchesTags(tmpl *Template, filterTags []string) bool { for _, filterTag := range filterTags { filterTag = strings.TrimSpace(filterTag) if filterTag == "" { continue } for _, templateTag := range tmpl.Info.Tags { - if strings.EqualFold(templateTag, filterTag) || - strings.Contains(strings.ToLower(templateTag), strings.ToLower(filterTag)) { + if strings.Contains(strings.ToLower(templateTag), strings.ToLower(filterTag)) { return true } } } return false }internal/cli/stop.go (1)
66-83: Consider exiting with non-zero status on partial failures.When some templates fail to stop, the command logs a warning but exits successfully. For CI/scripting scenarios, a non-zero exit code when
len(failed) > 0would better signal partial failure.🔎 Proposed change
if len(failed) > 0 { log.Warn().Msgf("failed to stop %d templates: %s", len(failed), strings.Join(failed, ", ")) + log.Fatal().Msgf("completed with %d failures", len(failed)) } if stopped > 0 { log.Info().Msgf("successfully stopped %d templates", stopped) }Alternatively, use
os.Exit(1)after logging to allow the success message to print first.internal/cli/start.go (2)
79-81: Consider exiting with non-zero status on partial failures.Same concern as in
stop.go: when some templates fail to start, the command logs a warning but exits with status 0. This may cause issues in CI/automation scenarios.
99-114: Inconsistent post-install log message format.In
startTemplate(line 107), the log says"Post-installation instructions:"without the template ID, while in the tags-based loop (line 70), it includes the template ID:"Post-installation instructions for %s:". Consider aligning these for consistency.🔎 Proposed fix
func (c *CLI) startTemplate(provider interface{ Start(*tmpl.Template) error }, template *tmpl.Template, providerName string) { err := provider.Start(template) if err != nil { log.Fatal().Msgf("%v", err) } if len(template.PostInstall) > 0 { - log.Info().Msg("Post-installation instructions:") + log.Info().Msgf("Post-installation instructions for %s:", template.ID) for _, instruction := range template.PostInstall { fmt.Printf(" %s\n", instruction) } } log.Info().Msgf("%s template is running on %s", template.ID, providerName) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/cli/start.gointernal/cli/stop.gopkg/template/template.gopkg/template/template_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/cli/start.go (3)
pkg/provider/registry/registry.go (1)
GetProvider(19-22)pkg/template/template.go (4)
GetByID(192-198)GetByTags(203-221)Info(27-39)Template(17-24)internal/cli/cli.go (1)
CLI(29-32)
internal/cli/stop.go (2)
pkg/provider/registry/registry.go (1)
GetProvider(19-22)pkg/template/template.go (3)
GetByID(192-198)Info(27-39)GetByTags(203-221)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
pkg/template/template.go (1)
199-221: LGTM! Well-structured tag-based retrieval.The implementation correctly:
- Validates for empty tags input
- Creates copies to avoid pointer aliasing issues when iterating over map values
- Returns a clear error when no templates match
pkg/template/template_test.go (2)
34-101: Comprehensive test coverage forGetByTags.The test cases cover all documented behaviors:
- Single and multiple tag matching (OR logic)
- Case-insensitive matching
- Substring matching
- Error cases (no matches, empty tags)
- Whitespace handling
103-128: Good unit tests fortemplateMatchesTags.The helper function is well tested with edge cases including empty and whitespace-only filter tags.
internal/cli/stop.go (2)
28-38: LGTM! Clear mutual exclusivity validation.The validation correctly enforces that exactly one of
--idor--tagsmust be provided.
95-96: LGTM! Flag definition is consistent with start command.internal/cli/start.go (2)
28-38: LGTM! Consistent with stop command validation.
100-100: The current inline interface is the better approach. The function only uses theStartmethod, and the existingProviderinterface inpkg/provider/provider.goincludes four methods (Name(),Start(),Stop(),Status()). Using the fullProviderinterface would violate the Interface Segregation Principle by coupling the function to methods it doesn't use. The inline interface is cleaner and makes testing/mocking easier.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/cli/stop.go`:
- Around line 58-64: The current splitting of tagsStr into tags can produce
empty or whitespace tokens that will match everything; instead, after splitting
tagsStr, trim each token and filter out empty strings to produce a cleaned tags
slice, then if the cleaned slice is empty fail with an error (or log.Fatal)
rather than calling tmpl.GetByTags; call tmpl.GetByTags(c.app.Templates,
cleanedTags) and update the log message to report cleanedTags (referencing
tagsStr, tags, tmpl.GetByTags and c.app.Templates).
🧹 Nitpick comments (2)
pkg/template/template_test.go (1)
36-103: Strengthen multi-match assertions to avoid false positives.Right now the tests only check
Lenfor the multi-tag cases, so duplicate or wrong matches could still pass. Consider asserting the exact set of IDs.♻️ Proposed test tightening
// Test multiple tags (OR logic) matched, err = GetByTags(templates, []string{"sqli", "xss"}) assert.NoError(t, err) assert.Len(t, matched, 2) + assert.ElementsMatch(t, + []string{"sqli-template", "xss-template"}, + []string{matched[0].ID, matched[1].ID}, + ) // Test tag that matches multiple templates matched, err = GetByTags(templates, []string{"web"}) assert.NoError(t, err) assert.Len(t, matched, 3) + assert.ElementsMatch(t, + []string{"sqli-template", "xss-template", "ssrf-template"}, + []string{matched[0].ID, matched[1].ID, matched[2].ID}, + ) // Test substring matching matched, err = GetByTags(templates, []string{"owa"}) assert.NoError(t, err) assert.Len(t, matched, 2) // sqli-template and xss-template have "owasp" + assert.ElementsMatch(t, + []string{"sqli-template", "xss-template"}, + []string{matched[0].ID, matched[1].ID}, + )internal/cli/start.go (1)
53-82: Consider reusingstartTemplateto remove duplicated start/post-install logic.The tag loop currently re-implements the same start + post-install + logging steps. A small refactor can keep behavior identical while reducing duplication and keeping single/multi flows consistent.
♻️ Suggested refactor (return error from helper)
- for _, template := range templates { - if err := provider.Start(template); err != nil { - log.Error().Msgf("failed to start %s: %v", template.ID, err) - failed = append(failed, template.ID) - continue - } - - if len(template.PostInstall) > 0 { - log.Info().Msgf("Post-installation instructions for %s:", template.ID) - for _, instruction := range template.PostInstall { - fmt.Printf(" %s\n", instruction) - } - } - - log.Info().Msgf("%s template is running on %s", template.ID, providerName) - } + for _, template := range templates { + if err := c.startTemplate(provider, template, providerName); err != nil { + log.Error().Msgf("failed to start %s: %v", template.ID, err) + failed = append(failed, template.ID) + continue + } + }-func (c *CLI) startTemplate(provider interface{ Start(*tmpl.Template) error }, template *tmpl.Template, providerName string) { - err := provider.Start(template) - if err != nil { - log.Fatal().Msgf("%v", err) - } - - if len(template.PostInstall) > 0 { - log.Info().Msg("Post-installation instructions:") - for _, instruction := range template.PostInstall { - fmt.Printf(" %s\n", instruction) - } - } - - log.Info().Msgf("%s template is running on %s", template.ID, providerName) -} +func (c *CLI) startTemplate(provider interface{ Start(*tmpl.Template) error }, template *tmpl.Template, providerName string) error { + if err := provider.Start(template); err != nil { + return err + } + + if len(template.PostInstall) > 0 { + log.Info().Msgf("Post-installation instructions for %s:", template.ID) + for _, instruction := range template.PostInstall { + fmt.Printf(" %s\n", instruction) + } + } + + log.Info().Msgf("%s template is running on %s", template.ID, providerName) + return nil +}- c.startTemplate(provider, template, providerName) + if err := c.startTemplate(provider, template, providerName); err != nil { + log.Fatal().Msgf("%v", err) + }Also applies to: 99-113
| tags := strings.Split(tagsStr, ",") | ||
| templates, err := tmpl.GetByTags(c.app.Templates, tags) | ||
| if err != nil { | ||
| log.Fatal().Msgf("%v", err) | ||
| } | ||
|
|
||
| log.Info().Msgf("found %d templates matching tags: %s", len(templates), tagsStr) |
There was a problem hiding this comment.
Sanitize tag tokens to avoid accidental “match all”.
strings.Split can yield empty/whitespace tags (e.g., --tags sqli, or --tags sqli, xss). With substring matching, an empty tag could unintentionally match every template. Trim and filter empties before calling GetByTags, and fail if none remain.
🛠️ Proposed fix
- tags := strings.Split(tagsStr, ",")
- templates, err := tmpl.GetByTags(c.app.Templates, tags)
+ rawTags := strings.Split(tagsStr, ",")
+ var tags []string
+ for _, t := range rawTags {
+ t = strings.TrimSpace(t)
+ if t != "" {
+ tags = append(tags, t)
+ }
+ }
+ if len(tags) == 0 {
+ log.Fatal().Msg("no valid tags provided")
+ }
+ templates, err := tmpl.GetByTags(c.app.Templates, tags)
if err != nil {
log.Fatal().Msgf("%v", err)
}
- log.Info().Msgf("found %d templates matching tags: %s", len(templates), tagsStr)
+ log.Info().Msgf("found %d templates matching tags: %s", len(templates), strings.Join(tags, ","))📝 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.
| tags := strings.Split(tagsStr, ",") | |
| templates, err := tmpl.GetByTags(c.app.Templates, tags) | |
| if err != nil { | |
| log.Fatal().Msgf("%v", err) | |
| } | |
| log.Info().Msgf("found %d templates matching tags: %s", len(templates), tagsStr) | |
| rawTags := strings.Split(tagsStr, ",") | |
| var tags []string | |
| for _, t := range rawTags { | |
| t = strings.TrimSpace(t) | |
| if t != "" { | |
| tags = append(tags, t) | |
| } | |
| } | |
| if len(tags) == 0 { | |
| log.Fatal().Msg("no valid tags provided") | |
| } | |
| templates, err := tmpl.GetByTags(c.app.Templates, tags) | |
| if err != nil { | |
| log.Fatal().Msgf("%v", err) | |
| } | |
| log.Info().Msgf("found %d templates matching tags: %s", len(templates), strings.Join(tags, ",")) |
🤖 Prompt for AI Agents
In `@internal/cli/stop.go` around lines 58 - 64, The current splitting of tagsStr
into tags can produce empty or whitespace tokens that will match everything;
instead, after splitting tagsStr, trim each token and filter out empty strings
to produce a cleaned tags slice, then if the cleaned slice is empty fail with an
error (or log.Fatal) rather than calling tmpl.GetByTags; call
tmpl.GetByTags(c.app.Templates, cleanedTags) and update the log message to
report cleanedTags (referencing tagsStr, tags, tmpl.GetByTags and
c.app.Templates).
Summary
--tags/-tflag tostartcommand for starting multiple templates by tag--tags/-tflag tostopcommand for stopping multiple templates by tagGetByTags()helper function for retrieving templates matching given tagsUsage
Features
--tags SQLImatches templates with "sqli" tag--tags owamatches templates with "owasp" tag--idand--tagscannot be used togetherTest plan
GetByTags()functiontemplateMatchesTags()helperCloses #101
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.