refactor: move command.rs to build, use ConfigFileNames, remove v2 / upstream references

maltesander · maltesander · commit 4a66071a5cee · 2026-06-18T10:33:52.000+02:00
diff --git a/rust/operator-binary/src/authentication/oidc/mod.rs b/rust/operator-binary/src/authentication/oidc/mod.rs
@@ -8,7 +8,7 @@ use stackable_operator::{
 
 use crate::{
     authentication::TrinoAuthenticationConfig,
-    command,
+    controller::build::command,
     crd::{STACKABLE_CLIENT_TLS_DIR, TrinoRole},
 };
 
diff --git a/rust/operator-binary/src/config/client_protocol.rs b/rust/operator-binary/src/config/client_protocol.rs
@@ -1,4 +1,4 @@
-// Consolidate Trino S3 properties in a single reusable struct.
+//! Resolves the Trino client (spooling) protocol configuration.
 
 use std::collections::BTreeMap;
 
diff --git a/rust/operator-binary/src/config/s3.rs b/rust/operator-binary/src/config/s3.rs
@@ -8,7 +8,7 @@ use stackable_operator::{
     k8s_openapi::api::core::v1::{Volume, VolumeMount},
 };
 
-use crate::{command, crd::STACKABLE_CLIENT_TLS_DIR};
+use crate::{controller::build::command, crd::STACKABLE_CLIENT_TLS_DIR};
 
 #[derive(Snafu, Debug)]
 pub enum Error {
diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs
@@ -1,5 +1,6 @@
 //! Builders that turn a `ValidatedCluster` into Kubernetes resource contents.
 
+pub mod command;
 pub mod graceful_shutdown;
 pub mod ports;
 pub mod properties;
diff --git a/rust/operator-binary/src/controller/build/command.rs b/rust/operator-binary/src/controller/build/command.rs
@@ -10,7 +10,7 @@ use crate::{
     authentication::TrinoAuthenticationConfig,
     catalog::config::CatalogConfig,
     config::{client_protocol, fault_tolerant_execution},
-    controller::{ValidatedCluster, ValidatedTrinoConfig},
+    controller::{ValidatedCluster, ValidatedTrinoConfig, build::properties::ConfigFileName},
     crd::{
         CONFIG_DIR_NAME, Container, RW_CONFIG_DIR_NAME, STACKABLE_CLIENT_TLS_DIR,
         STACKABLE_INTERNAL_TLS_DIR, STACKABLE_MOUNT_INTERNAL_TLS_DIR,
@@ -20,12 +20,6 @@ use crate::{
     trino_controller::{STACKABLE_LOG_CONFIG_DIR, STACKABLE_LOG_DIR},
 };
 
-// TODO: replace with build::properties::ConfigFileName once command.rs moves under build/
-//       (with the StatefulSet builder migration).
-const LOG_PROPERTIES: &str = "log.properties";
-const EXCHANGE_MANAGER_PROPERTIES: &str = "exchange-manager.properties";
-const SPOOLING_MANAGER_PROPERTIES: &str = "spooling-manager.properties";
-
 pub fn container_prepare_args(
     cluster: &ValidatedCluster,
     catalogs: &[CatalogConfig],
@@ -37,13 +31,14 @@ pub fn container_prepare_args(
 
     // Copy custom logging provided `log.properties` to rw config
     if let ValidatedContainerLogConfigChoice::Custom(_) = merged_config.logging.trino_container {
+        let log_properties = ConfigFileName::Log;
         // copy config files to a writeable empty folder
         args.push(format!(
-            "echo copying {STACKABLE_LOG_CONFIG_DIR}/{LOG_PROPERTIES} {rw_conf}/{LOG_PROPERTIES}",
+            "echo copying {STACKABLE_LOG_CONFIG_DIR}/{log_properties} {rw_conf}/{log_properties}",
             rw_conf = RW_CONFIG_DIR_NAME
         ));
         args.push(format!(
-            "cp -RL {STACKABLE_LOG_CONFIG_DIR}/{LOG_PROPERTIES} {rw_conf}/{LOG_PROPERTIES}",
+            "cp -RL {STACKABLE_LOG_CONFIG_DIR}/{log_properties} {rw_conf}/{log_properties}",
             rw_conf = RW_CONFIG_DIR_NAME
         ));
     }
@@ -118,13 +113,19 @@ pub fn container_trino_args(
     // Resolve credentials for fault tolerant execution exchange manager if needed
     args.push(format!(
         "test -f {rw_exchange_manager_config_file} && config-utils template {rw_exchange_manager_config_file}",
-        rw_exchange_manager_config_file = format!("{RW_CONFIG_DIR_NAME}/{EXCHANGE_MANAGER_PROPERTIES}")
+        rw_exchange_manager_config_file = format!(
+            "{RW_CONFIG_DIR_NAME}/{exchange_manager}",
+            exchange_manager = ConfigFileName::ExchangeManager
+        )
     ));
 
     // Resolve credentials for spooling manager if needed
     args.push(format!(
         "test -f {rw_spooling_config_file} && config-utils template {rw_spooling_config_file}",
-        rw_spooling_config_file = format!("{RW_CONFIG_DIR_NAME}/{SPOOLING_MANAGER_PROPERTIES}")
+        rw_spooling_config_file = format!(
+            "{RW_CONFIG_DIR_NAME}/{spooling_manager}",
+            spooling_manager = ConfigFileName::SpoolingManager
+        )
     ));
 
     args.push("set -x".to_string());
diff --git a/rust/operator-binary/src/controller/build/properties/config_properties.rs b/rust/operator-binary/src/controller/build/properties/config_properties.rs
@@ -51,7 +51,7 @@ const LOG_COMPRESSION: &str = "log.compression";
 const LOG_MAX_SIZE: &str = "log.max-size";
 const LOG_MAX_TOTAL_SIZE: &str = "log.max-total-size";
 
-// Defaults migrated from deploy/config-spec/properties.yaml.
+// Default values for `config.properties`.
 const DEFAULT_QUERY_MAX_MEMORY: &str = "50GB";
 const DEFAULT_NODE_SCHEDULER_INCLUDE_COORDINATOR: &str = "false";
 
@@ -134,8 +134,7 @@ pub fn build(
         format!("${{ENV:{ENV_INTERNAL_SECRET}}}"),
     );
 
-    // TLS gating — mirrors the existing compute_files logic, including the
-    // authentication-requires-TLS check.
+    // TLS gating, including the authentication-requires-TLS check.
     let server_tls_enabled = cluster.server_tls_enabled();
     let internal_tls_enabled = cluster.internal_tls_enabled();
     if cluster.cluster_config.authentication_enabled() && !server_tls_enabled {
diff --git a/rust/operator-binary/src/controller/build/properties/mod.rs b/rust/operator-binary/src/controller/build/properties/mod.rs
@@ -1,8 +1,7 @@
 //! Per-file builders for Trino `.properties` files.
 //!
 //! Each `<file>.rs` module produces the rendered key/value pairs for one
-//! Trino config file. The shared
-//! [`stackable_operator::v2::config_file_writer`] serializes the map to the
+//! Trino config file. The shared config-file writer serializes the map to the
 //! Java-properties on-wire format.
 
 pub mod access_control_properties;
@@ -101,8 +100,7 @@ mod tests {
     }
 
     /// The escape behaviours pinned by the kuttl smoke snapshot
-    /// (`tests/templates/kuttl/smoke/14-assert.yaml.j2`); previously asserted
-    /// against the hand-rolled writer, now against the shared one.
+    /// (`tests/templates/kuttl/smoke/14-assert.yaml.j2`).
     #[test]
     fn kuttl_pinned_escapes_are_stable() {
         assert_eq!(
diff --git a/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs b/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs
@@ -3,7 +3,7 @@
 /// The Vector agent configuration (`vector.yaml`).
 ///
 /// It is templated with environment variables (`${LOG_DIR}`, `${NAMESPACE}`, …) that the
-/// `v2` Vector container injects at runtime, so the same file content is used for every
+/// Vector container injects at runtime, so the same file content is used for every
 /// rolegroup.
 const VECTOR_CONFIG: &str = include_str!("vector.yaml");
 
diff --git a/rust/operator-binary/src/controller/build/resource/config_map.rs b/rust/operator-binary/src/controller/build/resource/config_map.rs
@@ -80,8 +80,7 @@ pub fn build_rolegroup_config_map(
 
     let mut data: BTreeMap<String, String> = BTreeMap::new();
 
-    // Auth files (e.g. password-authenticator file contents) — inserted FIRST
-    // to match the legacy precedence.
+    // Auth files (e.g. password-authenticator file contents).
     for (file_name, props) in cluster.cluster_config.authentication.config_files(role) {
         let rendered =
             to_java_properties_string(props.iter()).with_context(|_| WritePropertiesSnafu {
diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs
@@ -39,12 +39,14 @@ use stackable_operator::{
 
 use crate::{
     authorization::opa::OPA_TLS_VOLUME_NAME,
-    command,
     controller::{
         TrinoRoleGroupConfig, ValidatedCluster, build,
-        build::resource::listener::{
-            LISTENER_VOLUME_DIR, LISTENER_VOLUME_NAME, build_group_listener_pvc,
-            group_listener_name, secret_volume_listener_scope,
+        build::{
+            command,
+            resource::listener::{
+                LISTENER_VOLUME_DIR, LISTENER_VOLUME_NAME, build_group_listener_pvc,
+                group_listener_name, secret_volume_listener_scope,
+            },
         },
     },
     crd::{
@@ -426,9 +428,8 @@ pub fn build_rolegroup_statefulset(
         .security_context(PodSecurityContextBuilder::new().fs_group(1000).build());
 
     let mut pod_template = pod_builder.build_template();
-    // `pod_overrides` already carries the merged role + role-group overrides (see
-    // `with_validated_config`), so a single merge here is equivalent to the previous
-    // role-then-role-group sequence.
+    // `pod_overrides` already carries the merged role + role-group overrides, so a single merge
+    // here applies both.
     pod_template.merge_from(role_group_config.pod_overrides.clone());
 
     let annotations = restarter_ignore_secret_annotations(
diff --git a/rust/operator-binary/src/controller/dereference.rs b/rust/operator-binary/src/controller/dereference.rs
@@ -1,10 +1,7 @@
 //! The dereference step in the TrinoCluster controller
 //!
 //! Fetches all Kubernetes objects referenced by the TrinoCluster spec and returns them in
-//! [`DereferencedObjects`]. The functions called here (`CatalogConfig::from_catalog`,
-//! `TrinoOpaConfig::from_opa_config`, `ResolvedFaultTolerantExecutionConfig::from_config`,
-//! `ResolvedClientProtocolConfig::from_config`) currently mix fetching and validation; their
-//! outputs are treated as "dereferenced" for now. Splitting those helpers is a follow-up.
+//! [`DereferencedObjects`].
 
 use std::{num::ParseIntError, str::FromStr};
 
@@ -71,8 +68,7 @@ pub enum Error {
 
 type Result<T, E = Error> = std::result::Result<T, E>;
 
-/// Kubernetes objects referenced from the TrinoCluster spec, already fetched (and, for now, partly
-/// validated by the existing helper functions).
+/// Kubernetes objects referenced from the TrinoCluster spec, fetched from the cluster.
 pub struct DereferencedObjects {
     pub resolved_authentication_classes: Vec<ResolvedAuthenticationClassRef>,
     pub catalog_definitions: Vec<catalog::v1alpha1::TrinoCatalog>,
diff --git a/rust/operator-binary/src/crd/client_protocol.rs b/rust/operator-binary/src/crd/client_protocol.rs
@@ -1,5 +1,5 @@
-/// This module manages the client protocol properties, especially the for spooling.
-/// Trino documentation is available here: https://trino.io/docs/current/client/client-protocol.html
+//! This module manages the client protocol properties, especially for spooling.
+//! Trino documentation is available here: <https://trino.io/docs/current/client/client-protocol.html>
 use serde::{Deserialize, Serialize};
 use stackable_operator::schemars::{self, JsonSchema};
 
diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs
@@ -607,8 +607,8 @@ mod tests {
 
     use super::*;
 
-    /// The user-provided server TLS SecretClass as `Option<&str>` (mirrors the former
-    /// `TrinoCluster::get_server_tls`, kept here only for these CRD-defaulting assertions).
+    /// The user-provided server TLS SecretClass as `Option<&str>`, used by these CRD-defaulting
+    /// assertions.
     fn server_secret_class(trino: &v1alpha1::TrinoCluster) -> Option<&str> {
         trino
             .spec
diff --git a/rust/operator-binary/src/main.rs b/rust/operator-binary/src/main.rs
@@ -1,5 +1,4 @@
 // TODO: Look into how to properly resolve `clippy::result_large_err`.
-// This will need changes in our and upstream error types.
 #![allow(clippy::result_large_err)]
 
 use std::sync::Arc;
@@ -45,7 +44,6 @@ use crate::{
 mod authentication;
 mod authorization;
 mod catalog;
-mod command;
 mod config;
 mod controller;
 mod crd;
diff --git a/rust/operator-binary/src/trino_controller.rs b/rust/operator-binary/src/trino_controller.rs
@@ -463,11 +463,10 @@ mod tests {
             ),
             None => None,
         };
-        // For OPA, the legacy helper used a hard-coded `TrinoOpaConfig` literal
-        // rather than resolving from cluster config; mirror that here so that
-        // `test_access_control_overrides` does not need a Kubernetes client and
-        // so that `test_config_overrides` keeps observing an
-        // `access-control.properties` entry in the rendered ConfigMap.
+        // Build a `TrinoOpaConfig` literal directly instead of resolving it from cluster config,
+        // so that `test_access_control_overrides` does not need a Kubernetes client and
+        // `test_config_overrides` still observes an `access-control.properties` entry in the
+        // rendered ConfigMap.
         let trino_opa_config = Some(TrinoOpaConfig {
             non_batched_connection_string:
                 "http://simple-opa.default.svc.cluster.local:8081/v1/data/my-product/allow"

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-// Consolidate Trino S3 properties in a single reusable struct.`
	`1`	`+//! Resolves the Trino client (spooling) protocol configuration.`
`2`	`2`
`3`	`3`	`use std::collections::BTreeMap;`
`4`	`4`