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

fix: Return non-zero code, when some sub-status is wrong #126

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

jirihnidek
Copy link
Contributor

  • 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
Copy link
Collaborator

@subpop subpop left a 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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be?

Suggested change
return cli.Exit("", 1)
return cli.Exit("", systemStatus.returnCode)

Copy link
Contributor Author

@jirihnidek jirihnidek Jul 18, 2024

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.

Copy link
Collaborator

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.

Copy link
Collaborator

@subpop subpop left a comment

Choose a reason for hiding this comment

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

LGTM

@subpop
Copy link
Collaborator

subpop commented Jul 19, 2024

@Lorquas Is the downstream test failure something we should ignore for this PR?

@jirihnidek
Copy link
Contributor Author

@subpop Jenkins job is constantly failing. I think that we can ignore it.

@subpop subpop merged commit 8478045 into main Sep 10, 2024
20 of 21 checks passed
@subpop subpop deleted the jhnidek/status_return_code branch September 10, 2024 15:02
jirihnidek added a commit that referenced this pull request Sep 12, 2024
* 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
jirihnidek added a commit that referenced this pull request Sep 12, 2024
* 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
Archana-PandeyM pushed a commit that referenced this pull request Sep 13, 2024
* 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
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.

2 participants