Conversation
- Remove boxed models, since that's more what we expect. - For detached runs, make sure all that works. - For attached runs, print messages that are useful at the end.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
crates/tower-cmd/src/api.rs
Outdated
| while let Some(event) = source.next().await { | ||
| match event { | ||
| Ok(reqwest_eventsource::Event::Open) => { | ||
| // TODO: This shouldn't happen. |
There was a problem hiding this comment.
these sorts of comments just tempt fate
There was a problem hiding this comment.
correct--good point :) I added a panic.
| let api_config: configuration::Configuration = config.into(); | ||
|
|
||
| // These represent the messages that we'll stream to the client. | ||
| let (tx, rx) = mpsc::channel(1); |
There was a problem hiding this comment.
Yep, tx = transmit, rx = receive. Convention from the Tokio docs.
There was a problem hiding this comment.
Ha! I commented the same. Very weird naming
crates/tower-cmd/src/apps.rs
Outdated
| let dt: DateTime<Utc> = DateTime::parse_from_rfc3339(&line.timestamp) | ||
| .unwrap() | ||
| .with_timezone(&Utc); | ||
| let ts = dt.format("%Y-%m-%d %H:%M:%S").to_string(); |
There was a problem hiding this comment.
Ah nice. I'll update this. Actually, I'll refactor it out, too, to make it consistent.
| } | ||
| } | ||
|
|
||
| impl<'de> Deserialize<'de> for {{{classname}}} { |
There was a problem hiding this comment.
What's going on here? How come all the auto generated code now derives Deserializer as well as Deserialize?
There was a problem hiding this comment.
It's a bit hard to follow since it's in this (very hairy) mustache template. For enum types (with name {{{classname}}}), we implement the Deserialize trait. It takes an argument D which implements the Deserializer trait.
All of this is meant to allow us to deserialize enum types in a case-insensitive way! So both "PROGRAM" and "program" can deserialize to Channel::Program. It's otherwise very strict and often breaks out API when we don't conform to the spec--which, actually, was exactly what was happening for log lines and was blocking work on this PR!
| let api_config: configuration::Configuration = config.into(); | ||
|
|
||
| // These represent the messages that we'll stream to the client. | ||
| let (tx, rx) = mpsc::channel(1); |
There was a problem hiding this comment.
Ha! I commented the same. Very weird naming
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the ability to follow Tower runs in real-time from the CLI and includes generated API model updates. The default behavior now follows run logs when executing tower run, with an optional --detach flag to run without following. Additionally, the Rust client generator configuration has been updated to handle enum deserialization more flexibly.
Key changes include:
- Added real-time run following functionality with log streaming and completion monitoring
- Added
--detachflag to disable log following behavior - Updated generated API models to handle case-insensitive enum deserialization
- Enhanced error handling and Ctrl+C interrupt support
Reviewed Changes
Copilot reviewed 172 out of 173 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
crates/tower-cmd/src/run.rs |
Main implementation of run following functionality with log streaming and completion monitoring |
crates/tower-cmd/src/api.rs |
Added event streaming support and new API endpoints for run monitoring |
crates/tower-cmd/src/util/dates.rs |
New utility module for consistent timestamp formatting |
scripts/rust-client-templates/model.mustache |
Updated enum deserialization to be case-insensitive and more flexible |
crates/tower-api/src/models/*.rs |
Generated API model updates with improved enum handling and removed boxed types |
Comments suppressed due to low confidence (1)
crates/tower-cmd/src/run.rs:77
- The match pattern uses
Ok(Self::{{{name}}})in the template but here it's missing theOk()wrapper. This should beOk(Self::Scheduled)to maintain consistency with the deserialization pattern.
);
| while let Some(event) = source.next().await { | ||
| match event { | ||
| Ok(reqwest_eventsource::Event::Open) => { | ||
| panic!("Received unexpected open event in log stream. This shouldn't happen."); |
There was a problem hiding this comment.
Using panic! in production code should be avoided. Consider returning an error instead of panicking, as this could crash the entire application unexpectedly.
| panic!("Received unexpected open event in log stream. This shouldn't happen."); | |
| debug!("Received unexpected open event in log stream. This shouldn't happen."); | |
| break; // Exit the loop gracefully |
| }, | ||
| Ok(Event::Message(message)) => { | ||
| // This is a bug in the program and should never happen. | ||
| panic!("Received message when expected an open event. Message: {:?}", message); |
There was a problem hiding this comment.
Using panic! in production code should be avoided. Consider returning an error instead of panicking to maintain application stability.
| let run = wait_for_run_completion(&config_clone, &run_clone). | ||
| await. | ||
| unwrap(); | ||
|
|
There was a problem hiding this comment.
Using unwrap() in async code can cause panics. Consider proper error handling and propagating the error to the caller instead of unwrapping.
| let run = wait_for_run_completion(&config_clone, &run_clone). | |
| await. | |
| unwrap(); | |
| match wait_for_run_completion(&config_clone, &run_clone).await { | |
| Ok(run) => { | |
| let _ = tx.send(run); | |
| } | |
| Err(e) => { | |
| eprintln!("Error waiting for run completion: {:?}", e); | |
| } | |
| } |
There was a problem hiding this comment.
Is this relevant?
Edit: I mean the copilot comment at the same line
There was a problem hiding this comment.
I think we want it to panic in this case. The CLI will just die.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
konstantinoscs
left a comment
There was a problem hiding this comment.
Ok with me but I would prefer it if @socksy was also satisfied before we merge this
| let run = wait_for_run_completion(&config_clone, &run_clone). | ||
| await. | ||
| unwrap(); | ||
|
|
There was a problem hiding this comment.
Is this relevant?
Edit: I mean the copilot comment at the same line
This PR introduces the ability to follow Tower runs in the Tower cloud when you invoke them locally. The behavior by default is such that when you to a
tower runthe CLI will follow the logs. You can runtower run --detachto have it run locally, and you'll get a link to the Tower UI back.You can see a demo here: https://asciinema.org/a/PWMkIRy6Igl3FeY3hzygGQg55
Note: I also relaxed the constraints of the CLI a little bit such that it can handle enums that don't match spec.