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

Feature: Notify about failed updates, block detectable bad updates #7188

Merged
merged 6 commits into from Aug 26, 2017

Conversation

Projects
None yet
5 participants
@murrant
Member

murrant commented Aug 20, 2017

Ability to post notifications when the update fails.
Detect and roll back updates that will cause broken installs. (Needs testing)
Add severity to notifications, critical (2) notifications will display a toast.

This will be used for removing in-tree dependencies and raising the minimum php version.

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Aug 20, 2017

Thank you for submitting a PR @murrant! We have found the following @f0o, @laf and @Gorian based on the history of these files to review this PR.

mention-bot commented Aug 20, 2017

Thank you for submitting a PR @murrant! We have found the following @f0o, @laf and @Gorian based on the history of these files to review this PR.

Feature: Notify about failed updates, block detectable bad updates
Ability to post notifications when the update fails.
Detect and roll back updates that will cause broken installs. (Needs testing)
Add severity to notifications, critical (2) notifications will display a toast.

This will be used for removing in-tree dependencies and raising the minimum php version.
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Aug 21, 2017

Member

Does this just post one failed update notice or would it keep posting them each day?

Member

laf commented Aug 21, 2017

Does this just post one failed update notice or would it keep posting them each day?

@f0o

This comment has been minimized.

Show comment
Hide comment
@f0o

f0o Aug 22, 2017

Member

It looks like it would create a new notification for each failure

Member

f0o commented Aug 22, 2017

It looks like it would create a new notification for each failure

Improve naming a bit add phpdoc to new_notification
In case multiple notifications are created, remove them all.
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Aug 24, 2017

Member

@f0o @laf The new_notification function does not allow new notifications with the same checksum. So if the title and message are the same, it will not be created.

It will post one notification and remove it upon successful update. So, users are allowed to mark it as read. If the user marks it as read it won't show in their notification count, but it will still show a toast on page load.

Note that if users don't have updates enabled, they will not get a notification. (I also just pushed a change to have it remove any existing notifications in that case)

Member

murrant commented Aug 24, 2017

@f0o @laf The new_notification function does not allow new notifications with the same checksum. So if the title and message are the same, it will not be created.

It will post one notification and remove it upon successful update. So, users are allowed to mark it as read. If the user marks it as read it won't show in their notification count, but it will still show a toast on page load.

Note that if users don't have updates enabled, they will not get a notification. (I also just pushed a change to have it remove any existing notifications in that case)

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Aug 25, 2017

Member

FYI, I would like to get this merged in 1.31 because it is a pre-requisite for other work.

Member

murrant commented Aug 25, 2017

FYI, I would like to get this merged in 1.31 because it is a pre-requisite for other work.

@murrant murrant added this to the 1.31 milestone Aug 25, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Aug 25, 2017

Member

I'm unsure why travis hasn't picked this up:

MariaDB [librenms]> ALTER TABLE `notifications` ADD `severity` INT DEFAULT 0 NULL COMMENT '0=ok,1=warning,2=critical' AFTER `body`;
ERROR 1292 (22007): Incorrect datetime value: '0000-00-00' for column 'datetime' at row 13

That's after setting the sql mode to what we run in tests (I run that in my dev anyway)

Member

laf commented Aug 25, 2017

I'm unsure why travis hasn't picked this up:

MariaDB [librenms]> ALTER TABLE `notifications` ADD `severity` INT DEFAULT 0 NULL COMMENT '0=ok,1=warning,2=critical' AFTER `body`;
ERROR 1292 (22007): Incorrect datetime value: '0000-00-00' for column 'datetime' at row 13

That's after setting the sql mode to what we run in tests (I run that in my dev anyway)

laf added some commits Aug 26, 2017

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Aug 26, 2017

The inspection completed: 2 updated code elements

scrutinizer-notifier commented Aug 26, 2017

The inspection completed: 2 updated code elements

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Aug 26, 2017

Member

Ok updated, pretty sure as well the mysql checks were never running before so that's now fixed.

Member

laf commented Aug 26, 2017

Ok updated, pretty sure as well the mysql checks were never running before so that's now fixed.

@laf laf merged commit 0def643 into librenms:master Aug 26, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@murrant murrant deleted the murrant:update-blocker branch Oct 13, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 17, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.