Skip to content
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

add canceled downtimes to the history, if they were started #5184

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

Robnarok
Copy link
Contributor

@Robnarok Robnarok commented Feb 9, 2024

fixes #5176

Copy link

cla-bot bot commented Feb 9, 2024

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @Robnarok

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

  • If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

@Robnarok
Copy link
Contributor Author

Robnarok commented Feb 9, 2024

i have signed the CLA

@bobapple
Copy link
Member

bobapple commented Feb 9, 2024

@cla-bot check

@cla-bot cla-bot bot added the cla/signed label Feb 9, 2024
@nilmerg
Copy link
Member

nilmerg commented Feb 9, 2024

@gianlucapiccolo @mcodato This is the exact opposite of db9888b. Please have a look here and add your thoughts.

@Robnarok
Copy link
Contributor Author

Robnarok commented Feb 9, 2024

i took another look at the implementation, and if i understand it correctly, the current behavior is:

if a downtime has started and is not cancelled it is displayed
if a downtime has started and is cancelled it is NOT displayed
if a downtime has NOT started and is cancelled it is NOT displayed
if a downtime has NOT started and is NOT cancelled it is NOT displayed

removing the AND part like i did in the current PR, it would lead to

if a downtime has started and is NOT cancelled it is displayed
if a downtime has started and is cancelled it is displayed
if a downtime has NOT started and is cancelled it is NOT displayed
if a downtime has NOT started and is NOT cancelled it is NOT displayed

or a third solution could be if we would replace the AND with an OR (was_started=1 OR was_cancelled=0) we would get something like

if a downtime has started and is NOT cancelled it is displayed
if a downtime has started and is cancelled it is displayed
if a downtime has NOT started and is cancelled it is NOT displayed
if a downtime has NOT started and is NOT cancelled it is displayed

(Edit: And prior to db9888b all of the 4 cases were displayed in the history)

@mcodato
Copy link
Contributor

mcodato commented Feb 9, 2024

Hi @Robnarok, I see your point, the change we made was for another very annoying bug, let me do some internal tests and talk about it with my colleagues and I will let you know as soon as possible, thank you

@mcodato
Copy link
Contributor

mcodato commented Feb 12, 2024

@Robnarok Thank you for your patience, the tests showed that your PR is fine for us, as it keeps the was_started = 1 :)

@nilmerg nilmerg added this to the 2.12.2 milestone Feb 12, 2024
@nilmerg nilmerg self-requested a review February 12, 2024 15:53
@nilmerg
Copy link
Member

nilmerg commented Feb 12, 2024

@Robnarok Please rebase with main, this should fix the actions

@Robnarok
Copy link
Contributor Author

@mcodato Thank you for testing :)

@nilmerg i did the rebase and the actions passed 🥳

@nilmerg nilmerg merged commit dba77bc into Icinga:main Apr 5, 2024
15 checks passed
@nilmerg nilmerg added the bug Something isn't working label Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla/signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

downtimes, which were started and cancled, are missing in the history
4 participants