-
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
4cafba0
8776d35
e96d2fa
560872b
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 |
|---|---|---|
|
|
@@ -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)
Contributor
Author
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. I think we should err on the side of folks properly using git for the time being. The future state will be this is always up to date with Git anyway. I'm happy to say "you called it" if we get complaints about this behavior, which is strictly more limiting than less. |
||
|
|
||
| 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?