Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix labels for Service Monitors #2878
Fix labels for Service Monitors #2878
Changes from 15 commits
1625097
78f1315
9ccd87f
5f55a01
e3174b6
edfb463
82fefa2
28baa37
7dcaadb
2e143ee
cc96402
978dfa0
f5b29d8
fff9870
6432371
2469162
0b2a439
3712c43
743247b
552cb3e
720c6b1
d1889d0
cc84a10
e5b251c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
personal opinion but this statement can be written in a more readable way
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.
and I would move the logger creation close to returning
false
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.
Why a double condition + no log message is easier to read than the current approach?
The current is easy to follow and understand why the method returns
false
.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 single log message with all parameters makes it clear to understand the overall state.
The function has 2 states (
true
andfalse
), aligning the condition to it makes it easier to understandThere 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.
@iblancasa i think we should keep these around for now, mark them as deprecated and remove them in two+ versions rather than deleting them outright which would constitute a breaking change.
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.
Done