Skip to content

Exit codes cleanup#48439

Closed
isbm wants to merge 21 commits intosaltstack:developfrom
isbm:isbm-exitcodes
Closed

Exit codes cleanup#48439
isbm wants to merge 21 commits intosaltstack:developfrom
isbm:isbm-exitcodes

Conversation

@isbm
Copy link
Contributor

@isbm isbm commented Jul 4, 2018

What does this PR do?

Error codes cleanup

Previous Behavior

Error codes are hard-coded, brings meaningless data, overrides standards

New Behavior

Organised.

Tests written?

No

@terminalmage this helps the #48361

@isbm isbm requested a review from a team as a code owner July 4, 2018 14:19
@ghost ghost requested review from a team July 4, 2018 14:19
action='store_true',
help=('Exit with the salt call retcode and not the salt binary '
'retcode.')
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am against removing this, until it goes through a deprecation path, because there are tools out there that use --retcode-passthrough, and they will need to be updated.

Copy link
Contributor Author

@isbm isbm Jul 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gtmanfred ah you're right, backward-compat. I will add a stub with complaint here so the CLI won't fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gtmanfred fixd.

@rallytime rallytime requested a review from terminalmage July 5, 2018 17:19
@rallytime
Copy link
Contributor

@isbm This test failure is related: integration.shell.test_call.CallTest.test_syslog_file_not_found

https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu14-n/24184/

There's a small lint error too: https://jenkins.saltstack.com/job/PR/job/salt-pr-lint-n/23139/violations/
Just in case that gets lost in the long list ^

:)

@isbm
Copy link
Contributor Author

isbm commented Jul 6, 2018

@rallytime hopefully now is fine. But I touched another test, let see... :)

@rallytime
Copy link
Contributor

👌

Tests look good to me.

Copy link
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty substantial change and needs some input from @thatch45. The major changes include changing the return code range of the various salt-ssh steps, as well as defaulting --retcode-passthrough to being always on.

self.add_option('--retcode-passthrough',
default=False, action='store_true',
help=('Exit with the salt call retcode and not the salt binary return code. '
'NOTE: this is now always ON and is here only for backward compatibility.'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The help message seems to conflict with the default value here.

@rallytime rallytime requested a review from thatch45 July 24, 2018 17:59
@rallytime
Copy link
Contributor

@isbm Looks like there is a merge conflict on this now. Can you fix it?

@thatch45 Can you review this when you have a moment?

Copy link
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've already decided not to default --retcode-passthrough to being enabled in #48361. Also, I'm not really sure that such drastic changes are warranted to salt.defaults.exitcodes.

@cachedout
Copy link
Contributor

As I understand it, these changes were superceded by the changes made recently by @terminalmage. Is that correct and if so shall we go ahead and close this?

@cachedout cachedout closed this Aug 18, 2018
@isbm isbm deleted the isbm-exitcodes branch November 7, 2018 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants