-
Notifications
You must be signed in to change notification settings - Fork 151
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
Implement "Create Silence" Feature as a Modal #523
Implement "Create Silence" Feature as a Modal #523
Conversation
bai1024
commented
Jun 19, 2024
•
edited
Loading
edited
- Modal Implementation : Change the current setting interface to a modal design.
- Modal Features : Within the modal, Provide input fields to set Silence
- Support Flexible Silence Setting : Add silence setting field(Matcher), and target field. Matcher field is working like the behavior of Grafana's one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First review spin done. Nice job :)
Please have a look at my code suggestions and comments.
Thank you very much.
8fd12b1
to
296570e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated everything according to the first review cycle and squashed the commits. Please check.
Second review spin finished.
In some parts I could do GitHub's code suggestion that are easier to commit. In other parts that was not possible due to GitHub limitations.
Please check.
af63811
to
0cd465c
Compare
0cd465c
to
3eb1d00
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
<ul class="list-inline promgen-labels-list"> | ||
<li v-for="(value, label) in state.labels"> | ||
<div class="promgen-label-target"> | ||
<span>[[label]]=~[[value]]</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is somewhat incorrect. We are not always using =~
matches for everything. I think only the instance
field is a =~
while others should generally be a regular =
match 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little buried but
Lines 289 to 296 in 32f1bbd
kwargs["matchers"] = [ | |
{ | |
"name": name, | |
"value": value, | |
"isEqual": True, # Right now we only support =~ | |
"isRegex": True if value.endswith("*") else False, | |
} | |
for name, value in labels.items() |
The only time it is a =~
is when our matcher ends with *
and in Promgen that is generally only for our quick silence for instance/host