Workaround for unknown ALERT{} label length #79
Conversation
erdii
force-pushed
the
hash-issue-label
branch
2 times, most recently
from
3a52b80
to
e166f5a
Compare
|
I've prepared a commit that by default disables the hashing behavior and add a flag |
|
Nice! I will look at it, feel free to DM me offline as well 🤗 |
There was a problem hiding this comment.
Thanks for this!
That will work, just wonder if that statement is true:
Since the ALERT{...} label is not intended for human consumption, I would like to propose using a computed hash value instead:
🤗 Such label will scare people... but maybe there is no better choice. We could put something like this in Ticket description e.g, and do filtering on our side. I think I am find with this for now (:
LGTM!
pkg/notify/notify.go
Outdated
| buf.WriteString(p.Name) | ||
| buf.WriteString(fmt.Sprintf("=%q,", p.Value)) | ||
| kvString := fmt.Sprintf("%s=%q,", p.Name, p.Value) | ||
| hash.Write([]byte(kvString)) // nolint:errcheck // hash.Write can never return an error |
There was a problem hiding this comment.
Well done on nolint and comment 🤗
There was a problem hiding this comment.
Actually, there might better way! (:
| hash.Write([]byte(kvString)) // nolint:errcheck // hash.Write can never return an error | |
| _ = hash.Write([]byte(kvString)) |
|
will hide new behavior behind flag and in the long term deprecate the old behavior and the flag |
erdii
force-pushed
the
hash-issue-label
branch
2 times, most recently
from
1d93af7
to
f46fb82
Compare
|
I added the flag This is ready for another review @bwplotka :) |
README.md
Outdated
| @@ -52,6 +52,9 @@ $ curl -H "Content-type: application/json" -X POST \ | |||
|
|
|||
| ## Configuration | |||
|
|
|||
| Attention: We have introduced a new behavior in [#79](https://github.com/prometheus-community/jiralert/pull/79) for building the string that is used as an issue label in jira so jiralert can find an issue it opened. The new behavior ensures that at all times the string is a legal issue label in the jira api. **This change is opt-in** to avoid breaking users upgrading from older releases, but we will remove the old behavior + flag in a future release. | |||
There was a problem hiding this comment.
That should probably go to CHANGELOG.md instead 🤗
There was a problem hiding this comment.
- add current changelog for 1.1 #83 adds a changelog and is now a prerquisite for this pr
- this pr adds a new entry in the changelog
Signed-off-by: Josh Gwosdz <jgwosdz@redhat.com>
and disable errcheck linting for hash.Write call Signed-off-by: Josh Gwosdz <jgwosdz@redhat.com>
Signed-off-by: Josh Gwosdz <jgwosdz@redhat.com>
Signed-off-by: Josh Gwosdz <jgwosdz@redhat.com>
Signed-off-by: Josh Gwosdz <jgwosdz@redhat.com>
Signed-off-by: Josh Gwosdz <jgwosdz@redhat.com>
Signed-off-by: Josh Gwosdz <jgwosdz@redhat.com>
Signed-off-by: Josh Gwosdz <jgwosdz@redhat.com>
Signed-off-by: Josh Gwosdz <jgwosdz@redhat.com>
|
🎉 thank you for your reviews and guidance @bwplotka <3 |
Heyo 👋
I'm currently working on incorporating jiralert in our alerting-pipeline and have repeatedly run into a problem: JIRA has a length limit of 255 chars for a single issue label
Since the
ALERT{...}label is not intended for human consumption, I would like to propose using a computed hash value instead:ALERT{+sha512sum(groupLabels)+}this way, it is ensured, that jiralert will never pass an invalid label because the length is fixed.
What do you think? Do you think this breaking change is acceptable?