Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[develop] Ability to override retcode from various cmdmod functions #45851

Merged
merged 4 commits into from Feb 16, 2018

Conversation

Projects
None yet
3 participants
@garethgreenaway
Copy link
Member

commented Feb 3, 2018

What does this PR do?

Adding a flag to various cmdmod functions to allow overriding the retcode that is returned. This is useful when a command will return a non-zero retcode but the command is still considered successful.

What issues does this PR fix or reference?

#38625

Tests written?

Yes

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@rallytime rallytime requested a review from terminalmage Feb 8, 2018

@@ -951,6 +971,13 @@ def run(cmd,
Be absolutely certain that you have sanitized your input prior to using
python_shell=True
:param list success_exit_codes: This parameter will be allow a list of
non-zero return codes that should be considered a success. If the
return code returned from the run matches any in the provided list,

This comment has been minimized.

Copy link
@terminalmage

terminalmage Feb 8, 2018

Contributor

extra space between code and returned.

:param list success_exit_codes: This parameter will be allow a list of
non-zero return codes that should be considered a success. If the
return code returned from the run matches any in the provided list,
the return code will be overridden with zero.

This comment has been minimized.

Copy link
@terminalmage

terminalmage Feb 8, 2018

Contributor

"overridden" seems like the wrong word here. "replaced" maybe?

# Ensure all our retcodes are ints
success_exit_codes = [int(i) for i in success_exit_codes]
if _returncode in success_exit_codes:
ret['retcode'] = 0

This comment has been minimized.

Copy link
@terminalmage

terminalmage Feb 8, 2018

Contributor

In many places in salt we support both a Python and comma-separated list. We have a helper function for this, salt.utils.args.split_input(), which would be a good fit here.

Also , I noticed that there is a similar block below. How about moving the evaluation of success_exit_codes up higher in the function and doing something like this:

if success_exit_codes is None:
    success_exit_codes = [0]
else:
    try:
        success_exit_codes = [int(i) for i in salt.utils.args.split_input(success_exit_codes)]
    except ValueError:
        raise SaltInvocationError(
            'success_exit_codes must be a list of integers'
        )

Then you could just do a simple membership check:

if ret['retcode'] in success_exit_codes:

This comment has been minimized.

Copy link
@terminalmage

terminalmage Feb 8, 2018

Contributor

I'm also wondering if maybe this should be called success_retcodes instead of success_exit_codes. We already use retcode everywhere else in cmdmod.py to refer to the exit code of a command, so it may make sense to rename this for clarity/consistency.

the return code will be overridden with zero.
.. versionadded:: Fluorine

This comment has been minimized.

Copy link
@terminalmage

terminalmage Feb 8, 2018

Contributor

NOTE: The above two comments should be addressed in all places in the docstrings where they were copied.

garethgreenaway added some commits Feb 3, 2018

Adding a flag to various cmdmod functions to allow overriding the ret…
…code that is returned. This is useful when a command will return a non-zero retcode but the command is still considered successful.

@garethgreenaway garethgreenaway force-pushed the garethgreenaway:38625_overriding_retcode branch from 988923b to 80c0636 Feb 9, 2018

@rallytime rallytime requested a review from terminalmage Feb 16, 2018

@rallytime rallytime merged commit 7897d79 into saltstack:develop Feb 16, 2018

3 of 10 checks passed

codeclimate 10 issues to fix
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #14748 — FAILURE
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #2357 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #19999 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #6915 — FAILURE
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #16407 — FAILURE
Details
default Build started sha1 is merged.
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #22396 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #19357 — SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.