Skip to content

Allow filtering silences by creator #2541

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

m-masataka
Copy link
Contributor

@m-masataka m-masataka commented Apr 11, 2021

Add silence filter by "createdBy" item.

filter-demo.mov

related to #2476

Signed-off-by: m-masataka <[email protected]>
Signed-off-by: m-masataka <[email protected]>
@stale stale bot added the stale label Jun 10, 2021
@stale stale bot removed the stale label Jul 17, 2021
@roidelapluie
Copy link
Member

Hello @w0rm

Could you fancy a chance to review this from a Elm perspective? Thanks!

@w0rm
Copy link
Member

w0rm commented Aug 8, 2021

@m-masataka @roidelapluie apologies for the delay. Great work as always, looks like a pretty useful feature to me!

I wonder about the necessity to serialise the list of creators to a string and back: {creator1,creator2} vs passing things in URL encoded query vars, e.g. creator=creator1&creator=creator2. The current implementation feels a bit error prone to me, and we have to deal with things like string escaping and the allowlist of characters, while the original input field for the silence creator doesn't have these restrictions:

isVarCharAndSymbol char =
     Char.isLower char
         || Char.isUpper char
         || String.contains (String.fromChar char) "_ .&-^[]:;*+?!#$%&()"
         || Char.isDigit char

Another question I have is usability point of view.

  • Do we need to filter by multiple creators? The current created by filter is joined with "OR", that works differently when filtering alerts by matchers, that are joined with "AND".
  • The creator field can be any text, there may be typos and extra spaces, not sure this can work reliably. Maybe allowing filtering by regex would mitigate some of that.

From the UI point of view, the button that toggles the created by bar looks a bit weirdly positioned to me, but I think it would be OK to get this merged, and then I can improve (would like to play with a few ideas myself).

@stale stale bot added the stale label Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants