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.

Sign up for GitHub

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

Jump to bottom

Fixed issue where nextID was not inherited #179

Merged
merged 3 commits into from
Nov 22, 2018
Merged

Fixed issue where nextID was not inherited #179

merged 3 commits into from
Nov 22, 2018
+8 −4

Conversation

yaminoma
Copy link

I fixed it because I could not paging correctly at #178.

@susisu
Copy link
Contributor

susisu commented Nov 22, 2018

LGTM, but now I have other questions.

  • Is it meaningful that nextId is included in the output?
  • When specified limit is not a multiple of 100, like --limit 50, the number of output will still be 100 (if there are more than 100 alerts). Is it ok?

@yaminoma
Copy link
Author

yaminoma commented Nov 22, 2018 •
edited

Thanks. As you pointed out, there was a mistake in this implementation, so I fixed it.

  • The nextID did not need to be output.
  • Fixed because it turned out that the number specified by LIMIT is different from the number of alerts actually displayed.

susisu
susisu reviewed Nov 22, 2018
View reviewed changes
alerts.go
@@ -262,7 +263,7 @@ func doAlertsRetrieve(c *cli.Context) error {
}
}

PrettyPrintJSON(alerts)
PrettyPrintJSON(alerts.Alerts[:limit])
Copy link
Contributor

Choose a reason for hiding this comment

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

The output format is now the same as the latest released version 👍

susisu
susisu approved these changes Nov 22, 2018
View reviewed changes
Copy link
Contributor

@susisu susisu left a comment

Choose a reason for hiding this comment

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

LGTM!

@susisu susisu merged commit 0d4c18b into master Nov 22, 2018
@susisu susisu deleted the fix-with-closed branch November 22, 2018 08:29
This was referenced Nov 26, 2018
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@susisu susisu susisu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@yaminoma @susisu