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

Recreate silence with previous comment. #1927

Merged
merged 3 commits into from
Aug 27, 2019

Conversation

m-masataka
Copy link
Contributor

When recreate silence by expired one, we want to copy over the previous comment and matchers. ( not only matchers)

This PR is related to #1566.

Signed-off-by: m-masataka m.mizukoshi.wakuwaku@gmail.com

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>
@@ -114,7 +117,7 @@ editButton silence =
Expired ->
a
[ class "btn btn-outline-info border-0"
, href (newSilenceFromMatchers silence.matchers)
, href recreateUrl
Copy link
Member

@w0rm w0rm Aug 4, 2019

Choose a reason for hiding this comment

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

Hey! What do you think about reusing the new silence page and extending it to support passing an optional comment field rather than introducing the whole new route?

i.e. changing this to newSilenceFromMatchersAndComment silence.matchers silence.comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@w0rm Thank you for making a comment.
The reason why I introduce whole new route is to deal with possibilities change of SilenceView's field. This means to Avoid implement newSilenceFromMacherAndCommentAndXXXAndXXX..etc , when anyone need to add field in SilenceForm.

If low probability of extending new field in the future, implement newSilenceFromMatchersAndComment is simple and best way.
So, I can't make up my mind which is better.
Which do you think is better?

Copy link
Member

Choose a reason for hiding this comment

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

Hey! Sorry for taking so long to respond. newSilenceFromMatchersAndComment sounds like a simpler and better option to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey.
I’ll change this PR.
Wait a moment please.

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>
Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>
@w0rm w0rm merged commit c1040d5 into prometheus:master Aug 27, 2019
@w0rm
Copy link
Member

w0rm commented Aug 27, 2019

@m-masataka thank you!

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.

4 participants