-
Notifications
You must be signed in to change notification settings - Fork 3k
Description
the code here contains the main logic when executing a command:
Lines 939 to 963 in 4fa4fdf
if c.RunE != nil { | |
if err := c.RunE(c, argWoFlags); err != nil { | |
return err | |
} | |
} else { | |
c.Run(c, argWoFlags) | |
} | |
if c.PostRunE != nil { | |
if err := c.PostRunE(c, argWoFlags); err != nil { | |
return err | |
} | |
} else if c.PostRun != nil { | |
c.PostRun(c, argWoFlags) | |
} | |
for p := c; p != nil; p = p.Parent() { | |
if p.PersistentPostRunE != nil { | |
if err := p.PersistentPostRunE(c, argWoFlags); err != nil { | |
return err | |
} | |
break | |
} else if p.PersistentPostRun != nil { | |
p.PersistentPostRun(c, argWoFlags) | |
break | |
} | |
} |
Both PostRun
and PersistentPostRun
functions are not executed when the RunE
function returns an error.
This is surprising behaviour for us in podman. Given the name we assumed that this function always runs after the run function.
The documentation for the PostRun function is very brief and this behaviour is not mentioned. So as first step we should fix this to help others not making the same mistake.
I understand that cobra is very stable and changing this behaviour could break others. It is also not clear how cobra could handle the case where both RunE and PostRunE would return an error since the execute function can only return single error.
While it is possible to work around this in our code it is certainly not pretty since we need to know the real child command that was executed to call the correct PersistentPostRunE function on that command.
I think this is a problem that should be fixed here in cobra instead, this likely requires the addition of a new option to make this opt in and keep backwards compat.