-
Notifications
You must be signed in to change notification settings - Fork 21
Drop some superfluous Redis -> DB value mappings #964
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
base: main
Are you sure you want to change the base?
Conversation
9f60050
to
85b55ab
Compare
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.
First, thanks for this PR. In general, it looks good.
I went over the code - and its counterpart in Icinga 2 - multiple times and added some small comments.
Furthermore, I did some testing in a playground setup and clicking a non-sticky and a sticky in Icinga DB Web resulted in the expected data within the relation database.
MariaDB [icingadb]> select comment.text, is_acknowledged, is_sticky_acknowledgement from service_state join comment on service_state.acknowledgement_comment_id = comment.id where is_acknowledged = 'y'\G
*************************** 1. row ***************************
text: F
is_acknowledged: y
is_sticky_acknowledgement: n
*************************** 2. row ***************************
text: sticky F
is_acknowledged: y
is_sticky_acknowledgement: y
2 rows in set (0.004 sec)
85b55ab
to
ca18d4c
Compare
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.
Looks good so far, thanks!
After this PR's counterpart against Icinga 2 was properly reviewed and merged, I would take another look. Consider this a soft approve.
However, my comment regarding the different column order in PostgreSQL remains; #964 (comment). I would like to hear some opinions if we should resolve this or treat this as an annoyance to be ignored.
ca18d4c
to
3a20d8d
Compare
dd51836
to
7b2fd34
Compare
5026371
to
7fdc513
Compare
d15e93d
to
0b5af71
Compare
50f67f6
to
7cf3c9c
Compare
That was my final push :). So please have another look again! |
7cf3c9c
to
c7da5ed
Compare
Icinga 2 sets now the `state_type` field to either `hard` or `soft`, thus the mappings within the Icinga DB code is no longer needed.
c7da5ed
to
c6a569e
Compare
c6a569e
to
ea17fa4
Compare
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.
LGTM.
Went over the code another time, tested it together with Icinga 2 based on your other PR, tested updated icingadb-migrate
command. Nothing fell apart. Thanks!
This PR is basically the same as Icinga/icinga2#10452 and removes the now superfluous Redis -> DB value mappings, and also makes the
{host,service}_state.is_acknowledged
column a simpleboolenum
and introduces a newis_sticky_acknowledgement
column to indicate sticky acknowledgements.Requires