refactor: eliminate global state and introduce di #97
Conversation
- Add App struct as central dependency container (internal/app/app.go) - Remove global Templates map from pkg/template, use pure functions - Remove global Providers map from pkg/provider/registry, use factory - Inject StateManager into DockerCompose provider - Refactor CLI to struct-based approach with dependency injection - Add logger factory functions (New, NewWithLevel, SetGlobal) - Delete duplicate pkg/templates/ package - Update tests to work with new function signatures All dependencies are now explicitly wired in main.go at startup, making the codebase more testable and eliminating hidden state.
WalkthroughThis PR rewires startup into an explicit dependency-injected App: adds Config/App, replaces package Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Main as cmd/vt/main
participant Config as app.DefaultConfig
participant Logger as logger.NewWithLevel
participant Templates as template.LoadTemplates
participant Disk as disk.NewConfig
participant State as state.NewManager
participant Registry as registry.NewProviders
participant App as app.NewApp
participant CLI as cli.New / CLI.Run
participant Cobra as Cobra (commands)
participant Provider as provider (docker-compose, ...)
Main->>Config: load defaults
Main->>Logger: NewWithLevel(cfg.LogLevel) / SetGlobal
Main->>Templates: LoadTemplates(cfg.TemplatesPath)
Main->>Disk: NewConfig().WithFileName(...).WithBucketName(...)
Main->>State: NewManager(diskConfig)
Main->>Registry: NewProviders(stateManager)
Main->>App: NewApp(templates, providers, stateManager, cfg)
Main->>CLI: New(app) -> setupCommands
CLI->>Cobra: execute command (Run)
Cobra->>App: access StateManager / Templates / GetProvider
Cobra->>Provider: Start/Stop/Status(template)
Provider-->>Cobra: result/status
Cobra-->>CLI: render output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
dogancanbakir
changed the title
![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
🧹 Nitpick comments (7)
internal/cli/start.go (1)
28-33: Consider removing redundant empty check.Since the
idflag is marked as required on line 68, Cobra will enforce its presence beforeRunis invoked. This empty-string check is effectively unreachable unless the flag's default were changed to a non-empty value.If retained for defensive purposes, consider adding a comment explaining why.
pkg/provider/dockercompose/dockercompose.go (2)
110-114: Clarify return values for consistency.When
runningis false on line 110, returningerr(which isnilat this point) is technically correct but could be clearer. Consider explicitly returningnilor a descriptive error explaining why the status is unknown.Similarly, line 114 could explicitly return
nilinstead of reusingerr.🔎 Suggested clarification
if !running { - return "unknown", err + return "unknown", nil } - return "running", err + return "running", nil
31-31: Consider documenting why the error is ignored.The
nolint:errchecksuppresses the linter, but a brief comment explaining why it's safe to ignore the error fromDeploymentExistwould aid future maintainers.internal/cli/cli.go (3)
60-62: Consider sorting log levels for consistent help text.
maps.Keysreturns keys in undefined order, so the verbosity help text will display levels in a different order on each run. For a consistent user experience, consider sorting the levels.🔎 Suggested fix
+ levelNames := slices.Sorted(maps.Keys(logLevels)) c.rootCmd.PersistentFlags().StringP("verbosity", "v", zerolog.InfoLevel.String(), fmt.Sprintf("Set the verbosity level for logs (%s)", - strings.Join(slices.Collect(maps.Keys(logLevels)), ", "))) + strings.Join(levelNames, ", ")))
76-79: Provider names ordering is non-deterministic.Similar to the log levels,
providerNames()returns keys in undefined order. If this is used in help text (as seen in start.go and stop.go), consider sorting for consistency.🔎 Suggested fix
func (c *CLI) providerNames() []string { - return slices.Collect(maps.Keys(c.app.Providers)) + return slices.Sorted(maps.Keys(c.app.Providers)) }
19-26: The logLevels map is only used for help text, not validation.The map appears intended for validating log levels, but validation actually occurs in
zerolog.ParseLevelwithinlogger.InitWithLevel. Consider either:
- Adding explicit validation in
PersistentPreRunusing this map, or- Simplifying to a slice if only needed for help text.
internal/cli/list.go (1)
50-70: Consider thread-safety for template map mutation.Directly mutating
c.app.Templates(line 67) works for sequential CLI usage but could cause data races if any background operations access the templates concurrently. If this is a concern, consider using a mutex or an atomic swap pattern.For now this is likely fine since CLI commands run sequentially, but worth noting if concurrency is introduced later.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
.gitignoreCONTRIBUTING.mdREADME.mdcmd/vt/main.gogo.modinternal/app/app.gointernal/cli/cli.gointernal/cli/list.gointernal/cli/ps.gointernal/cli/root.gointernal/cli/start.gointernal/cli/stop.gointernal/logger/logger.gopkg/provider/dockercompose/dockercompose.gopkg/provider/dockercompose/utils.gopkg/provider/provider.gopkg/provider/registry/registry.gopkg/template/downloader.gopkg/template/parser.gopkg/template/parser_test.gopkg/template/template.gopkg/template/template_test.gopkg/template/validator.go
💤 Files with no reviewable changes (1)
- internal/cli/root.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T04:55:30.645Z
Learnt from: recepgunes1
Repo: HappyHackingSpace/vulnerable-target PR: 57
File: pkg/provider/dockercompose/utils.go:76-91
Timestamp: 2025-08-29T04:55:30.645Z
Learning: When using the Docker Compose Go library (compose-go) directly instead of the docker-compose CLI, manually setting Docker Compose-managed labels like com.docker.compose.project, com.docker.compose.service, etc. is necessary for proper functionality. Without these labels, Docker Compose won't properly attach and manage volumes, networks, and containers as part of the project.
Applied to files:
pkg/provider/dockercompose/utils.gogo.modpkg/provider/dockercompose/dockercompose.go
🧬 Code graph analysis (13)
pkg/provider/dockercompose/utils.go (1)
pkg/template/template.go (1)
Template(17-24)
internal/app/app.go (4)
pkg/template/template.go (1)
Template(17-24)pkg/provider/provider.go (1)
Provider(9-14)internal/state/state.go (1)
Manager(20-22)pkg/provider/registry/registry.go (1)
GetProvider(19-22)
pkg/provider/registry/registry.go (3)
internal/state/state.go (1)
Manager(20-22)pkg/provider/provider.go (1)
Provider(9-14)pkg/provider/dockercompose/dockercompose.go (1)
NewDockerCompose(20-22)
pkg/template/downloader.go (1)
pkg/template/template.go (1)
TemplateRemoteRepository(14-14)
pkg/provider/provider.go (1)
pkg/template/template.go (1)
Template(17-24)
pkg/provider/dockercompose/dockercompose.go (3)
pkg/provider/provider.go (1)
Provider(9-14)internal/state/state.go (1)
Manager(20-22)pkg/template/template.go (1)
Template(17-24)
internal/cli/stop.go (3)
internal/cli/cli.go (1)
CLI(29-32)pkg/provider/registry/registry.go (1)
GetProvider(19-22)pkg/template/template.go (1)
GetByID(192-198)
internal/cli/cli.go (3)
internal/app/app.go (1)
App(21-26)internal/logger/logger.go (2)
New(34-52)InitWithLevel(71-74)internal/banner/banner.go (1)
AppVersion(15-15)
internal/logger/logger.go (2)
internal/app/app.go (2)
DefaultConfig(29-40)Config(14-18)internal/cli/cli.go (1)
New(35-41)
pkg/template/template.go (1)
pkg/template/parser.go (1)
LoadTemplate(12-24)
internal/cli/list.go (3)
internal/cli/cli.go (1)
CLI(29-32)pkg/template/template.go (4)
ListTemplatesWithFilter(139-189)SyncTemplates(125-131)LoadTemplates(55-64)Info(27-39)internal/app/app.go (1)
Config(14-18)
pkg/template/template_test.go (1)
pkg/template/template.go (2)
Template(17-24)GetByID(192-198)
cmd/vt/main.go (8)
internal/app/app.go (2)
DefaultConfig(29-40)NewApp(43-55)internal/logger/logger.go (4)
DefaultConfig(22-29)NewWithLevel(56-60)SetGlobal(65-67)New(34-52)internal/banner/banner.go (1)
pkg/template/template.go (1)
LoadTemplates(55-64)pkg/store/disk/config.go (1)
NewConfig(10-15)internal/state/state.go (1)
NewManager(25-31)pkg/provider/registry/registry.go (1)
NewProviders(12-16)internal/cli/cli.go (1)
New(35-41)
⏰ 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)
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (32)
.gitignore (1)
36-37: No concerns with the added ignore patterns.Adding tool-specific artifact files (
.claudeandCLAUDE.md) to.gitignoreis appropriate and follows standard practice for IDE and helper tool ignores.CONTRIBUTING.md (1)
11-11: External repository links are valid and accessible. The documentation update correctly directs template contributions to the vt-templates repository, and the example template atcves/vt-2024-53995/index.yamlexists as referenced.pkg/template/validator.go (1)
1-1: LGTM! Package rename aligns with PR objectives.The package rename from
templatestotemplateis consistent with the broader refactoring to eliminate global state and follows Go naming conventions.README.md (1)
15-75: Excellent documentation improvements!The updated README provides clear, actionable guidance for users:
- Specific feature list aligned with the new architecture
- Clear prerequisites and installation steps
- Comprehensive CLI usage examples
- Information about template management
These changes greatly improve the developer experience and align well with the dependency injection refactoring.
pkg/template/template.go (6)
13-14: Good catch fixing the typo!The constant name correction from
TemplateRemoteRepoistorytoTemplateRemoteRepositoryimproves code quality.
52-64: Excellent refactoring to eliminate global state!The new
LoadTemplatesfunction properly:
- Accepts an explicit
repoPathparameter instead of relying on global state- Clones the repository on first use when it doesn't exist
- Returns a map and error instead of populating a global variable
- Provides proper error propagation with context
This significantly improves testability and follows dependency injection principles.
68-122: Well-structured functional refactoring!The helper functions
loadTemplatesFromDirectoryandloadTemplatesFromCategorynow:
- Return
(map[string]Template, error)instead of mutating global state- Properly aggregate results by merging maps
- Validate template ID matches directory name (lines 115-116)
- Propagate errors with descriptive messages using
fmt.Errorfwith%wThe removal of
log.Fatalin favor of error returns allows callers to handle errors appropriately.
125-131: SyncTemplates properly refactored for dependency injection.The function now:
- Accepts an explicit
repoPathparameter- Returns an error instead of calling
log.Fatal- Provides proper error context with
fmt.ErrorfwrappingThis allows callers to handle synchronization failures gracefully.
134-189: List functions successfully migrated to functional style.Both
ListTemplatesandListTemplatesWithFilternow:
- Accept a
templatesmap parameter instead of referencing global state- Maintain the same display logic and filtering behavior
- Use cleaner variable naming (
tmplinstead oftemplate)The refactoring preserves functionality while improving testability.
192-198: GetByID improved with explicit parameters and error handling.The new signature
GetByID(templates map[string]Template, templateID string) (*Template, error)is a significant improvement:
- Accepts an explicit templates map instead of using global state
- Returns an error when template is not found instead of returning
nil- Provides better error information to callers
Note: This is a breaking API change, but it aligns with the PR's objectives and improves error handling.
pkg/template/parser_test.go (1)
1-1: LGTM! Test package rename is consistent.The package rename from
templatestotemplatealigns with the corresponding changes in the production code and maintains test coverage.pkg/template/parser.go (1)
1-2: LGTM! Package rename is clean and consistent.The package declaration and documentation have been correctly updated from
templatestotemplate, aligning with the broader refactoring effort across the codebase.pkg/template/downloader.go (2)
1-1: LGTM! Package declaration updated correctly.Package renamed from
templatestotemplateas part of the broader refactoring.
50-50: Great catch fixing the typo!The constant name has been corrected from
TemplateRemoteRepoistory(typo) toTemplateRemoteRepository, matching the actual constant definition in pkg/template/template.go.pkg/provider/dockercompose/utils.go (1)
16-16: LGTM! Import alias and function signature updated consistently.The import has been updated to use the
tmplalias for the renamedtemplatepackage, and theloadComposeProjectfunction signature correctly usestmpl.Template. This is consistent with the package rename across the codebase.Also applies to: 34-34
pkg/provider/provider.go (1)
5-5: LGTM! Provider interface updated consistently.The import alias has been changed to
tmpland all three interface methods (Start,Stop,Status) now correctly use*tmpl.Template. The interface changes are uniform and align with the package rename.Also applies to: 11-13
pkg/provider/registry/registry.go (2)
10-16: Excellent refactoring! Factory function eliminates global state.The
NewProvidersfactory function is a significant improvement:
- Accepts explicit
*state.Managerdependency instead of relying on globals- Returns a properly initialized provider map
- Enables dependency injection for better testability
- Thread-safe by design (no shared mutable state)
This aligns perfectly with the PR's objective to eliminate global state and introduce dependency injection.
18-22: LGTM! GetProvider is now a pure function.The refactored
GetProviderfunction correctly accepts the providers map as a parameter and returns a clean(provider.Provider, bool)tuple. This eliminates hidden global state access and makes the function's dependencies explicit.pkg/template/template_test.go (1)
1-32: LGTM!The test correctly adapts to the new functional API where
GetByIDaccepts an explicit templates map. The localtemplatesvariable properly replaces the removed global, and both success and error cases are appropriately tested.internal/cli/stop.go (1)
12-63: LGTM!The stop command is cleanly refactored to use dependency injection via the CLI receiver. Provider and template lookups correctly use the injected app context, and the required flags are properly configured.
internal/cli/start.go (1)
12-73: Clean DI refactor for start command.The command correctly uses the injected app context for provider and template lookups. Post-install instruction handling is preserved.
internal/cli/ps.go (1)
13-67: LGTM!The ps command is well-refactored with proper error handling. The approach of logging errors for missing providers/templates while continuing iteration provides a good user experience. The early return on
ListDeploymentserror and the "no running environment" message when count is zero are appropriate.pkg/provider/dockercompose/dockercompose.go (1)
15-22: Good use of constructor-based dependency injection.The
stateManagerfield andNewDockerComposeconstructor cleanly inject the state dependency, improving testability and eliminating hidden global state. This aligns well with the retrieved learning about Docker Compose label management requiring explicit control.internal/app/app.go (1)
1-55: Well-structured dependency container.The
Appstruct provides a clean central container for application dependencies.DefaultConfigwith home directory fallback andNewAppconstructor enable explicit wiring at startup, which aligns well with the PR's goal of eliminating hidden global state.internal/cli/cli.go (1)
28-74: Clean struct-based CLI architecture.The refactored CLI with dependency injection via
*app.Appis well-structured. The separation ofNew,setupCommands, andRunprovides good testability and clear initialization flow.internal/logger/logger.go (3)
13-29: Well-designed configuration struct with sensible defaults.The
Configstruct provides good flexibility for testing and configuration. TheDefaultConfig()function provides reasonable defaults.
54-67: Clean separation of concerns for logger initialization.
NewWithLevelprovides a convenient factory, andSetGlobalis explicitly documented as the only function that modifies global state. This aligns well with the PR's goal of eliminating hidden global state.
69-80: Good deprecation approach.The deprecated functions delegate to the new APIs, maintaining backward compatibility while guiding users toward the preferred pattern.
cmd/vt/main.go (2)
18-27: Clean initialization and wiring sequence.The explicit configuration-driven initialization flow is clear and testable. Dependencies are wired in a logical order with the logger set up early to capture any subsequent errors.
29-48: Well-structured dependency wiring.Each component (templates, state manager, providers, app) is created with explicit dependencies and proper error handling. This aligns well with the PR's goal of eliminating global state.
internal/cli/list.go (2)
9-48: Well-structured command with thorough flag validation.The method-based approach with receiver
(c *CLI)aligns well with the DI pattern. Flag validation logic is comprehensive and provides clear error messages.
74-78: Flags are properly configured.The flags are well-defined with appropriate short-hand aliases and descriptions.
- Remove unreachable os.Exit(1) after log.Fatal() in main.go - Remove unused os import from main.go - Remove dead GetTemplate method from App struct - Add nil check guard clause in logger.New() to prevent panic - Update go-git dependency from v5.16.4 to v5.16.3
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/vt/main.go (1)
33-40: Consider making storage configuration values configurable.The hardcoded database filename (
"deployments.db") and bucket name ("deployments") reduce flexibility. For improved testability and deployment scenarios, consider adding these as fields inapp.Configwith sensible defaults.🔎 Proposed refactor
In
internal/app/app.go, add fields to the Config struct:type Config struct { TemplatesPath string StoragePath string LogLevel string + DBFileName string + DBBucketName string }Update
DefaultConfig():return &Config{ TemplatesPath: filepath.Join(homeDir, "vt-templates"), StoragePath: filepath.Join(homeDir, ".vt-cli"), LogLevel: "info", + DBFileName: "deployments.db", + DBBucketName: "deployments", }Then in
cmd/vt/main.go:storeCfg := disk.NewConfig(). - WithFileName("deployments.db"). - WithBucketName("deployments") + WithFileName(cfg.DBFileName). + WithBucketName(cfg.DBBucketName)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/vt/main.gogo.modinternal/app/app.gointernal/logger/logger.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T04:55:30.645Z
Learnt from: recepgunes1
Repo: HappyHackingSpace/vulnerable-target PR: 57
File: pkg/provider/dockercompose/utils.go:76-91
Timestamp: 2025-08-29T04:55:30.645Z
Learning: When using the Docker Compose Go library (compose-go) directly instead of the docker-compose CLI, manually setting Docker Compose-managed labels like com.docker.compose.project, com.docker.compose.service, etc. is necessary for proper functionality. Without these labels, Docker Compose won't properly attach and manage volumes, networks, and containers as part of the project.
Applied to files:
go.mod
🧬 Code graph analysis (1)
internal/app/app.go (1)
internal/state/state.go (1)
Manager(20-22)
⏰ 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)
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
go.mod (1)
9-9: ✅ Version issue resolved.Line 9 correctly upgrades go-git from the non-existent v5.16.4 to v5.16.3, the latest official v5 release. This addresses the dependency resolution issue flagged in previous reviews and aligns with the PR's goal of explicit dependency wiring.
internal/logger/logger.go (5)
13-29: LGTM! Clean config-driven design.The
Configstruct andDefaultConfig()function provide a clean, testable API for logger initialization. The defaults are sensible and the design eliminates global state mutation during construction.
31-57: LGTM! Nil-check correctly addresses past review.The nil-check at lines 36-38 correctly guards against nil dereference and falls back to
DefaultConfig(). This addresses the previous review comment and follows defensive programming best practices.
59-65: LGTM! Convenient helper function.The convenience wrapper simplifies the common case of creating a logger with a custom level while using defaults for other options.
67-72: LGTM! Global state mutation is explicit and documented.Isolating global state mutation to a single, well-documented function aligns with the PR's goal of making global state explicit. The comment clearly indicates it should be called once at startup.
74-85: LGTM! Clear deprecation path.The deprecated functions are properly marked with clear migration guidance, and their implementations correctly delegate to the new API. This provides a smooth transition path for existing code.
cmd/vt/main.go (4)
17-22: LGTM! Clean initialization flow.The config-driven logger initialization clearly demonstrates the new pattern: create logger from config, then explicitly set it globally. This aligns perfectly with the PR's goal of eliminating hidden global state.
42-46: LGTM! Clean dependency wiring.The explicit dependency injection pattern is clear: providers receive the state manager, and the app receives all components. This eliminates hidden globals and improves testability.
48-51: LGTM! Past review comment correctly addressed.The unreachable
os.Exit(1)mentioned in the past review has been removed. The error handling is now clean and correct.
27-31: No action needed. LoadTemplates already returns the correct type.The
LoadTemplatesfunction (pkg/template/template.go:55) returnsmap[string]Template, which matches the type expected byNewApp(internal/app/app.go:44). The code is correct as-is.internal/app/app.go (5)
13-18: LGTM! Clean configuration structure.The
Configstruct provides a clear, centralized place for application configuration. The fields are well-chosen for the app's needs.
20-26: LGTM! Well-designed dependency container.The
Appstruct serves as a clear dependency container with all components explicitly wired. The use of maps for templates and providers enables efficient lookups.
28-40: LGTM! Robust default configuration.The fallback to
"."whenos.UserHomeDir()fails is a pragmatic choice for a CLI tool, and usingfilepath.Joinensures cross-platform compatibility.
42-55: LGTM! Straightforward dependency wiring.The constructor cleanly wires all dependencies into the app container. The lack of validation is acceptable for an internal package where callers are trusted.
57-61: LGTM! Idiomatic accessor with past issue addressed.The
GetProvidermethod follows Go's idiomatic pattern for map lookups with existence checking. The unusedGetTemplatemethod mentioned in past reviews has been correctly removed.
recepgunes1
left a comment
recepgunes1
left a comment
There was a problem hiding this comment.
remove logs between banner and help text
|
@dogancanbakir we should refactor Proposed implementation: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
internal/cli/stop.go (1)
23-26: Consider adding empty ID validation for consistency.Unlike
start.go(lines 28-33), this command doesn't validate whethertemplateIDis empty before proceeding. While Cobra validates flag presence (line 58), it won't catch explicit empty values like--id "". Consider adding the same validation asstart.goto provide consistent UX across commands.🔎 Proposed validation check
templateID, err := cmd.Flags().GetString("id") if err != nil { log.Fatal().Msgf("%v", err) } + +if len(templateID) == 0 { + if err := cmd.Help(); err != nil { + log.Fatal().Msgf("%v", err) + } + return +} provider, ok := c.app.GetProvider(providerName)internal/cli/start.go (1)
28-33: Good defensive validation, but inconsistent with stop.go.This empty ID check is valuable because it catches
--id ""cases that Cobra's required flag validation misses. However,stop.go(lines 23-26) lacks this same validation, creating an inconsistency in UX between commands. Consider adding the same validation tostop.gofor consistency.cmd/vt/main.go (2)
18-19: Consider passing logger via DI instead of setting global.While global loggers are a common pattern, setting
logger.SetGlobal()reintroduces global state. For stricter DI adherence, consider passing the logger instance through the app context and injecting it where needed.
26-28: Move database configuration to app config.The hardcoded database filename and bucket name should be externalized to
app.Configfor consistency with other configuration values likeTemplatesPathand to support different environments.🔎 Suggested approach
Add fields to the config struct (in
internal/app/app.go):type Config struct { // ... existing fields StateFileName string StateBucketName string }Then use them in main.go:
- storeCfg := disk.NewConfig(). - WithFileName("deployments.db"). - WithBucketName("deployments") + storeCfg := disk.NewConfig(). + WithFileName(cfg.StateFileName). + WithBucketName(cfg.StateBucketName)pkg/provider/dockercompose/utils.go (2)
174-177: Move categoryMap to package level.The
categoryMapis recreated on every function call. Move it to package level as a variable or use a sync.Once pattern for better performance.🔎 Proposed refactor
At the package level (after imports):
+var categoryMap = map[string]string{ + "Lab": "labs", + "CVE": "cves", +} + func createDockerCLI() (command.Cli, error) {Then remove lines 174-177 from inside the function.
179-182: Improve error message to include context.The error message doesn't indicate which template type was provided or what the valid options are, making debugging harder.
🔎 Proposed improvement
category, ctgExist := categoryMap[templateType] if !ctgExist { - return "", "", errors.New("undefined category for template") + return "", "", fmt.Errorf("undefined category for template type %q (expected: Lab or CVE)", templateType) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/vt/main.gointernal/cli/cli.gointernal/cli/start.gointernal/cli/stop.gopkg/provider/dockercompose/utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cli/cli.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T04:55:30.645Z
Learnt from: recepgunes1
Repo: HappyHackingSpace/vulnerable-target PR: 57
File: pkg/provider/dockercompose/utils.go:76-91
Timestamp: 2025-08-29T04:55:30.645Z
Learning: When using the Docker Compose Go library (compose-go) directly instead of the docker-compose CLI, manually setting Docker Compose-managed labels like com.docker.compose.project, com.docker.compose.service, etc. is necessary for proper functionality. Without these labels, Docker Compose won't properly attach and manage volumes, networks, and containers as part of the project.
Applied to files:
pkg/provider/dockercompose/utils.go
🧬 Code graph analysis (3)
internal/cli/start.go (3)
internal/cli/cli.go (1)
CLI(29-32)pkg/provider/registry/registry.go (1)
GetProvider(19-22)pkg/template/template.go (1)
GetByID(192-198)
internal/cli/stop.go (3)
internal/cli/cli.go (1)
CLI(29-32)pkg/provider/registry/registry.go (1)
GetProvider(19-22)pkg/template/template.go (1)
GetByID(192-198)
pkg/provider/dockercompose/utils.go (1)
pkg/template/template.go (2)
Template(17-24)Info(27-39)
🔇 Additional comments (10)
internal/cli/stop.go (1)
12-63: LGTM! Clean DI refactoring.The refactoring successfully eliminates global state by using the app context for provider and template lookups. The command structure follows the new pattern correctly with proper error handling and flag validation.
internal/cli/start.go (1)
12-77: LGTM! Clean DI refactoring with proper validation.The refactoring successfully eliminates global state using the app context. The command structure follows the new pattern with proper error handling, flag validation, and post-install instructions display.
cmd/vt/main.go (4)
16-16: LGTM: Config initialization is clean.The default configuration approach provides a good starting point for the DI container.
21-24: LGTM: Template loading with proper error handling.The error handling with
log.Fatal()is appropriate for critical startup failures.
38-40: LGTM: CLI execution with proper error handling.The error handling and fatal logging on CLI errors is appropriate for the main entry point.
34-36: No changes needed. Bothregistry.NewProviders()andapp.NewApp()are infallible functions by design—they return only their primary types (map[string]provider.Providerand*Apprespectively) without error returns. Error handling is not required.Likely an incorrect or invalid review comment.
pkg/provider/dockercompose/utils.go (4)
5-5: LGTM! Import changes support the refactoring.The new imports are necessary for the dependency injection refactoring and error handling improvements.
Also applies to: 17-18
45-45: LGTM! Correctly passes template type for category resolution.The call now properly passes
template.Info.Typeto enable the category-based path resolution.
36-36: All call sites correctly updated. The function signature change fromtemplates.Templatetotmpl.Templateis properly implemented. All three call sites indockercompose.go(Start, Stop, and Status methods) correctly dereference the pointer parameter when callingloadComposeProject(*template).
170-192: No changes needed. The implementation follows proper dependency injection patterns already established in the codebase.The function correctly uses
app.DefaultConfig(), which is a pure factory function that creates a fresh configuration instance on each call—it does not introduce global state. This pattern is consistently used throughout the codebase (seecmd/vt/main.goandinternal/logger/logger.go, which explicitly documents this approach as "a pure factory function that returns a configured logger without modifying any global state"). The function is already fully testable without refactoring.Likely an incorrect or invalid review comment.
The categoryMap lookup now normalizes templateType to lowercase before matching, preventing failures when templates use different casing like "lab" instead of "Lab". Error message now lists accepted values. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/provider/dockercompose/utils.go (1)
175-182: Case-insensitive matching implemented correctly.The code now properly handles case variations by converting
templateTypeto lowercase before the map lookup (line 180) and provides a clear error message listing accepted values. This addresses the concern from the previous review.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/provider/dockercompose/utils.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T04:55:30.645Z
Learnt from: recepgunes1
Repo: HappyHackingSpace/vulnerable-target PR: 57
File: pkg/provider/dockercompose/utils.go:76-91
Timestamp: 2025-08-29T04:55:30.645Z
Learning: When using the Docker Compose Go library (compose-go) directly instead of the docker-compose CLI, manually setting Docker Compose-managed labels like com.docker.compose.project, com.docker.compose.service, etc. is necessary for proper functionality. Without these labels, Docker Compose won't properly attach and manage volumes, networks, and containers as part of the project.
Applied to files:
pkg/provider/dockercompose/utils.go
🧬 Code graph analysis (1)
pkg/provider/dockercompose/utils.go (1)
pkg/template/template.go (2)
Template(17-24)Info(27-39)
⏰ 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 (2)
pkg/provider/dockercompose/utils.go (2)
3-20: LGTM! Import changes align with refactoring.The new imports (
errors,strings,internal/app, and aliasedtmpl) are properly utilized for the refactored path resolution logic and dependency injection approach.
37-49: LGTM! Function signature updated correctly.The updated signature using
tmpl.Templateand the additionaltemplate.Info.Typeparameter passed toresolveComposePathproperly support the new category-based path resolution logic.
| cfg := app.DefaultConfig() | ||
| composePath = filepath.Join(cfg.TemplatesPath, category, templateID, path) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider injecting config rather than calling app.DefaultConfig().
Calling app.DefaultConfig() at line 185 introduces a hidden dependency that may contradict this PR's goal of explicit dependency injection. Each call to resolveComposePath retrieves the default config, which could bypass injected configuration and make testing harder.
Consider passing TemplatesPath as a parameter to resolveComposePath, or inject the config into the DockerCompose provider and make it available to this function. This would make dependencies explicit and improve testability.
💡 Refactoring approach
Option 1: Add templatesPath parameter
-func resolveComposePath(templateID, templateType, path string) (composePath string, workingDir string, err error) {
+func resolveComposePath(templateID, templateType, path, templatesPath string) (composePath string, workingDir string, err error) {
if filepath.IsAbs(path) {
return path, filepath.Dir(path), nil
}
var categoryMap = map[string]string{
"lab": "labs",
"cve": "cves",
}
category, ctgExist := categoryMap[strings.ToLower(templateType)]
if !ctgExist {
return "", "", errors.New("undefined category for template: type must be one of 'lab', 'cve'")
}
- cfg := app.DefaultConfig()
- composePath = filepath.Join(cfg.TemplatesPath, category, templateID, path)
+ composePath = filepath.Join(templatesPath, category, templateID, path)
if _, statErr := os.Stat(composePath); statErr != nil {
return "", "", fmt.Errorf("compose file %q not accessible: %w", composePath, statErr)
}
return composePath, filepath.Dir(composePath), nil
}Then update the caller at line 46 to pass the templates path from the provider's injected config.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/provider/dockercompose/utils.go around lines 185-186, calling
app.DefaultConfig() is a hidden dependency; change resolveComposePath to accept
a templatesPath string (remove the DefaultConfig call) and use
filepath.Join(templatesPath, category, templateID, path) instead, then update
the caller(s) (e.g., at line 46) to pass the provider's injected
config.TemplatesPath (or inject the config into the DockerCompose provider and
read TemplatesPath there), and update any tests to supply the templatesPath
parameter.
recepgunes1
deleted the
feat/eliminate-global-state-introduce-app-context
branch
recepgunes1
restored the
feat/eliminate-global-state-introduce-app-context
branch
recepgunes1
deleted the
feat/eliminate-global-state-introduce-app-context
branch
Summary
Appstruct as central dependency container (internal/app/app.go)Templatesmap frompkg/template, use pure functions that return valuesProvidersmap frompkg/provider/registry, use factory functionStateManagerintoDockerComposeprovider instead of creating inlineNew,NewWithLevel,SetGlobal)pkg/templates/packageAll dependencies are now explicitly wired in
main.goat startup, making the codebase more testable and eliminating hidden state.Test plan
go test ./...)golangci-lint run)go build ./...)vt template --list,vt start,vt stop,vt ps)Summary by CodeRabbit
New Features
Documentation
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.