From f44dee72a685a2bc209234d1d61c6c9888e86e69 Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Thu, 30 Jun 2022 03:26:25 -0400 Subject: [PATCH 1/3] tmpl8: add clap self-test --- tmpl8/src/main.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tmpl8/src/main.rs b/tmpl8/src/main.rs index 5164c9c..e8f7fc2 100644 --- a/tmpl8/src/main.rs +++ b/tmpl8/src/main.rs @@ -84,3 +84,14 @@ fn main() -> Result<()> { Cmd::GithubMatrix(c) => github::get_matrix(c), } } + +#[cfg(test)] +mod test { + use super::*; + use clap::IntoApp; + + #[test] + fn clap_app() { + Cmd::command().debug_assert() + } +} From 4417fa776f62390912db1fb81afda9ded28e5673 Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Wed, 29 Jun 2022 19:05:33 -0400 Subject: [PATCH 2/3] tmpl8: move stderr handle creation to a helper function --- tmpl8/src/render.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tmpl8/src/render.rs b/tmpl8/src/render.rs index 748ddf0..49dbaec 100644 --- a/tmpl8/src/render.rs +++ b/tmpl8/src/render.rs @@ -152,8 +152,6 @@ fn clean_rendered_output(output: &str) -> String { fn do_update_cache(cfg: &Config, cache_dir: &Path, force: bool) -> Result<()> { for (name, repo) in &cfg.repos { let path = cache_dir.join(name); - let stderr_fd = nix::unistd::dup(2_i32.as_raw_fd()).context("duplicating stderr")?; - let stderr = unsafe { Stdio::from_raw_fd(stderr_fd) }; match fs::metadata(&path) { Ok(meta) => { // Update the cache at most once per hour, unless forced @@ -163,7 +161,7 @@ fn do_update_cache(cfg: &Config, cache_dir: &Path, force: bool) -> Result<()> { run_command( Command::new("git") .arg("pull") - .stdout(stderr) + .stdout(stderr()?) .current_dir(&path), )?; filetime::set_file_mtime(&path, FileTime::now()) @@ -175,7 +173,7 @@ fn do_update_cache(cfg: &Config, cache_dir: &Path, force: bool) -> Result<()> { Command::new("git") .args(["clone", "--depth=1", &repo.url]) .arg(&path) - .stdout(stderr), + .stdout(stderr()?), )?; } Err(e) => return Err(e).with_context(|| format!("querying {}", path.display())), @@ -228,3 +226,8 @@ fn run_command(cmd: &mut Command) -> Result<()> { } Ok(()) } + +fn stderr() -> Result { + let stderr_fd = nix::unistd::dup(2_i32.as_raw_fd()).context("duplicating stderr")?; + Ok(unsafe { Stdio::from_raw_fd(stderr_fd) }) +} From d4f92803d3afd7c56595e22b18ba313807da95df Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Thu, 30 Jun 2022 04:31:27 -0400 Subject: [PATCH 3/3] tmpl8: allow diffing against branch in pending PR We've been diffing against the downstream main branch, which creates an incentive to merge upstream template PRs expeditiously to keep the diff clean. However, it also adds noise to the downstream Git history. It'd be better to let changes accumulate in a PR, and merge it just before starting a release. Add tmpl8 command-line options allowing it to sync the cache (and thus generate the diff) from the downstream PR branch instead. If that branch is missing, fall back to the downstream main branch as before. This is implemented via a regex and replacement string for repo URLs and a single branch name for all repos, since that's all we actually need right now. Hardcode arguments in the Makefile to match the values used by the GitHub workflow. --- Makefile | 9 +++- tmpl8/src/main.rs | 21 +++++++++ tmpl8/src/render.rs | 108 ++++++++++++++++++++++++++++++++++++-------- 3 files changed, 117 insertions(+), 21 deletions(-) diff --git a/Makefile b/Makefile index a224024..14cd924 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,11 @@ +FORK_ARGS = \ + --fork-regex /coreos/ \ + --fork-replacement /coreosbot-releng/ \ + --fork-branch repo-templates + .PHONY: diff diff: tmpl8 - tmpl8/target/debug/tmpl8 diff | less -R + tmpl8/target/debug/tmpl8 diff $(FORK_ARGS) | less -R .PHONY: output output: tmpl8 @@ -9,7 +14,7 @@ output: tmpl8 # Force sync of downstream repo cache .PHONY: sync sync: tmpl8 - tmpl8/target/debug/tmpl8 update-cache + tmpl8/target/debug/tmpl8 update-cache $(FORK_ARGS) .PHONY: tmpl8 tmpl8: diff --git a/tmpl8/src/main.rs b/tmpl8/src/main.rs index e8f7fc2..2da29a5 100644 --- a/tmpl8/src/main.rs +++ b/tmpl8/src/main.rs @@ -16,6 +16,7 @@ use std::path::PathBuf; use anyhow::Result; use clap::Parser; +use regex::Regex; mod github; mod render; @@ -54,6 +55,8 @@ struct DiffArgs { /// Config file #[clap(short = 'c', long, value_name = "file", default_value = "config.yaml")] config: PathBuf, + #[clap(flatten)] + fork: ForkArgs, /// Disable color output #[clap(short = 'n', long)] no_color: bool, @@ -64,6 +67,24 @@ struct UpdateCacheArgs { /// Config file #[clap(short = 'c', long, value_name = "file", default_value = "config.yaml")] config: PathBuf, + #[clap(flatten)] + fork: ForkArgs, +} + +#[derive(Debug, Parser)] +struct ForkArgs { + /// Regex for the upstream part of repo URL + #[clap(long = "fork-regex", value_name = "regex")] + #[clap(requires_all = &["replacement", "branch"])] + regex: Option, + /// Replacement for upstream part of repo URL + #[clap(long = "fork-replacement", value_name = "string")] + #[clap(requires_all = &["regex", "branch"])] + replacement: Option, + /// Fork branch + #[clap(long = "fork-branch", value_name = "branch")] + #[clap(requires_all = &["regex", "replacement"])] + branch: Option, } #[derive(Debug, Parser)] diff --git a/tmpl8/src/render.rs b/tmpl8/src/render.rs index 49dbaec..290ec47 100644 --- a/tmpl8/src/render.rs +++ b/tmpl8/src/render.rs @@ -29,6 +29,8 @@ use yansi::Paint; use super::schema::*; use super::*; +const DEFAULT_BRANCH: &str = "DEFAULT"; + pub(super) fn render(args: RenderArgs) -> Result<()> { let cfg = Config::parse(&args.config)?; if let Some(repo) = &args.repo { @@ -62,7 +64,7 @@ pub(super) fn diff(args: DiffArgs) -> Result<()> { // update Git cache let cache_dir = cache_dir(&args.config)?; - do_update_cache(&cfg, &cache_dir, false)?; + do_update_cache(&cfg, &cache_dir, &args.fork, false)?; if args.no_color { Paint::disable(); @@ -102,7 +104,7 @@ pub(super) fn diff(args: DiffArgs) -> Result<()> { pub(super) fn update_cache(args: UpdateCacheArgs) -> Result<()> { let cfg = Config::parse(&args.config)?; - do_update_cache(&cfg, &cache_dir(&args.config)?, true) + do_update_cache(&cfg, &cache_dir(&args.config)?, &args.fork, true) } fn do_render(config_path: &Path, cfg: &Config) -> Result> { @@ -149,35 +151,103 @@ fn clean_rendered_output(output: &str) -> String { output.to_string() } -fn do_update_cache(cfg: &Config, cache_dir: &Path, force: bool) -> Result<()> { +fn do_update_cache(cfg: &Config, cache_dir: &Path, fork: &ForkArgs, force: bool) -> Result<()> { for (name, repo) in &cfg.repos { + // clone repo if missing let path = cache_dir.join(name); - match fs::metadata(&path) { - Ok(meta) => { + if !path.exists() { + run_command( + Command::new("git") + .args(["clone", "--depth=1", &repo.url]) + .arg(&path) + .stdout(stderr()?), + )?; + // use consistent name for default branch + run_command( + Command::new("git") + .args(["branch", "-m", DEFAULT_BRANCH]) + .stdout(stderr()?) + .current_dir(&path), + )?; + } + + // compute unique identifier of remote branch + let remote_url = fork + .regex + .as_ref() + .map(|re| re.replace(&repo.url, fork.replacement.as_ref().unwrap())); + let ident = if let Some(url) = &remote_url { + format!("{} {}\n", url, fork.branch.as_ref().unwrap()) + } else { + DEFAULT_BRANCH.into() + }; + + // see if we need to update + let stamp_path = path.join(".git/tmpl8-stamp"); + // need to switch branches if the stamp contents are different + match fs::read(&stamp_path) { + Ok(id) if id == ident.as_bytes() => { + // update anyway if stale or forced + let meta = fs::metadata(&stamp_path) + .with_context(|| format!("statting {}", stamp_path.display()))?; // Update the cache at most once per hour, unless forced let age = FileTime::now().seconds() - FileTime::from_last_modification_time(&meta).seconds(); - if force || !(0..3600).contains(&age) { - run_command( - Command::new("git") - .arg("pull") - .stdout(stderr()?) - .current_dir(&path), - )?; - filetime::set_file_mtime(&path, FileTime::now()) - .with_context(|| format!("updating timestamp of {}", path.display()))?; + if (0..3600).contains(&age) && !force { + continue; } } - Err(e) if e.kind() == io::ErrorKind::NotFound => { + Ok(_) => (), + Err(e) if e.kind() == io::ErrorKind::NotFound => (), + Err(e) => return Err(e).with_context(|| format!("reading {}", stamp_path.display())), + } + + // update checkout + let mut updated = false; + // remote fork branch exists? + if let Some(remote_url) = &remote_url { + if Command::new("git") + .args(&[ + "fetch", + "--depth", + "1", + remote_url, + fork.branch.as_ref().unwrap(), + ]) + .stdout(stderr()?) + .current_dir(&path) + .status() + .context("running git fetch")? + .success() + { run_command( Command::new("git") - .args(["clone", "--depth=1", &repo.url]) - .arg(&path) - .stdout(stderr()?), + .args(&["-c", "advice.detachedHead=false", "checkout", "FETCH_HEAD"]) + .stdout(stderr()?) + .current_dir(&path), )?; + updated = true; } - Err(e) => return Err(e).with_context(|| format!("querying {}", path.display())), } + // fall back to default branch + if !updated { + run_command( + Command::new("git") + .args(&["checkout", DEFAULT_BRANCH]) + .stdout(stderr()?) + .current_dir(&path), + )?; + run_command( + Command::new("git") + .arg("pull") + .stdout(stderr()?) + .current_dir(&path), + )?; + } + + // update stamp + fs::write(&stamp_path, &ident) + .with_context(|| format!("writing {}", stamp_path.display()))?; } Ok(()) }