-
Notifications
You must be signed in to change notification settings - Fork 0
refactor(provisioner): Remove dependency injector #1873
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
We are phasing out the dependency injector. This PR removes it from the provisioner, instead using explicit dependencies passed to constructors. Signed-off-by: Ryan VanGundy <85766511+rmvangun@users.noreply.github.com>
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 PR is being reviewed by Cursor Bugbot
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: Method fails instead of skipping disabled Terraform.
The Down method returns an error when TerraformStack is nil, but TerraformStack is only created when terraform.enabled config is true. This causes Down to fail even in valid configurations where terraform is disabled. The method should skip terraform operations when the stack is not configured rather than treating it as an error condition.
pkg/provisioner/provisioner.go#L117-L129
cli/pkg/provisioner/provisioner.go
Lines 117 to 129 in f95ffd1
| // Down orchestrates the high-level infrastructure teardown process. It executes terraform destroy operations | |
| // for all components in the stack in reverse dependency order. Components with Destroy set to false are skipped. | |
| // This method coordinates terraform, kubernetes, and cluster operations to tear down the infrastructure. | |
| // Initializes components as needed. The blueprint parameter is required. Returns an error if any destroy operation fails. | |
| func (i *Provisioner) Down(blueprint *blueprintv1alpha1.Blueprint) error { | |
| if blueprint == nil { | |
| return fmt.Errorf("blueprint not provided") | |
| } | |
| if i.TerraformStack == nil { | |
| return fmt.Errorf("terraform stack not configured") | |
| } |
Signed-off-by: Ryan VanGundy <85766511+rmvangun@users.noreply.github.com>
|
|
||
| if err := p.Runtime.LoadEnvironment(false); err != nil { | ||
| return fmt.Errorf("failed to load environment: %w", err) | ||
| } |
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: Double Environment Initialization
The Configure method calls LoadEnvironment(false), but Initialize already calls LoadEnvironment(true). Since all commands call Configure followed by Initialize, LoadEnvironment executes twice for every command, causing redundant component initialization and potential side effects from re-initializing environment printers, tools manager, and secrets providers.
Signed-off-by: Ryan VanGundy 85766511+rmvangun@users.noreply.github.com
Note
Eliminates dependency-injector usage across provisioner, terraform stack, Kubernetes manager, and Talos client by passing explicit runtime/clients, updates CLI flows and tests accordingly.
pkg/provisioner/Provisioner: embeds*runtime.Runtime; constructorNewProvisioner(rt, blueprintHandler, opts...)creates deps explicitly (optional Terraform based onterraform.enabled), andUp/Downno-op when Terraform disabled.pkg/provisioner/kubernetes/BaseKubernetesManager:NewKubernetesManager(client)now accepts aKubernetesClient; removedInitialize; logic unchanged (SSA apply, waits, health checks).pkg/provisioner/terraform:BaseStack/WindsorStacknow take*runtime.Runtimeandblueprint.Handler; removedInitialize; useruntime.EnvPrinters.TerraformEnvandruntime.Shellfor TF ops; path/source resolution preserved.pkg/provisioner/cluster/TalosClusterClient: removed injector; plain constructorNewTalosClusterClient().pkg/project/Project:NewProject(injector, contextName, opts...)buildsComposerandProvisionerwith new signatures; loads env earlier inConfigure.cmd/check node-health,up,init,bundle,push): constructRuntime/Composer/Provisionerper new APIs; add context override keys (projectOverrides,composerOverrides).upprints success message; workstationUpno longer prints success directly.Initializecalls; added/adjusted mocks to pass explicit clients/handlers and runtime env printers.Written by Cursor Bugbot for commit 84609a0. This will update automatically on new commits. Configure here.