Forbid scripts to be attached to the terminal states #2537
Conversation
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
didier-wenzek
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Codecov ReportAttention:
Additional details and impacted files
|
Robot Results
|
| } | ||
|
|
||
| // The successful state can be omitted, | ||
| // but must be associated to a `clear` if provided. |
There was a problem hiding this comment.
That comment seemed to suggest that the only the clear action is allowed in the terminal state, if the user has explicitly defined this state. But, the following impl seems to be adding clear as the default, even if the user has not explicitly provided one. There seems to be a mismatch.
| // but must be associated to a `clear` if provided. | ||
| let action_on_success = states | ||
| .entry("successful".to_string()) | ||
| .or_insert(OperationAction::Clear); |
There was a problem hiding this comment.
That default behaviour of auto-clearing a command on successful, when not explicitly provide, seems a bit dangerous, considering the fact that most of the ops in a deployment would be triggered by the C8y mapper and it would want to explicitly clear those operations as well, to avoid the risk of losing that terminal state transition, just in-case it was down while operation was completed by the agent. I'd rather prefer "clear on success/failure" to be more explicitly given by the user, to be on the safer side.
There was a problem hiding this comment.
OperationAction::Clear might be a misnomer. The agent clear nothing but wait for the requester to clear the command topic.
There was a problem hiding this comment.
Aah okay. My bad. I thought that Clear action was giving the ability to automatically clear the operation, as that's what I was trying to do in #2484.
And yes, the naming could be more explicit to avoid such confusion. But, since it's not part of the external workflow TOML API like action=clear, it's okay.
There was a problem hiding this comment.
I just noticed that action=cleanup maps to the same OperationAction::Clear. That's also a bit misleading IMO, giving the notion that we perform some cleanup. But we can improve these things later.
| action: format!("{action_on_error:?}"), | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
We could add even more validations here like the restarting action must have an on_exec handler, or the background_script action must have the on_exec handler but the other handlers like on_success, on_stdout etc are not expected.
But, for the limited scope of this ticket, this seems sufficient.
| // but must be associated to a `clear` if provided. | ||
| let action_on_success = states | ||
| .entry("successful".to_string()) | ||
| .or_insert(OperationAction::Clear); |
There was a problem hiding this comment.
Aah okay. My bad. I thought that Clear action was giving the ability to automatically clear the operation, as that's what I was trying to do in #2484.
And yes, the naming could be more explicit to avoid such confusion. But, since it's not part of the external workflow TOML API like action=clear, it's okay.
Proposed changes
Simply forbid scripts to be attached to the terminal states in order to avoid unexpected behavior such as:
This is more a workaround rather than the definitive solution.
Types of changes
Paste Link to the issue
#2484
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments