replace - always return rc

[jscheible]

Error handling in playbooks generally expects rc to be set to 0 when a module has not failed. Playbook authors should not have to check for the existence of rc first.

SUMMARY

Added two lines to set res_args['rc'] = 0 before normal module.exit() calls.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

replace

ADDITIONAL INFORMATION

[jborean93]

I don't see the point in returning rc as this isn't calling any command to produce a reliable rc from. Ansible already has tests to check if a module failed or succeeded with these test plugins.

[jscheible]

Our issue was that the module was failing when the file we wanted to modify did not exist. In our situation, that was not to be deemed a failure. So we added `failed_when: result.rc != 0 and 'Path {{file}} does not exist' not in result.msg`. Then that failed with "'dict object' has no attribute 'rc'"! As standard programming practice, a return code should always be set. Furthermore, the return variables should always be consistent, not exist under some conditions and not under others. Thanks,

…

-Jack

On Sat, Sep 26, 2020 at 1:13 AM Jordan Borean ***@***.***> wrote: I don't see the point in returning rc as this isn't calling any command to produce a reliable rc from. Ansible already has tests to check if a module failed or succeeded with these test plugins <https://docs.ansible.com/ansible/latest/user_guide/playbooks_tests.html#testing-task-results> . — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#71963 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVPRRGTXIBBHXJTV6GTYKDSHVZ6LANCNFSM4R2T7SDA> .

[jscheible]

Really, for my case, I should be able to put in failed_when: result.rc == 256, because that is the only error condition I care about. If rc is not set on success, I cannot do that. Thanks,

…

-Jack

On Sat, Sep 26, 2020 at 9:16 AM Jack Scheible ***@***.***> wrote: Our issue was that the module was failing when the file we wanted to modify did not exist. In our situation, that was not to be deemed a failure. So we added `failed_when: result.rc != 0 and 'Path {{file}} does not exist' not in result.msg`. Then that failed with "'dict object' has no attribute 'rc'"! As standard programming practice, a return code should always be set. Furthermore, the return variables should always be consistent, not exist under some conditions and not under others. Thanks, -Jack On Sat, Sep 26, 2020 at 1:13 AM Jordan Borean ***@***.***> wrote: > I don't see the point in returning rc as this isn't calling any command > to produce a reliable rc from. Ansible already has tests to check if a > module failed or succeeded with these test plugins > <https://docs.ansible.com/ansible/latest/user_guide/playbooks_tests.html#testing-task-results> > . > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#71963 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAVPRRGTXIBBHXJTV6GTYKDSHVZ6LANCNFSM4R2T7SDA> > . >

[jborean93]

My point is that rc codes are not consistent in Ansible and not every module has them. Typically the ones that do are the ones that call a new process as it is reporting the rc that it reports. In this case this PR simply returns 0 but Ansible’s way of notifying a success is using the tests I mentioned.

[jborean93]

My point is that rc codes are not consistent in Ansible and not every module has them. Typically the ones that do are the ones that call a new process as it is reporting the rc that it reports. In this case this PR simply returns 0 but Ansible’s way of notifying a success is using the tests I mentioned.

[jscheible]

My point is, whatever return variables a module has (but especially rc), those variables should always be in the output, so that subsequent processing can be consistent. Thanks,

…

-Jack

On Sat, Sep 26, 2020 at 6:03 PM Jordan Borean ***@***.***> wrote: My point is that rc codes are not consistent in Ansible and not every module has them. Typically the ones that do are the ones that call a new process as it is reporting the rc that it reports. In this case this PR simply returns 0 but Ansible’s way of notifying a success is using the tests I mentioned. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#71963 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVPRRA2O36OZWZSYMDSSM3SHZQMTANCNFSM4R2T7SDA> .

[jborean93]

Ah I see now, I thought this PR added the rc value but it was already there. A better way would be to have rc=0 when the return dict is defined rather than setting to 0 in 2 places. That way the default is 0 and only changes when explicitly done so.

[jscheible]

You want to do that, or shall I?

…

-Jack

On Sun, Sep 27, 2020 at 7:10 AM Jordan Borean ***@***.***> wrote: Ah I see now, I thought this PR added the rc value but it was already there. A better way would be to have rc=0 when the return dict is defined rather than setting to 0 in 2 places. That way the default is 0 and only changes when explicitly done so. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#71963 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVPRRE4JNXQVJDG7TD23FDSH4MRVANCNFSM4R2T7SDA> .

[samdoran]

👍 To adding rc: 0 to the res_args when it is defined. Please add a changelog and tests as well.