Propagate install and uninstall hook exit codes #7782
Conversation
davidMcneil
requested review from
christophermaier,
eeyun and
smacfarlane
as code owners
| @@ -61,7 +61,7 @@ impl Default for ExitCode { | |||
| pub trait Hook: fmt::Debug + Sized + Send { | |||
| type ExitValue: fmt::Debug + Send; | |||
|
|
|||
| fn file_name() -> &'static str; | |||
| const FILE_NAME: &'static str; | |||
| @@ -315,6 +320,61 @@ pub trait Hook: fmt::Debug + Sized + Send { | |||
| fn stderr_log_path(&self) -> &Path; | |||
| } | |||
|
|
|||
| #[async_trait::async_trait] | |||
| pub trait HookExt: Hook<ExitValue = ExitStatus> + Sync { | |||
There was a problem hiding this comment.
The naming and typing of this trait doesn't clearly capture that it's for install and uninstall hooks only. It'd be nice to make that more obvious (though I admittedly am not sure what a better name for the trait and function would be).
There was a problem hiding this comment.
Agreed. I struggled around the naming of this. There is no reason this has to apply only to the install and uninstall hooks so I left the naming very general.
The method and frequency of when the install and uninstall hooks are run may provide an axis of differentiation. What if I called the trait one of the following: OneOffHookExt, AuxiliaryHookExt, PackageMaintenceHookExt...?
Ultimately, I think the Hook trait could use some cleanup. There is a lot of duplicated functionality when calling the hooks that should be moved into the trait itself and the handle_exit functionality seems unnecessarily general. There is really only a few different kinds of exit types and this would probably be better handled with different run functions.
There was a problem hiding this comment.
I agree that the Hook trait needs a lot of love
The idea of using method/frequency of hook execution as an axis of differentiation is a great one. Of the suggestions given, I lean toward PackageMaintenanceHookExt as it's more descriptive and seems to capture something core to the whole idea of having install and uninstall hooks. Making that name change, along with a nice documentation comment, would be a nice addition 👍
There was a problem hiding this comment.
Thanks! Addressed in c02aca
davidMcneil
force-pushed
the
dmcneil/propogate-hook-exit-status
branch
2 times, most recently
from
c02acae
to
62adbd3
Compare
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
davidMcneil
force-pushed
the
dmcneil/propogate-hook-exit-status
branch
from
62adbd3
to
d661b9a
Compare
Resolves #6964
This PR adds the logic to propagate the exit code of the
installanduninstallhook to thehabcommand.