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

List all notes and show status note if any #234

Merged
merged 1 commit into from
Aug 18, 2019
Merged

Conversation

satterly
Copy link
Member

@satterly satterly commented Aug 16, 2019

Screenshot 2019-08-18 at 08 54 59

@satterly
Copy link
Member Author

satterly commented Aug 16, 2019

@strangeman @vykulakov Please review. (Taken feedback from #228)

@satterly
Copy link
Member Author

I'll merge this for now and iterate on it later if required.

@vykulakov
Copy link

vykulakov commented Aug 19, 2019

Thank you for this improvement - it is really helpful and convenient now.

But I see two problems with the current approach:

  1. I cannot edit/remove an old note. As a result, I must always see all notes which I left. Even those which are not actual now. In some cases, it may be helpful to see all alert notes, but in other cases, it will be confusing to see old unactual notes. It happens because you try to use alert history to store notes. But in my vision, they should be different and independent entities. So alert history should contain only changes in alert notes/state/status/severity and so on. And alert notes should contain all alert notes with a possibility to add/edit/remove them.
  2. We won't see notes if there are more than 100 history records. It is very unexpected to suddenly lose all notes for an alert. It may happen for flapping alerts, for instance, or for alerts with a long history (which live years). The reason for this behaviour is also using alert history to store notes.

Nonetheless, status comments work fine and I don't see any obvious problems with them right now. We may lose the user's comment when alert severity is changing, though, but I'm not sure about it and should test this behaviour.

@vykulakov
Copy link

What I see (pay attention to comment content):
image

@satterly
Copy link
Member Author

Thanks for the feedback. You have raised valid concerns. I'll keep track of this issue and will consider the improvements you have suggested in the near future. In the meantime, further comments and feedback are always welcome.

@satterly
Copy link
Member Author

Discussion moved to #235

@alerta alerta locked as resolved and limited conversation to collaborators Aug 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants