-
Notifications
You must be signed in to change notification settings - Fork 3
feat: send X-Tower-Idempotency-Key on deploy #303
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,20 @@ | ||
| use chrono::{DateTime, Duration, Utc}; | ||
| use clap::{Arg, ArgMatches, Command}; | ||
| use config::{Config, Towerfile}; | ||
| use std::convert::From; | ||
| use std::path::PathBuf; | ||
| use std::path::{Path, PathBuf}; | ||
|
|
||
| use crate::{output, util}; | ||
| use tower_api::apis::configuration::Configuration; | ||
| use tower_package::{Package, PackageSpec}; | ||
| use tower_telemetry::debug; | ||
|
|
||
| /// How far in the past a returned version's `created_at` must be before we treat | ||
| /// it as a reuse (rather than a fresh insert) for the stdout hint. A fresh | ||
| /// deploy stamps `created_at` at request time, so anything older than this was | ||
| /// created by an earlier deploy that shared the same idempotency key. | ||
| const REUSE_AGE_THRESHOLD: Duration = Duration::seconds(60); | ||
|
|
||
| pub fn deploy_cmd() -> Command { | ||
| Command::new("deploy") | ||
| .arg( | ||
|
|
@@ -38,6 +45,22 @@ pub fn deploy_cmd() -> Command { | |
| .action(clap::ArgAction::SetTrue) | ||
| .conflicts_with("environment"), | ||
| ) | ||
| .arg( | ||
| Arg::new("idempotency-key") | ||
| .long("idempotency-key") | ||
| .help( | ||
| "Reuse the existing version deployed with this key instead of creating a new one. \ | ||
| Defaults to the current git commit SHA when the working tree is clean.", | ||
| ) | ||
| .conflicts_with("no-idempotency-key"), | ||
| ) | ||
| .arg( | ||
| Arg::new("no-idempotency-key") | ||
| .long("no-idempotency-key") | ||
| .help("Never send an idempotency key, even on a clean git tree") | ||
| .action(clap::ArgAction::SetTrue) | ||
| .conflicts_with("idempotency-key"), | ||
| ) | ||
| .about("Deploy your latest code to Tower") | ||
| } | ||
|
|
||
|
|
@@ -59,9 +82,29 @@ pub enum DeployTarget { | |
| All, | ||
| } | ||
|
|
||
| /// Resolves the idempotency key to send with the deploy, in precedence order: | ||
| /// | ||
| /// 1. `--no-idempotency-key` → never send a key. | ||
| /// 2. `--idempotency-key <value>` → send that exact value. | ||
| /// 3. Otherwise auto-detect the git `HEAD` SHA, but only when the working tree | ||
| /// is clean (a dirty tree gets no key, so every deploy creates a new version). | ||
| fn resolve_idempotency_key(args: &ArgMatches, dir: &Path) -> Option<String> { | ||
| if args.get_flag("no-idempotency-key") { | ||
| debug!("--no-idempotency-key set; suppressing idempotency key"); | ||
| return None; | ||
| } | ||
|
|
||
| if let Some(key) = args.get_one::<String>("idempotency-key") { | ||
| return Some(key.clone()); | ||
| } | ||
|
|
||
| util::git::clean_head_sha(dir) | ||
| } | ||
|
|
||
| pub async fn do_deploy(config: Config, args: &ArgMatches) { | ||
| let dir = resolve_path(args); | ||
| let create_app = args.get_flag("create"); | ||
| let idempotency_key = resolve_idempotency_key(args, &dir); | ||
|
|
||
| let target = if args.get_flag("all") { | ||
| DeployTarget::All | ||
|
|
@@ -71,7 +114,7 @@ pub async fn do_deploy(config: Config, args: &ArgMatches) { | |
| DeployTarget::Environment("default".to_string()) | ||
| }; | ||
|
|
||
| if let Err(err) = deploy_from_dir(config, dir, create_app, target).await { | ||
| if let Err(err) = deploy_from_dir(config, dir, create_app, target, idempotency_key).await { | ||
| match err { | ||
| crate::Error::ApiDeployError { source } => { | ||
| output::tower_error_and_die(source, "Deploying app failed") | ||
|
|
@@ -100,6 +143,7 @@ pub async fn deploy_from_dir( | |
| dir: PathBuf, | ||
| create_app: bool, | ||
| target: DeployTarget, | ||
| idempotency_key: Option<String>, | ||
| ) -> Result<(), crate::Error> { | ||
| debug!("Building package from directory: {:?}", dir); | ||
|
|
||
|
|
@@ -135,14 +179,15 @@ pub async fn deploy_from_dir( | |
| }; | ||
|
|
||
| spinner.success(); | ||
| do_deploy_package(api_config, package, &towerfile, target).await | ||
| do_deploy_package(api_config, package, &towerfile, target, idempotency_key).await | ||
| } | ||
|
|
||
| async fn do_deploy_package( | ||
| api_config: Configuration, | ||
| package: Package, | ||
| towerfile: &Towerfile, | ||
| target: DeployTarget, | ||
| idempotency_key: Option<String>, | ||
| ) -> Result<(), crate::Error> { | ||
| let (environment, all_environments) = match &target { | ||
| DeployTarget::All => (None, true), | ||
|
|
@@ -155,6 +200,7 @@ async fn do_deploy_package( | |
| package, | ||
| environment, | ||
| all_environments, | ||
| idempotency_key.as_deref(), | ||
| ) | ||
| .await; | ||
|
|
||
|
|
@@ -172,22 +218,72 @@ async fn do_deploy_package( | |
| ), | ||
| }; | ||
| output::success(&line); | ||
|
|
||
| // Only emit the informational hint in normal mode; in JSON mode a | ||
| // bare line would corrupt the structured output. | ||
| if output::get_output_mode().is_normal() { | ||
| if let Some(hint) = reuse_hint(&version, idempotency_key.as_deref()) { | ||
| output::write(&hint); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| Err(err) => Err(crate::Error::ApiDeployError { source: err }), | ||
| } | ||
| } | ||
|
|
||
| /// Builds the "reused an existing version" hint, or `None` when the deploy | ||
| /// created a fresh version. | ||
| /// | ||
| /// We treat the returned version as a reuse when (a) we sent an idempotency key, | ||
| /// (b) the server echoed it back on the version, and (c) the version's | ||
| /// `created_at` is meaningfully older than now — i.e. it predates this deploy. | ||
| fn reuse_hint(version: &tower_api::models::AppVersion, sent_key: Option<&str>) -> Option<String> { | ||
| let sent_key = sent_key?; | ||
|
|
||
| if version.idempotency_key.as_deref() != Some(sent_key) { | ||
| return None; | ||
| } | ||
|
|
||
| let created_at = DateTime::parse_from_rfc3339(&version.created_at) | ||
| .ok()? | ||
| .with_timezone(&Utc); | ||
|
|
||
| if Utc::now().signed_duration_since(created_at) < REUSE_AGE_THRESHOLD { | ||
| return None; | ||
| } | ||
|
|
||
| Some(format!( | ||
| "Reusing version `{}` (deployed {}) — no source changes since key {}.", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since this is not actually true, but is rather that the git repository doesn't have any committed changes, we should probably mention "git" |
||
| version.version, | ||
| util::dates::format(created_at), | ||
| sent_key | ||
| )) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::deploy_cmd; | ||
| use super::*; | ||
| use tower_api::models::AppVersion; | ||
|
|
||
| fn parse(args: &[&str]) -> Result<clap::ArgMatches, clap::Error> { | ||
| let mut full = vec!["deploy"]; | ||
| full.extend_from_slice(args); | ||
| deploy_cmd().try_get_matches_from(full) | ||
| } | ||
|
|
||
| fn version_with(idempotency_key: Option<&str>, created_at: &str) -> AppVersion { | ||
| AppVersion { | ||
| created_at: created_at.to_string(), | ||
| parameters: vec![], | ||
| towerfile: String::new(), | ||
| version: "v3".to_string(), | ||
| idempotency_key: idempotency_key.map(|s| s.to_string()), | ||
| content_checksum: None, | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn no_args_uses_defaults() { | ||
| let m = parse(&[]).unwrap(); | ||
|
|
@@ -276,4 +372,76 @@ mod tests { | |
| let err = parse(&["--help"]).unwrap_err(); | ||
| assert_eq!(err.kind(), clap::error::ErrorKind::DisplayHelp); | ||
| } | ||
|
|
||
| #[test] | ||
| fn idempotency_key_flag() { | ||
| let m = parse(&["--idempotency-key", "abc123"]).unwrap(); | ||
| assert_eq!( | ||
| m.get_one::<String>("idempotency-key").map(|s| s.as_str()), | ||
| Some("abc123") | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn no_idempotency_key_flag() { | ||
| let m = parse(&["--no-idempotency-key"]).unwrap(); | ||
| assert!(m.get_flag("no-idempotency-key")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn idempotency_key_and_no_idempotency_key_conflict() { | ||
| let err = parse(&["--idempotency-key", "abc", "--no-idempotency-key"]).unwrap_err(); | ||
| assert_eq!(err.kind(), clap::error::ErrorKind::ArgumentConflict); | ||
| } | ||
|
|
||
| #[test] | ||
| fn resolve_key_prefers_explicit_flag() { | ||
| let m = parse(&["--idempotency-key", "explicit"]).unwrap(); | ||
| // A non-existent path guarantees git auto-detection can't interfere. | ||
| let key = resolve_idempotency_key(&m, Path::new("/nonexistent/path")); | ||
| assert_eq!(key.as_deref(), Some("explicit")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn resolve_key_opt_out_wins_over_git() { | ||
| let m = parse(&["--no-idempotency-key"]).unwrap(); | ||
| // Even pointed at the (clean-or-dirty) repo we're in, opt-out yields None. | ||
| let key = resolve_idempotency_key(&m, Path::new(".")); | ||
| assert_eq!(key, None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn resolve_key_none_outside_git() { | ||
| let m = parse(&[]).unwrap(); | ||
| let key = resolve_idempotency_key(&m, Path::new("/nonexistent/path")); | ||
| assert_eq!(key, None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn reuse_hint_none_without_sent_key() { | ||
| let version = version_with(Some("abc"), "2020-01-01T00:00:00Z"); | ||
| assert_eq!(reuse_hint(&version, None), None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn reuse_hint_none_when_key_mismatch() { | ||
| let version = version_with(Some("other"), "2020-01-01T00:00:00Z"); | ||
| assert_eq!(reuse_hint(&version, Some("abc")), None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn reuse_hint_none_for_freshly_created_version() { | ||
| // created_at "now" → not a reuse, no hint. | ||
| let now = Utc::now().to_rfc3339(); | ||
| let version = version_with(Some("abc"), &now); | ||
| assert_eq!(reuse_hint(&version, Some("abc")), None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn reuse_hint_present_for_old_matching_version() { | ||
| let version = version_with(Some("abc123"), "2020-01-01T00:00:00Z"); | ||
| let hint = reuse_hint(&version, Some("abc123")).expect("expected a reuse hint"); | ||
| assert!(hint.contains("Reusing version `v3`")); | ||
| assert!(hint.contains("abc123")); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -812,7 +812,20 @@ impl TowerService { | |
| let env = request.environment.unwrap_or_else(|| "default".to_string()); | ||
| let deploy_target = deploy::DeployTarget::Environment(env); | ||
|
|
||
| match deploy::deploy_from_dir(self.config.clone(), working_dir, true, deploy_target).await { | ||
| // Auto-detect the idempotency key from git (clean tree HEAD) just like | ||
| // the CLI deploy command does, so repeated deploys of unchanged source | ||
| // collapse to a single AppVersion server-side. | ||
| let idempotency_key = crate::util::git::clean_head_sha(&working_dir); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would be good to do a SHA of the SHAs for the idempotency_key when git is dirty. Alternatively, swap the warning around to tell the user that because their git repo is not clean, the whole bundle is being re-uploaded (current way around you'd only know this behaviour exists if you pushed a clean repo, and given we state that even an untracked file is enough to make a repo dirty, some people may never hit upon this behaviour and wonder why tower doesn't support it) |
||
|
|
||
| match deploy::deploy_from_dir( | ||
| self.config.clone(), | ||
| working_dir, | ||
| true, | ||
| deploy_target, | ||
| idempotency_key, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(_) => Self::text_success("Deploy completed successfully".to_string()), | ||
| Err(e) => Self::error_result("Deploy failed", e), | ||
| } | ||
|
|
||
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.
FWIW I think if you're using this function to work out what to write out, then it's almost certainly a code smell (it's public so that we can check for
ctrl-cbehaviour differently, but wanting to only print things out if it's not json or mcp mode seems to me like anoutput.rsspecific concern).Additionally, in this case would it make sense to either re-use
output::success, or to print it to stderr withoutput::write_to_stderr? If not, perhaps a simple function addition tooutputthat writes only informational text tooutput_mode().is_normal()texts would be better?