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

Sort alerts by their fingerprint #969

Merged
merged 1 commit into from Sep 6, 2017
Merged

Conversation

stuartnelson3
Copy link
Contributor

addresses #812

@fabxc @brancz @mxinden opinion on using Fingerprint?

Seemed like an ok way to sort.
@brancz
Copy link
Member

brancz commented Sep 4, 2017

Not sure how to message this or whether that is necessary at all, but we need to make sure that this is just a default sorting order, that happens to be consistent at the moment, and that this behavior may change at any moment. Those things are easy for things like config files, that have documentation, but for front-end it's harder, but we also don't want to confuse people who start using it, however, given the current behavior, this is already much better, so probably nothing to worry about.

@mxinden
Copy link
Member

mxinden commented Sep 4, 2017

@stuartnelson3 @brancz Why not use StartsAt? Both StartsAt and Fingerprint would serve a consistent ordering, but StartsAt would additionally give a human useful sorting?

@brancz
Copy link
Member

brancz commented Sep 4, 2017

I can't find right now in which issue I commented this, but I think sorting by fingerprint is the default that the backend should always apply, and then additional sorting should be configured by the front-end explicitly, but sorting by StartAt could be a useful default selection in the front-end. Just my two cents, don't really have a very strong opinion.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

@brancz @stuartnelson3 Let's go with the fingerprint for now?!

@brancz
Copy link
Member

brancz commented Sep 4, 2017

code lgtm, but there is something wrong with bindata.go on Travis CI. Also we'll need to keep in mind what to do if we want to have pagination as we probably don't want to sort all records, but we can think of that when the time comes, just want to make sure this is not set in stone, if we encounter problems with it, I think we can be free to change this behavior.

@mxinden
Copy link
Member

mxinden commented Sep 6, 2017

The failure on Travis was just a flake. I have re-triggered it, all green now.

@mxinden mxinden merged commit 1007e0f into master Sep 6, 2017
@mxinden
Copy link
Member

mxinden commented Sep 6, 2017

Thanks @stuartnelson3!

@stuartnelson3 stuartnelson3 deleted the stn/ui-consistent-alert-sorting branch September 6, 2017 08:13
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.

None yet

3 participants