Unify execution interfaces to new one, deprecate old one#179
Conversation
| #[async_trait] | ||
| pub trait ExecutionHandle: Send + Sync { | ||
| /// Get a unique identifier for this execution | ||
| pub trait App: Send + Sync { |
There was a problem hiding this comment.
Note this would require a change in the runner side for the k8s impl. I propose just keeping the new name rather than renaming since I believe it's better name than "App"
290029a to
8ffdb01
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This marks the previous lib::App one as deprecated, moves everything over to the new interface from Vim as the default, renames that to be more similar to what we had before + a couple simplifications, and keeps the previous ExecutionBackend/ExecutionHandle names as deprecated traits so tower-runner sees guiding deprecation warnings on its next relink rather than unresolved-import errors.
8ffdb01 to
5e60137
Compare
|
I think the main thrust of this PR is to standardize the interfaces slightly better than they are now. I do think that's valid, it just needs to be modernized a bit. I don't have strong opinions about how that should come together however. I'd be open to keeping this around and reintegrating the latest into it. |
Standardising on one execution interface. Renames
ExecutionBackend/ExecutionHandleback toBackend/App(the old names) and adds default method impls so backends don't stub what they don't use.LocalAppimplementsAppdirectly now;SubprocessHandlesticks around as a thin wrapper becauseauto_cleanup's temp-dir lifecycle has to hang off something.Cancellation used to ride in as exit code
-1, which conflated it with IO errors and signal kills.execute_local_appnow sends aResult<Status, Error>andStatusgrows aCancelledvariant. Hopefully this lets the control plane drop the "preserve Cancelled if last_known_status was Cancelled" heuristic.Cancelledis a hard break for exhaustive matches — one-line arm needed in tower-runner when it relinks, tracked out of band. The trait rename is kept deprecated-but-defined (not aliased) so downstream impls compile with a guiding message rather than an unresolved-import error. Root-leveltower_runtime::Appis also deprecated in favour ofexecution::App.