feat: separate product level configuration

Author: dervoeti

rust/patchable/README.md

@@ -31,20 +31,36 @@ For more details, run `cargo patchable --help`.
 
 ## Configuration
 
-Patchable stores a per-version file in `docker-images/<PRODUCT>/stackable/patches/<VERSION>/patchable.toml`.
-It currently recognizes the following keys:
+Patchable uses a two-level configuration system:
 
+1. A product-level config file at `docker-images/<PRODUCT>/stackable/patches/patchable.toml`
+2. A version-level config file at `docker-images/<PRODUCT>/stackable/patches/<VERSION>/patchable.toml`
+
+The product-level config typically contains:
 - `upstream` - the URL of the upstream repository (such as `https://github.com/apache/druid.git`)
-- `base` - the commit hash of the upstream base commit (such as `7cffb81a8e124d5f218f9af16ad685acf5e9c67c`)
+- `mirror` - optional URL of a mirror repository (such as `https://github.com/stackabletech/druid.git`)
 
-### Template
+The version-level config typically contains:
+- `base` - the commit hash of the upstream base commit
 
-Instead of creating this manually, run `patchable init`:
+Fields in the version-level config override those in the product-level config if both are specified.
+
+### Template
 
+If you're adding a completely new product, you need to create the product-level config once:
 ```toml
-cargo patchable init druid 28.0.0 --upstream=https://github.com/apache/druid.git --base=druid-28.0.0
+# docker-images/druid/stackable/patches/patchable.toml
+upstream = "https://github.com/apache/druid.git"
+mirror = "https://github.com/stackabletech/druid.git"
 ```
 
+If you just want to add a new version, simply initiatilize it with patchable:
+```
+cargo patchable init druid 28.0.0 --base=druid-28.0.0 --mirror
+```
+
+Using the `--mirror` flag will push the base ref to the mirror URL specified in the product-level config.
+
 ## Glossary
 
 - Images repo/directory - The checkout of stackabletech/docker-images

rust/patchable/src/main.rs

@@ -26,9 +26,9 @@ struct ProductVersion {
 
 #[derive(Deserialize, Serialize)]
 struct ProductVersionConfig {
-    upstream: String,
-    #[serde(with = "utils::oid_serde")]
-    base: Oid,
+    upstream: Option<String>,
+    #[serde(with = "utils::option_oid_serde")]
+    base: Option<Oid>,
     mirror: Option<String>,
 }
 
@@ -39,15 +39,77 @@ struct ProductVersionContext {
 
 impl ProductVersionContext {
     fn load_config(&self) -> Result<ProductVersionConfig> {
-        let path = &self.config_path();
-        tracing::info!(
-            config.path = ?path,
-            "loading config"
-        );
-        toml::from_str::<ProductVersionConfig>(
-            &std::fs::read_to_string(path).context(LoadConfigSnafu { path })?,
-        )
-        .context(ParseConfigSnafu { path })
+        let version_config_path = &self.version_config_path();
+        let product_config_path = &self.product_config_path();
+
+        // Load product-level config (required)
+        let product_config: ProductVersionConfig = {
+            tracing::info!(
+                config.path = ?product_config_path,
+                "loading product-level config"
+            );
+            toml::from_str::<ProductVersionConfig>(
+                &std::fs::read_to_string(product_config_path).context(LoadConfigSnafu {
+                    path: product_config_path,
+                })?,
+            )
+            .context(ParseConfigSnafu {
+                path: product_config_path,
+            })?
+        };
+
+        // Load version-level config (optional)
+        let version_config: Option<ProductVersionConfig> =
+            match std::fs::read_to_string(version_config_path) {
+                Ok(s) => {
+                    tracing::info!(
+                        config.path = ?version_config_path,
+                        "loading version-level config"
+                    );
+                    Some(
+                        toml::from_str::<ProductVersionConfig>(&s).context(ParseConfigSnafu {
+                            path: version_config_path,
+                        })?,
+                    )
+                }
+                Err(e) if e.kind() == std::io::ErrorKind::NotFound => None,
+                Err(e) => {
+                    return Err(e).context(LoadConfigSnafu {
+                        path: version_config_path,
+                    })
+                }
+            };
+        // Merge: version-level overrides product-level, validate required fields afterwards
+        let merged = ProductVersionConfig {
+            upstream: version_config
+                .as_ref()
+                .and_then(|c| c.upstream.clone())
+                .or(product_config.upstream.clone())
+                .or_else(|| {
+                    Err(ValidateConfigSnafu {
+                        reason: "upstream is required".to_string(),
+                        paths: vec![product_config_path, version_config_path],
+                    })
+                    .ok()
+                }),
+            base: version_config
+                .as_ref()
+                .and_then(|c| c.base)
+                .or(product_config.base)
+                .or_else(|| {
+                    Err(ValidateConfigSnafu {
+                        reason: "base commit is required".to_string(),
+                        paths: vec![product_config_path, version_config_path],
+                    })
+                    .ok()
+                }),
+            mirror: version_config
+                .as_ref()
+                .and_then(|c| c.mirror.clone())
+                .or(product_config.mirror.clone()),
+        };
+
+        Ok(merged)
     }
 
     /// The root directory for files related to the product (across all versions).
@@ -63,10 +125,15 @@ impl ProductVersionContext {
     }
 
     /// The patchable configuration file for the product version.
-    fn config_path(&self) -> PathBuf {
+    fn version_config_path(&self) -> PathBuf {
         self.patch_dir().join("patchable.toml")
     }
 
+    /// The product-level patchable configuration file
+    fn product_config_path(&self) -> PathBuf {
+        self.product_dir().join("stackable/patches/patchable.toml")
+    }
+
     /// The directory containing all ephemeral data used by patchable for the product (across all versions).
     ///
     /// Should be gitignored, and can safely be deleted as long as all relevant versions have been `patchable export`ed.
@@ -190,6 +257,8 @@ pub enum Error {
         source: toml::de::Error,
         path: PathBuf,
     },
+    #[snafu(display("failed to validate config: {reason:?} (used config files: {paths:?})"))]
+    ValidateConfig { reason: String, paths: Vec<PathBuf> },
     #[snafu(display("failed to serialize config"))]
     SerializeConfig { source: toml::ser::Error },
     #[snafu(display("failed to create patch dir at {path:?}"))]
@@ -203,8 +272,6 @@ pub enum Error {
         path: PathBuf,
     },
 
-    #[snafu(display("failed to parse upstream URL {url:?} to extract repository name"))]
-    ParseUpstreamUrl { url: String },
     #[snafu(display("failed to add temporary mirror remote for {url:?}"))]
     AddMirrorRemote { source: git2::Error, url: String },
     #[snafu(display("failed to push commit {commit} (as {refspec}) to mirror {url:?}"))]
@@ -302,10 +369,14 @@ fn main() -> Result<()> {
             let product_repo = repo::ensure_bare_repo(&product_repo_root)
                 .context(OpenProductRepoForCheckoutSnafu)?;
 
+            // Presence of `base` and `upstream` already validated by load_config, so unwrap is safe here
             let base_commit = repo::resolve_and_fetch_commitish(
                 &product_repo,
-                &config.base.to_string(),
-                config.mirror.as_deref().unwrap_or(&config.upstream),
+                &config.base.unwrap().to_string(),
+                config
+                    .mirror
+                    .as_deref()
+                    .unwrap_or(config.upstream.as_ref().unwrap()),
             )
             .context(FetchBaseCommitSnafu)?;
             let base_branch = ctx.base_branch();
@@ -378,7 +449,8 @@ fn main() -> Result<()> {
                     path: product_worktree_root,
                 })?;
 
-            let base_commit = config.base;
+            // Presence of `base` already validated by load_config, so unwrap is safe here
+            let base_commit = config.base.unwrap();
             let original_leaf_commit = product_version_repo
                 .head()
                 .and_then(|c| c.peel_to_commit())
@@ -437,43 +509,45 @@ fn main() -> Result<()> {
             // --base can be a reference, but patchable.toml should always have a resolved commit id,
             // so that it cannot be changed under our feet (without us knowing so, anyway...).
             tracing::info!(?base, "resolving base commit-ish");
-            let base_commit = repo::resolve_and_fetch_commitish(&product_repo, &base, &upstream).context(FetchBaseCommitSnafu)?;
+            let base_commit = repo::resolve_and_fetch_commitish(&product_repo, &base, &upstream)
+                .context(FetchBaseCommitSnafu)?;
             let mut upstream_mirror = None;
 
             if mirror {
-                // Parse e.g. "https://github.com/apache/druid.git" into "druid"
-                let repo_name = upstream.split('/').last().map(|repo| repo.trim_end_matches(".git")).filter(|name| !name.is_empty()).context(ParseUpstreamUrlSnafu { url: &upstream })?;
-
-                let mirror_url = format!("https://github.com/stackabletech/{repo_name}.git");
-                tracing::info!(mirror_url, "using mirror repository");
+                // Load mirror URL from product-level config
+                let mirror_url = ctx.load_config()?.mirror.context(ValidateConfigSnafu {
+                        reason: "mirror repository needs to be specified in product config when using --mirror flag".to_string(),
+                        paths: vec![ctx.product_config_path()],
+                    })?;
 
                 // Add mirror remote
-                let mut mirror_remote = product_repo
-                    .remote_anonymous(&mirror_url)
-                    .context(AddMirrorRemoteSnafu { url: mirror_url.clone() })?;
+                let mut mirror_remote =
+                    product_repo
+                        .remote_anonymous(&mirror_url)
+                        .context(AddMirrorRemoteSnafu {
+                            url: mirror_url.clone(),
+                        })?;
 
                 // Push the base commit to the mirror
                 tracing::info!(commit = %base_commit, base = base, url = mirror_url, "pushing commit to mirror");
                 let mut callbacks = git2::RemoteCallbacks::new();
                 callbacks.credentials(|url, username_from_url, _allowed_types| {
                     git2::Cred::credential_helper(
-                        &git2::Config::open_default().unwrap(), // Use default git config
+                        &git2::Config::open_default()
+                            .expect("failed to open default Git configuration"), // Use default git config,
                         url,
                         username_from_url,
                     )
                 });
 
                 // Add progress tracking for push operation
-                let (span_push, mut quant_push) = utils::setup_progress_tracking(tracing::info_span!("pushing"));
+                let (span_push, mut quant_push) =
+                    utils::setup_progress_tracking(tracing::info_span!("pushing"));
                 let _ = span_push.enter();
 
                 callbacks.push_transfer_progress(move |current, total, _| {
                     if total > 0 {
-                        quant_push.update_span_progress(
-                            current,
-                            total,
-                            &span_push,
-                        );
+                        quant_push.update_span_progress(current, total, &span_push);
                     }
                 });
 
@@ -486,7 +560,7 @@ fn main() -> Result<()> {
                 mirror_remote
                     .push(&[&refspec], Some(&mut push_options))
                     .context(PushToMirrorSnafu {
-                        url: &mirror_url,
+                        url: mirror_url.clone(),
                         refspec: &refspec,
                         commit: base_commit,
                     })?;
@@ -500,11 +574,11 @@ fn main() -> Result<()> {
 
             tracing::info!("saving configuration");
             let config = ProductVersionConfig {
-                upstream,
+                upstream: Some(upstream),
                 mirror: upstream_mirror,
-                base: base_commit,
+                base: Some(base_commit),
             };
-            let config_path = ctx.config_path();
+            let config_path = ctx.version_config_path();
             if let Some(config_dir) = config_path.parent() {
                 std::fs::create_dir_all(config_dir)
                     .context(CreatePatchDirSnafu { path: config_dir })?;

rust/patchable/src/utils.rs

@@ -72,19 +72,22 @@ pub fn raw_git_cmd(repo: &Repository) -> std::process::Command {
     cmd
 }
 
-/// Implements (equivalents of) the [`serde`] traits over [`git2::Oid`].
+/// Implements (equivalents of) the [`serde`] traits over [`Option<git2::Oid>`].
 ///
 /// For use with `#[serde(with = ...)]`.
-pub mod oid_serde {
+pub mod option_oid_serde {
     use git2::Oid;
     use serde::{Deserialize, Deserializer, Serialize, Serializer};
 
-    pub fn serialize<S: Serializer>(value: &Oid, ser: S) -> Result<S::Ok, S::Error> {
-        value.to_string().serialize(ser)
+    pub fn serialize<S: Serializer>(value: &Option<Oid>, ser: S) -> Result<S::Ok, S::Error> {
+        value.as_ref().map(|oid| oid.to_string()).serialize(ser)
     }
-    pub fn deserialize<'de, D: Deserializer<'de>>(de: D) -> Result<Oid, D::Error> {
-        String::deserialize(de)
-            .and_then(|oid| Oid::from_str(&oid).map_err(<D::Error as serde::de::Error>::custom))
+
+    pub fn deserialize<'de, D: Deserializer<'de>>(de: D) -> Result<Option<Oid>, D::Error> {
+        Option::<String>::deserialize(de).map(|opt| {
+            opt.map(|s| Oid::from_str(&s).map_err(<D::Error as serde::de::Error>::custom))
+                .transpose()
+        })?
     }
 }