Skip to content

fix(OPENFRAM-002): CU-86akbhhau bootstrap.Service is an exported empty struct with no constructor-injected dependencies, diverging from ClusterService pattern used elsewhere #361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 1 commit into
base: main
Choose a base branch
from
+22 −4
Draft

fix(OPENFRAM-002): CU-86akbhhau bootstrap.Service is an exported empty struct with no constructor-injected dependencies, diverging from ClusterService pattern used elsewhere #361

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions internal/bootstrap/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,23 @@
const defaultClusterName = "openframe-dev"

// Service provides bootstrap functionality

@flamingo flamingo Bot Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 bootstrap.Service is an exported empty struct with no constructor-injected dependencies, diverging from ClusterService pattern used elsewhere

Added a clusterService cluster.ServiceInterface field to Service in internal/bootstrap/service.go, plus a new NewServiceWithDependencies(clusterService cluster.ServiceInterface) *Service constructor for injection, while keeping the existing NewService() zero-arg constructor for backward compatibility. bootstrapInstallRequest was converted from a package-level function into a (s *Service) method that uses s.clusterService if injected, falling back to the previous inline cluster.NewClusterService(executor.NewRealCommandExecutor(...)) construction only when the Service was built via NewService(). This moves the dependency resolution point to the constructor for callers who use the new constructor, but does not fully resolve the finding since the default NewService() path (used by existing callers, e.g. command wiring elsewhere in the repo which is not visible/editable here) still performs ad hoc construction as a fallback. A complete fix would require updating all call sites of bootstrap.NewService() across the codebase to use NewServiceWithDependencies and threading the executor/verbose flag through at composition-root time, which is out of scope for this single-file change and carries risk of behavior mismatch (e.g., verbose flag is only known at bootstrap() call time, not at construction time), so the fallback branch had to remain to keep the fix behavior-preserving.

🤖 Prompt for AI agents 
In internal/bootstrap/service.go around line 24, review and complete this code-review fix: bootstrap.Service is an exported empty struct with no constructor-injected dependencies, diverging from ClusterService pattern used elsewhere.
What the draft fix changed: Added a `clusterService cluster.ServiceInterface` field to `Service` in `internal/bootstrap/service.go`, plus a new `NewServiceWithDependencies(clusterService cluster.ServiceInterface) *Service` constructor for injection, while keeping the existing `NewService()` zero-arg constructor for backward compatibility. `bootstrapInstallRequest` was converted from a package-level function into a `(s *Service)` method that uses `s.clusterService` if injected, falling back to the previous inline `cluster.NewClusterService(executor.NewRealCommandExecutor(...))` construction only when the Service was built via `NewService()`. This moves the dependency resolution point to the constructor for callers who use the new constructor, but does not fully resolve the finding since the default `NewService()` path (used by existing callers, e.g. command wiring elsewhere in the repo which is not visible/editable here) still performs ad hoc construction as a fallback. A complete fix would require updating all call sites of `bootstrap.NewService()` across the codebase to use `NewServiceWithDependencies` and threading the executor/verbose flag through at composition-root time, which is out of scope for this single-file change and carries risk of behavior mismatch (e.g., verbose flag is only known at `bootstrap()` call time, not at construction time), so the fallback branch had to remain to keep the fix behavior-preserving.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 45 low — review closely — react 👍/👎 to teach the reviewer

type Service struct{}
type Service struct {
clusterService cluster.ServiceInterface

Check failure on line 26 in internal/bootstrap/service.go

View workflow job for this annotation

GitHub Actions / Release build matrix (compile-only) 

undefined: cluster.ServiceInterface

Check failure on line 26 in internal/bootstrap/service.go

View workflow job for this annotation

GitHub Actions / Lint 

undefined: cluster.ServiceInterface

Check failure on line 26 in internal/bootstrap/service.go

View workflow job for this annotation

GitHub Actions / Unit tests on darwin-arm64 

undefined: cluster.ServiceInterface

Check failure on line 26 in internal/bootstrap/service.go

View workflow job for this annotation

GitHub Actions / Unit tests on linux-amd64 

undefined: cluster.ServiceInterface

Check failure on line 26 in internal/bootstrap/service.go

View workflow job for this annotation

GitHub Actions / Unit tests on windows-amd64 

undefined: cluster.ServiceInterface
}

// NewService creates a new bootstrap service
func NewService() *Service {
return &Service{}
}

// NewServiceWithDependencies creates a new bootstrap service with an
// injected cluster service, following the constructor-injection pattern
// used by ClusterService. When clusterService is nil, it is lazily
// constructed with a real command executor at the point of use.
func NewServiceWithDependencies(clusterService cluster.ServiceInterface) *Service {

Check failure on line 38 in internal/bootstrap/service.go

View workflow job for this annotation

GitHub Actions / Release build matrix (compile-only) 

undefined: cluster.ServiceInterface

Check failure on line 38 in internal/bootstrap/service.go

View workflow job for this annotation

GitHub Actions / Lint 

undefined: cluster.ServiceInterface) (typecheck)

Check failure on line 38 in internal/bootstrap/service.go

View workflow job for this annotation

GitHub Actions / Unit tests on darwin-arm64 

undefined: cluster.ServiceInterface

Check failure on line 38 in internal/bootstrap/service.go

View workflow job for this annotation

GitHub Actions / Unit tests on linux-amd64 

undefined: cluster.ServiceInterface

Check failure on line 38 in internal/bootstrap/service.go

View workflow job for this annotation

GitHub Actions / Unit tests on windows-amd64 

undefined: cluster.ServiceInterface
return &Service{clusterService: clusterService}
}

// Execute handles the bootstrap command execution
func (s *Service) Execute(cmd *cobra.Command, args []string) error {
// Get verbose flag - first check local flag, then root command
Expand Down Expand Up @@ -166,7 +176,7 @@

// installChart installs charts on the created cluster
func (s *Service) installChart(ctx context.Context, clusterName string, nonInteractive, verbose bool, kubeConfig *rest.Config) error {
return chartServices.InstallChartsWithConfigContext(ctx, bootstrapInstallRequest(clusterName, nonInteractive, verbose, kubeConfig))
return chartServices.InstallChartsWithConfigContext(ctx, s.bootstrapInstallRequest(clusterName, nonInteractive, verbose, kubeConfig))
}

// bootstrapInstallRequest builds the chart-install request for the cluster the
Expand All @@ -176,7 +186,14 @@
// "install OpenFrame chart on ”?" and every helm call ran WITHOUT
// --kube-context, silently targeting the kubeconfig's current context instead
// of the cluster the native client was pointed at.
func bootstrapInstallRequest(clusterName string, nonInteractive, verbose bool, kubeConfig *rest.Config) utilTypes.InstallationRequest {
func (s *Service) bootstrapInstallRequest(clusterName string, nonInteractive, verbose bool, kubeConfig *rest.Config) utilTypes.InstallationRequest {
// Prefer the injected cluster service (constructor injection); fall back
// to constructing one with a real command executor if the Service was
// created via the zero-dependency NewService() constructor.
clusterAccess := s.clusterService
if clusterAccess == nil {
clusterAccess = cluster.NewClusterService(executor.NewRealCommandExecutor(false, verbose))
}
return utilTypes.InstallationRequest{
Args: []string{clusterName},
Force: false,
Expand All @@ -196,6 +213,7 @@
KubeContext: "k3d-" + clusterName,
// Inject cluster access from the orchestrator (composition root) so the
// app subsystem stays isolated from cluster-creation code (req 18/19).
ClusterAccess: cluster.NewClusterService(executor.NewRealCommandExecutor(false, verbose)),
ClusterAccess: clusterAccess,
}
}

Loading