Make returncode friendly to onfail requisites

[kiorky]

What does this PR do?

Handles correctly returncode of state executions when onfail requisites are used

What issues does this PR fix or reference?

#39873

Previous Behavior

RC==2 when onfail states related to failed states are present and suceed

New Behavior

RC==0 when onfail states related to failed states are present and suceed

Tests written?

Doing them right now

Im currently writing tests and polishing what's added, but as it is non trivial, im opening this for discussion

cc @terminalmage @thatch45 @basepi @regilero

[kiorky]

just pushed a new rebase.

[kiorky]

failures seems unrelated

[thatch45]

Sorry I am being slow on this, I have just not had the time to get into it. Hopefully I can get this reviewed today

[thatch45]

@kiorky here is what I want to do, the issue is that an onfail does not necessarily mean that the run did not have intended failures, since an onfail can be used as an expected trigger or as a safety net for a legit error you would want to know about.
So I would say that we should add a flag to the state that specifies that the intent of the onfail is to catch a legit failure check. Then we could accommodate all cases.

[kiorky]

OK, but as a default what do we choose ?

1. Current behavior of RC==2
2. implemented in this PR one which not fail if the state is correctly handled by enslaved states.

I vote for 2.

[thatch45]

Hmm, I would vote for 1, minimize the change in behavior. What do you think @cachedout ?

[cachedout]

I don't like changes in behavior in bugfix branches, I'm open to a discuss about a change if this moves to develop.

@kiorky ?

[thatch45]

Oh, yes, I would not call this a bugfix

[kiorky]

Well, for me the behavior is totally bugged as i wont expect an error if i handle with onfails, but yes, that's my sole POV.

This code is already ported in our fork from 2015.8 -> 2016.11 so i can rebase this to be on top of develop, that's not a big deal.

I think no one should use them that way that much in the wild and hoping for rc==0 as if a onfail is triggered, we are already sending, with current code, a rc==2. So, that's impossible per se.

[cachedout]

@kiorky If you want to go ahead and move this to the devlop branch than I think we can move forward with this. Thanks.

[kiorky]

Y, im just busy and dont have time right now to reopen the subject.
Can we do the burden of reviewing and making the code on 2016.11 and i ll rebase on develop when we will consider this more finished ?
That would be simpler for me as we don't use develop.

[thatch45]

Approved assuming that the code gets merged to the develop branch

[cachedout]

@kiorky Looks like we are ready for this to be merged after you rebase and change the branch?

[cachedout]

I have changed the base branch to develop per the earlier discusison. This now needs a rebase to address merge conflicts, please.

[kiorky]

Well, sorry, i have really little time right now, think the best ETA would be in the WE.

The PR as it is untouched still does not have the option toggle, so i have to add this support first.

[kiorky]

See the 2nd commit for the flag ! cc @thatch45 @basepi