Deduplicate embedded ansible notifications #17394
Conversation
| return | ||
| end | ||
|
|
||
| create_notification(notification_type) unless notifications.any? do |n| |
There was a problem hiding this comment.
We can probably handle the if notifications.empty? case here. notifications.any? should be nearly as fast as notifications.empty? or the speed difference should not warrant the mental cost of an early return and extra conditional. Of course, I assume it would work by just removing the entire if notifications.empty? condition.
There was a problem hiding this comment.
Unfortunately [].any? always returns false, so we can't remove the empty? check. I had a version of this where I combined the conditions, but this felt easier to read.
There was a problem hiding this comment.
oh yeah, lol. I inverted my truth table.
There was a problem hiding this comment.
Yeah, this one took some work to make it even a bit readable. Still looking for improvements if you've got anything.
I could also add comments if it would be helpful.
This commit changes the notification behavior so that a new notification is only created when either there is no existing one for that server or the existing one has been read. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1486695
carbonin
force-pushed
the
dedup_embedded_ansible_notifications
branch
from
48f6618 to
92884fe
Compare
|
Checked commits carbonin/manageiq@a6b0434~...92884fe with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
@carbonin add your labels, is it only gaprindashvili/yes? |
|
@jrafanie the bug doesn't have any backport flags, but I guess this can go back |
This PR changes the Embedded Ansible role notification behavior so that a new notification is only created when either there is no existing one for that server or the existing one has been read.
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1486695