-
Notifications
You must be signed in to change notification settings - Fork 18
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
fix: Return non-zero code, when some sub-status is wrong #126
Conversation
jirihnidek
commented
Jul 17, 2024
- When some sub-status is not correct (system is not registered, insights-client is not registered or yggdrasil is not running), then return non-zero status code
- This change follow behavior of subscription-manager and it could help automation
* When some sub-status is not correct (system is not registered, insights-client is not registered or yggdrasil is not running), then return non-zero status code * This change follow behavior of subscription-manager and it could help automation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. We should document the values of the return code in the man pages somehow too.
// At the end check if all statuses are correct. | ||
// If not, return 1 exit code without any message. | ||
if systemStatus.returnCode != 0 { | ||
return cli.Exit("", 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be?
return cli.Exit("", 1) | |
return cli.Exit("", systemStatus.returnCode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the value of systemStatus.returnCode
depends on the number of "failed" sub-statuses. It could be 1, 2, 3, but we return value 1 for all other error cases.
I think that we should return different values for various situations (not only 1), but we should think twice before we do it and create some design document for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so this change is just applying a non-zero return code when any sub-status is non-zero. Seems reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@Lorquas Is the downstream test failure something we should ignore for this PR? |
@subpop Jenkins job is constantly failing. I think that we can ignore it. |
* The returned value is non-zero, when some sub-task returns non-zero value. This change was introduced in this PR: #126 * We should check for non-zero code, not zero code in this case
* The returned value is non-zero, when some sub-task returns non-zero value. This change was introduced in this PR: #126 * We should check for non-zero code, not zero code in this case
* The returned value is non-zero, when some sub-task returns non-zero value. This change was introduced in this PR: #126 * We should check for non-zero code, not zero code in this case