feat(conf): add optional field_labels to override Labels#189
feat(conf): add optional field_labels to override Labels#189cropalato wants to merge 1 commit intoprometheus-community:masterfrom
Conversation
cropalato
force-pushed
the
master
branch
4 times, most recently
from
ab7b098 to
cd931b9
Compare
Signed-off-by: Ricardo Melo <ricardo@cropa.ca>
|
What about this, is this going to get merged into prometheus alertmanager? Very useful and needed feature for jiralert. |
bwplotka
left a comment
bwplotka
left a comment
There was a problem hiding this comment.
This is great, thanks!
Some suggestions only
| return string(b) | ||
| } | ||
|
|
||
| // GetJiraFieldKey returns the jira key associated to a field. |
There was a problem hiding this comment.
Comment is different vs actual function signature -- GetJiraFieldKey only returns error or nil.
| } | ||
|
|
||
| // GetJiraFieldKey returns the jira key associated to a field. | ||
| func (c *Config) GetJiraFieldKey() error { |
There was a problem hiding this comment.
Do you mind moving this function out of the config.go? It would be epic to separate JIRA API integrations vs config logic. Also let's avoid extra config state that does not belong to user config directly - it's ok to store this fieldkey as variable somewhere.
Also probably you mean ValidateJIRAField or something - it's ok to have validateXYZ function in cmd/ somewhere 🤔
| if c.Defaults.FieldLabels != "" { | ||
| rc.FieldLabels = c.Defaults.FieldLabels | ||
| } else { | ||
| rc.FieldLabels = "Labels" |
There was a problem hiding this comment.
We might need constant variable for this
| projectList := "'" + strings.Join(projects, "', '") + "'" | ||
| query := fmt.Sprintf("project in(%s) and labels=%q order by resolutiondate desc", projectList, issueLabel) | ||
| if r.conf.FieldLabels == "Labels" { | ||
| labelKey = "labels" |
There was a problem hiding this comment.
Why not using labels as a default value (instead of Labels) then?
| # Define the jira field used by jiralert to avoid ticket duplication. Optional (default: Labels) | ||
| field_labels: Labels |
There was a problem hiding this comment.
Hm, what about id_field?
| # Define the jira field used by jiralert to avoid ticket duplication. Optional (default: Labels) | |
| field_labels: Labels | |
| # The jira field used by jiralert for the unique hash and optional grouping and static labels that identify the ticket for reuse. It's recommended to keep using labels field, but some users prefer custom fields (see issue #164). Optional (default: labels). | |
| id_field: labels |
|
|
||
| if len(data.Alerts.Firing()) == 0 { | ||
| level.Debug(r.logger).Log("msg", "no firing alert; nothing to do.", "label", issueGroupLabel) | ||
| level.Debug(r.logger).Log("msg", "no firing alert; nothing to do.", r.conf.FieldLabels, issueGroupLabel) |
There was a problem hiding this comment.
Not sure if we should use IDLabel value for logging key. What if somebody will have custom field called msg? Can we just use id here?
Also I would rename issueGroupLabel everywhere to issueIDLabel because at this point it might be hash, static and maybe group labels. "Group" is a bit misleading given the add_group_labels config field which only referes to Prometheus labels. 🙈 not an ideal name, but not worth changing (unless we want to deprecate)
|
Also please rebase as we updated CI. |
3 participants
It adds an optional parameter to replace Labels field with a custom field.
It should be enough to fix #164.