-
Notifications
You must be signed in to change notification settings - Fork 0
refactor(workstation): Remove dependency injector #1876
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.
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
Conversation
Factors out the dependency injector from the workstation package. Signed-off-by: Ryan VanGundy <85766511+rmvangun@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on December 14
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Bug: Services Lack IPs: Networking Failure
The Up method never calls NetworkManager.AssignIPs(w.Services) to assign IP addresses to services. The refactor renamed Initialize to AssignIPs but removed the call from Up, leaving services without assigned IP addresses. This prevents services from getting their network configuration, breaking container networking. The AssignIPs call was removed when dependency injection was factored out but not replaced in the execution flow.
pkg/workstation/workstation.go#L123-L169
cli/pkg/workstation/workstation.go
Lines 123 to 169 in 3e1f57f
| // configures networking components, and informs the user of successful environment setup. | |
| func (w *Workstation) Up() error { | |
| if err := os.Setenv("NO_CACHE", "true"); err != nil { | |
| return fmt.Errorf("Error setting NO_CACHE environment variable: %w", err) | |
| } | |
| vmDriver := w.ConfigHandler.GetString("vm.driver") | |
| if vmDriver == "colima" { | |
| if w.VirtualMachine == nil { | |
| return fmt.Errorf("no virtual machine found") | |
| } | |
| if err := w.VirtualMachine.Up(); err != nil { | |
| return fmt.Errorf("error running virtual machine Up command: %w", err) | |
| } | |
| } | |
| for _, service := range w.Services { | |
| if err := service.WriteConfig(); err != nil { | |
| return fmt.Errorf("Error writing config for service %s: %w", service.GetName(), err) | |
| } | |
| } | |
| containerRuntimeEnabled := w.ConfigHandler.GetBool("docker.enabled") | |
| if containerRuntimeEnabled { | |
| if w.ContainerRuntime == nil { | |
| return fmt.Errorf("no container runtime found") | |
| } | |
| if err := w.ContainerRuntime.WriteConfig(); err != nil { | |
| return fmt.Errorf("failed to write container runtime config: %w", err) | |
| } | |
| if err := w.ContainerRuntime.Up(); err != nil { | |
| return fmt.Errorf("error running container runtime Up command: %w", err) | |
| } | |
| } | |
| if w.NetworkManager != nil { | |
| // Only configure guest and host routes for colima | |
| if vmDriver == "colima" { | |
| if err := w.NetworkManager.ConfigureGuest(); err != nil { | |
| return fmt.Errorf("error configuring guest: %w", err) | |
| } | |
| if err := w.NetworkManager.ConfigureHostRoute(); err != nil { | |
| return fmt.Errorf("error configuring host route: %w", err) | |
| } | |
| } | |
| // Configure DNS if enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing IP Assignment Cripples Network Startup
The Workstation.Up() method never calls NetworkManager.AssignIPs(), which is required to assign IP addresses to services before they write configs and start containers. The refactoring removed the call that used to happen via Initialize(), but didn't add it to Up(). Without IP assignment, services won't have valid addresses when WriteConfig() is called at line 144, causing networking to fail. The call should happen after VM startup but before service config writing.
pkg/workstation/workstation.go#L123-L147
cli/pkg/workstation/workstation.go
Lines 123 to 147 in 211bf2e
| // configures networking components, and informs the user of successful environment setup. | |
| func (w *Workstation) Up() error { | |
| if err := os.Setenv("NO_CACHE", "true"); err != nil { | |
| return fmt.Errorf("Error setting NO_CACHE environment variable: %w", err) | |
| } | |
| vmDriver := w.ConfigHandler.GetString("vm.driver") | |
| if vmDriver == "colima" { | |
| if w.VirtualMachine == nil { | |
| return fmt.Errorf("no virtual machine found") | |
| } | |
| if err := w.VirtualMachine.Up(); err != nil { | |
| return fmt.Errorf("error running virtual machine Up command: %w", err) | |
| } | |
| } | |
| for _, service := range w.Services { | |
| if err := service.WriteConfig(); err != nil { | |
| return fmt.Errorf("Error writing config for service %s: %w", service.GetName(), err) | |
| } | |
| } | |
| containerRuntimeEnabled := w.ConfigHandler.GetBool("docker.enabled") | |
| if containerRuntimeEnabled { | |
| if w.ContainerRuntime == nil { |
| // Create NetworkManager if not already set | ||
| if workstation.NetworkManager == nil { | ||
| workstation.NetworkManager = network.NewBaseNetworkManager(injector) | ||
| workstation.NetworkManager = network.NewBaseNetworkManager(rt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Colima Networking Not Configured
For Colima environments, the workstation creates a BaseNetworkManager instead of a ColimaNetworkManager. When ConfigureGuest() is called at line 163, the base implementation is just a no-op, so the Colima-specific networking setup (SSH configuration, iptables rules, Docker bridge detection) never happens. The network manager creation should check vm.driver and instantiate ColimaNetworkManager with the required dependencies (sshClient, secureShell, networkInterfaceProvider) when using Colima.
Factors out the dependency injector from the workstation package .
Signed-off-by: Ryan VanGundy 85766511+rmvangun@users.noreply.github.com
Note
Replaces the DI injector with direct runtime-based wiring across workstation, network, services, and virt, simplifying initialization (e.g., AssignIPs), updating constructors, and adjusting Docker/Colima handling and tests.
*runtime.Runtimeacrossworkstation,network,services, andvirt.Initializemethods; use direct fields fromRuntimeand new flows (e.g.,AssignIPs).NewWorkstation(rt, opts...)with optional overrides; auto-constructs deps (NetworkManager, Services, VM, ContainerRuntime, SSH).Up/Downsimplified (no explicit init calls); networking/DNS configured post runtime bring-up.SetServices.AssignIPsonBaseNetworkManager; default CIDR applied if missing.ColimaNetworkManagernow constructed with(rt, sshClient, secureShell, networkInterfaceProvider);ConfigureGuestuses default CIDR lookup.DNSService,RegistryService,GitLivereloadService,LocalstackService,TalosServicenow useNewXxxService(rt); rely onruntime.ProjectRootand config handler directly.DNSServiceaddsSetServicesand writes Corefile without shell project-root lookup.BaseVirttakesrt; remove explicit init; consumers read shell/config from runtime.ColimaVirt/DockerVirtupdated accordingly;DockerVirtdetermines compose command robustly and usesruntime.ProjectRoot.NewProjectcreates workstation viaworkstation.NewWorkstation(rt); initialization usesAssignIPs.AssignIPs, and removal of DI/Initialize; adapt mocks accordingly.Written by Cursor Bugbot for commit 211bf2e. This will update automatically on new commits. Configure here.