From 69332805756b6a2774d92b92f019f9c9a4f13396 Mon Sep 17 00:00:00 2001 From: Jean-Gabriel Gill-Couture Date: Thu, 8 Jan 2026 23:42:54 -0500 Subject: [PATCH] feat(helm): refactor helm execution to use topology-specific commands Refactors the `HelmChartInterpret` to move away from the `helm-wrapper-rs` crate in favor of a custom command builder pattern. This allows the `HelmCommand` trait to provide topology-specific configurations, such as `kubeconfig` and `kube-context`, directly to the `helm` CLI. - Implements `get_helm_command` for `K8sAnywhereTopology` to inject configuration flags. - Replaces `DefaultHelmExecutor` with a manual `Command` construction in `run_helm_command`. - Updates `HelmChartInterpret` to pass the topology through to repository and installation logic. - Cleans up unused imports and removes the temporary `HelmCommand` implementation for `LocalhostTopology`. --- .../topology/k8s_anywhere/k8s_anywhere.rs | 18 +- harmony/src/domain/topology/localhost.rs | 5 +- harmony/src/modules/helm/chart.rs | 170 +++++++----------- 3 files changed, 80 insertions(+), 113 deletions(-) diff --git a/harmony/src/domain/topology/k8s_anywhere/k8s_anywhere.rs b/harmony/src/domain/topology/k8s_anywhere/k8s_anywhere.rs index 72fa819..c3a6baa 100644 --- a/harmony/src/domain/topology/k8s_anywhere/k8s_anywhere.rs +++ b/harmony/src/domain/topology/k8s_anywhere/k8s_anywhere.rs @@ -1112,7 +1112,21 @@ impl MultiTargetTopology for K8sAnywhereTopology { } } -impl HelmCommand for K8sAnywhereTopology {} +impl HelmCommand for K8sAnywhereTopology { + fn get_helm_command(&self) -> Command { + let mut cmd = Command::new("helm"); + if let Some(k) = &self.config.kubeconfig { + cmd.args(["--kubeconfig", k]); + } + + if let Some(c) = &self.config.k8s_context { + cmd.args(["--kube-context", c]); + } + + info!("Using helm command {cmd:?}"); + cmd + } +} #[async_trait] impl TenantManager for K8sAnywhereTopology { @@ -1133,7 +1147,7 @@ impl TenantManager for K8sAnywhereTopology { #[async_trait] impl Ingress for K8sAnywhereTopology { async fn get_domain(&self, service: &str) -> Result { - use log::{debug, trace, warn}; + use log::{trace, warn}; let client = self.k8s_client().await?; diff --git a/harmony/src/domain/topology/localhost.rs b/harmony/src/domain/topology/localhost.rs index 667b3f8..3af4d62 100644 --- a/harmony/src/domain/topology/localhost.rs +++ b/harmony/src/domain/topology/localhost.rs @@ -2,7 +2,7 @@ use async_trait::async_trait; use derive_new::new; use serde::{Deserialize, Serialize}; -use super::{HelmCommand, PreparationError, PreparationOutcome, Topology}; +use super::{PreparationError, PreparationOutcome, Topology}; #[derive(new, Clone, Debug, Serialize, Deserialize)] pub struct LocalhostTopology; @@ -19,6 +19,3 @@ impl Topology for LocalhostTopology { }) } } - -// TODO: Delete this, temp for test -impl HelmCommand for LocalhostTopology {} diff --git a/harmony/src/modules/helm/chart.rs b/harmony/src/modules/helm/chart.rs index 4b678f1..d447126 100644 --- a/harmony/src/modules/helm/chart.rs +++ b/harmony/src/modules/helm/chart.rs @@ -6,15 +6,11 @@ use crate::topology::{HelmCommand, Topology}; use async_trait::async_trait; use harmony_types::id::Id; use harmony_types::net::Url; -use helm_wrapper_rs; -use helm_wrapper_rs::blocking::{DefaultHelmExecutor, HelmExecutor}; use log::{debug, info, warn}; pub use non_blank_string_rs::NonBlankString; use serde::Serialize; use std::collections::HashMap; -use std::path::Path; -use std::process::{Command, Output, Stdio}; -use std::str::FromStr; +use std::process::{Output, Stdio}; use temp_file::TempFile; #[derive(Debug, Clone, Serialize)] @@ -65,7 +61,7 @@ pub struct HelmChartInterpret { pub score: HelmChartScore, } impl HelmChartInterpret { - fn add_repo(&self) -> Result<(), InterpretError> { + fn add_repo(&self, topology: &T) -> Result<(), InterpretError> { let repo = match &self.score.repository { Some(repo) => repo, None => { @@ -84,7 +80,7 @@ impl HelmChartInterpret { add_args.push("--force-update"); } - let add_output = run_helm_command(&add_args)?; + let add_output = run_helm_command(topology, &add_args)?; let full_output = format!( "{}\n{}", String::from_utf8_lossy(&add_output.stdout), @@ -100,23 +96,19 @@ impl HelmChartInterpret { } } -fn run_helm_command(args: &[&str]) -> Result { - let command_str = format!("helm {}", args.join(" ")); - debug!( - "Got KUBECONFIG: `{}`", - std::env::var("KUBECONFIG").unwrap_or("".to_string()) - ); - debug!("Running Helm command: `{}`", command_str); +fn run_helm_command(topology: &T, args: &[&str]) -> Result { + let mut helm_cmd = topology.get_helm_command(); + helm_cmd.args(args); - let output = Command::new("helm") - .args(args) + debug!("Running Helm command: `{:?}`", helm_cmd); + + let output = helm_cmd .stdout(Stdio::piped()) .stderr(Stdio::piped()) .output() .map_err(|e| { InterpretError::new(format!( - "Failed to execute helm command '{}': {}. Is helm installed and in PATH?", - command_str, e + "Failed to execute helm command '{helm_cmd:?}': {e}. Is helm installed and in PATH?", )) })?; @@ -124,13 +116,13 @@ fn run_helm_command(args: &[&str]) -> Result { let stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); warn!( - "Helm command `{}` failed with status: {}\nStdout:\n{}\nStderr:\n{}", - command_str, output.status, stdout, stderr + "Helm command `{helm_cmd:?}` failed with status: {}\nStdout:\n{stdout}\nStderr:\n{stderr}", + output.status ); } else { debug!( - "Helm command `{}` finished successfully. Status: {}", - command_str, output.status + "Helm command `{helm_cmd:?}` finished successfully. Status: {}", + output.status ); } @@ -142,7 +134,7 @@ impl Interpret for HelmChartInterpret { async fn execute( &self, _inventory: &Inventory, - _topology: &T, + topology: &T, ) -> Result { let ns = self .score @@ -150,98 +142,62 @@ impl Interpret for HelmChartInterpret { .as_ref() .unwrap_or_else(|| todo!("Get namespace from active kubernetes cluster")); - let tf: TempFile; - let yaml_path: Option<&Path> = match self.score.values_yaml.as_ref() { - Some(yaml_str) => { - tf = temp_file::with_contents(yaml_str.as_bytes()); - debug!( - "values yaml string for chart {} :\n {yaml_str}", - self.score.chart_name - ); - Some(tf.path()) - } - None => None, + self.add_repo(topology)?; + + let mut args = if self.score.install_only { + vec!["install"] + } else { + vec!["upgrade", "--install"] }; - self.add_repo()?; - - let helm_executor = DefaultHelmExecutor::new_with_opts( - &NonBlankString::from_str("helm").unwrap(), - None, - 900, - false, - false, - ); - - let mut helm_options = Vec::new(); - if self.score.create_namespace { - helm_options.push(NonBlankString::from_str("--create-namespace").unwrap()); - } - - if self.score.install_only { - let chart_list = match helm_executor.list(Some(ns)) { - Ok(charts) => charts, - Err(e) => { - return Err(InterpretError::new(format!( - "Failed to list scores in namespace {:?} because of error : {}", - self.score.namespace, e - ))); - } - }; - - if chart_list - .iter() - .any(|item| item.name == self.score.release_name.to_string()) - { - info!( - "Release '{}' already exists in namespace '{}'. Skipping installation as install_only is true.", - self.score.release_name, ns - ); - - return Ok(Outcome::success(format!( - "Helm Chart '{}' already installed to namespace {ns} and install_only=true", - self.score.release_name - ))); - } else { - info!( - "Release '{}' not found in namespace '{}'. Proceeding with installation.", - self.score.release_name, ns - ); - } - } - - let res = helm_executor.install_or_upgrade( - ns, + args.extend(vec![ &self.score.release_name, &self.score.chart_name, - self.score.chart_version.as_ref(), - self.score.values_overrides.as_ref(), - yaml_path, - Some(&helm_options), - ); + "--namespace", + &ns, + ]); - let status = match res { - Ok(status) => status, - Err(err) => return Err(InterpretError::new(err.to_string())), - }; + if self.score.create_namespace { + args.push("--create-namespace"); + } - match status { - helm_wrapper_rs::HelmDeployStatus::Deployed => Ok(Outcome::success(format!( + if let Some(version) = &self.score.chart_version { + args.push("--version"); + args.push(&version); + } + + let tf: TempFile; + if let Some(yaml_str) = &self.score.values_yaml { + tf = temp_file::with_contents(yaml_str.as_bytes()); + args.push("--values"); + args.push(tf.path().to_str().unwrap()); + } + + let overrides_strings: Vec; + if let Some(overrides) = &self.score.values_overrides { + overrides_strings = overrides + .iter() + .map(|(key, value)| format!("{key}={value}")) + .collect(); + for o in overrides_strings.iter() { + args.push("--set"); + args.push(&o); + } + } + + let output = run_helm_command(topology, &args)?; + + if output.status.success() { + Ok(Outcome::success(format!( "Helm Chart {} deployed", self.score.release_name - ))), - helm_wrapper_rs::HelmDeployStatus::PendingInstall => Ok(Outcome::running(format!( - "Helm Chart {} pending install...", - self.score.release_name - ))), - helm_wrapper_rs::HelmDeployStatus::PendingUpgrade => Ok(Outcome::running(format!( - "Helm Chart {} pending upgrade...", - self.score.release_name - ))), - helm_wrapper_rs::HelmDeployStatus::Failed => Err(InterpretError::new(format!( - "Helm Chart {} installation failed", - self.score.release_name - ))), + ))) + } else { + Err(InterpretError::new(format!( + "Helm Chart {} installation failed: {}", + self.score.release_name, + String::from_utf8_lossy(&output.stderr) + ))) } }