Add ability to update in comments (now configurable per receiver) #180
Conversation
twotired
force-pushed
the
updates-in-comments2
branch
from
eca0730 to
f9637fc
Compare
|
@bwplotka @MytkoEnko @cropalato @holger-waschke Please review this new PR for adding updates in comments with better configurability and additional tests. |
pkg/config/config.go
Outdated
| @@ -47,6 +47,36 @@ func (s *Secret) UnmarshalYAML(unmarshal func(interface{}) error) error { | |||
| return unmarshal((*plain)(s)) | |||
| } | |||
|
|
|||
| // NullBool will be used for boolean configuration values that should not | |||
| // default to false when undefined and the default setting is true. | |||
| type NullBool struct { | |||
There was a problem hiding this comment.
Do we really need this? None of other boolean flags required this. Essentially if default is false, it's not needed, right? And we plan to use default false at the moment 🤔
There was a problem hiding this comment.
If we use a normal bool and the user sets the value to true in the defaults section, and leaves it blank in the receiver configs, the receiver config setting will actually be interpreted as false. By using NullBool, we are able to honor the global setting in the defaults section, which to me, seems more intuitive than allowing users to specify a default setting that does not actually change the behavior of the application.
There was a problem hiding this comment.
Got it, thanks--good point about that logic. However wouldn't using *bool fix the problem. Also if we fix it for this field let's do this for all bools that can be in default fields for consistency.
Alternatively we could make it a bool (and accept the bug) and move those bools to *bool or your struct in separate PR, wdyt? I would love to avoid complexity of some fields of the same type having different logic.
There was a problem hiding this comment.
I updated the PR to use bool* instead of NullBool and applied the same logic to AddGroupLabels so that default will be honored.
| numComments = len(issue.Fields.Comments.Comments) | ||
| } | ||
| if numComments > 0 && issue.Fields.Comments.Comments[(numComments-1)].Body == issueDesc { | ||
| // if the new comment is identical to the most recent comment, |
|
That looks good, I will test it in our environment soon. One idea would be to provide a seperate template for the comments. per example in our case we only want to post the annotations but not the labels as comment. |
@holger-waschke any problems discovered in testing? |
|
Yes it works just fine. One issue we faced were too long comments, as we have dozen of static labels. I added some code to cut anything but the annotations, this is were changes occur in our case. But that's not suitable for everyone, you think it's a good idea to implement a template which applies only for comments? |
pkg/config/config.go
Outdated
| @@ -47,6 +47,36 @@ func (s *Secret) UnmarshalYAML(unmarshal func(interface{}) error) error { | |||
| return unmarshal((*plain)(s)) | |||
| } | |||
|
|
|||
| // NullBool will be used for boolean configuration values that should not | |||
There was a problem hiding this comment.
| // NullBool will be used for boolean configuration values that should not | |
| // NullBool will be used for boolean configuration values that can have 3 values: true, false and unspecified. |
There was a problem hiding this comment.
Can we simply use *bool instead though?
pkg/config/config.go
Outdated
| @@ -47,6 +47,36 @@ func (s *Secret) UnmarshalYAML(unmarshal func(interface{}) error) error { | |||
| return unmarshal((*plain)(s)) | |||
| } | |||
|
|
|||
| // NullBool will be used for boolean configuration values that should not | |||
| // default to false when undefined and the default setting is true. | |||
| type NullBool struct { | |||
There was a problem hiding this comment.
Got it, thanks--good point about that logic. However wouldn't using *bool fix the problem. Also if we fix it for this field let's do this for all bools that can be in default fields for consistency.
Alternatively we could make it a bool (and accept the bug) and move those bools to *bool or your struct in separate PR, wdyt? I would love to avoid complexity of some fields of the same type having different logic.
… for each Receiver. Signed-off-by: Jason Wells <[email protected]>
Co-authored-by: Bartlomiej Plotka <[email protected]> Signed-off-by: Jason Wells <[email protected]>
twotired
force-pushed
the
updates-in-comments2
branch
from
44cdf24 to
5de4ce3
Compare
…bels Signed-off-by: Jason Wells <[email protected]>
twotired
force-pushed
the
updates-in-comments2
branch
from
66a32ba to
28c7421
Compare
|
@bwplotka please review... I switched from NullBool to bool* |
As an alternative, or as an addition to updating the Description, additional Alert updates should go to comments.
The default behavior is to update the JIRA Description when the Alert details change. This makes it difficult if the viewers of the JIRA are researching each failure because it is not obvious which alerts are new or existing. Depending on the Alert expression, old firing details may also be removed.
Additionally, adding too many firing details to the description increases the likelihood of exceeding the 32KB char limit for the JIRA Description.
By optionally sending each update to a Comment, viewers can better keep track of which activity is new, and it will be possible to continue adding firing details without exceeding the Description char limit.
resolves #160