Resolve "Add health check for dbbackup" #2140

Merged
hansegucker merged 3 commits from 378-add-health-check-for-dbbackup into master 2021-02-21 21:04:11 +01:00
Owner

Closes #378

Closes #378
Author
Owner

added 1 commit

  • da26944f - Add health checks for media and db backups. Closes #378

Compare with previous version

added 1 commit <ul><li>da26944f - Add health checks for media and db backups. Closes #378</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4853&start_sha=26fabd33ca43c29529133b5e9e9d6d3d03f1b252)
Author
Owner

requested review from @hansegucker

requested review from @hansegucker
Author
Owner

assigned to @nik and unassigned @debdolph

assigned to @nik and unassigned @debdolph
Author
Owner

marked this merge request as ready

marked this merge request as **ready**
Owner

approved this merge request

approved this merge request
Owner

unapproved this merge request

unapproved this merge request
Owner

As the two newly added system checks share a lot of code, I would suggest to use a common base class.

As the two newly added system checks share a lot of code, I would suggest to use a common base class.
Author
Owner

added 1 commit

  • 085cbbb5 - Add health checks for media and db backups. Closes #378

Compare with previous version

added 1 commit <ul><li>085cbbb5 - Add health checks for media and db backups. Closes #378</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4854&start_sha=da26944f835d3b9d620133c52c1696f3d939e052)
Author
Owner

added 1 commit

  • 8810d676 - Add health checks for media and db backups. Closes #378

Compare with previous version

added 1 commit <ul><li>8810d676 - Add health checks for media and db backups. Closes #378</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4855&start_sha=085cbbb519b4643f4c287fefbbe376c6ccd8637e)
Owner

This should not inherit from BaseDbBackupCommand as this is not a management command.

This should not inherit from `BaseDbBackupCommand` as this is not a management command.
Owner

This should not inherit from BaseDbBackupCommand as this is not a management command.

This should not inherit from `BaseDbBackupCommand` as this is not a management command.
Owner

This import name is to generic.

Please import as dbbackup_utils or, even better, just import dbbackup itself and use the rest of it namespaced.

This import name is to generic. Please import as `dbbackup_utils` or, even better, just import `dbbackup` itself and use the rest of it namespaced.
Owner

Is this DBBACKUP_SECONDS by intention. i.e. timeout is intended to be the same for dbbackup and media backup?

Is this `DBBACKUP_SECONDS` by intention. i.e. timeout is intended to be the same for dbbackup and media backup?
Owner

Why do you convert the date to a string and then back to a datetime?

Why do you convert the date to a string and then back to a datetime?
Owner

Why do you convert the date to a string and then back to a datetime?

Why do you convert the date to a string and then back to a datetime?
Owner

This is not precise. It checks whether the last backup was run a defined timespan ago.

Are you sure that existence of a backup file means the backup was successful?

This is not precise. It checks whether the last backup was run a defined timespan ago. Are you sure that existence of a backup file means the backup was successful?
Owner

This is not precise. It checks whether the last backup was run a defined timespan ago.

Are you sure that existence of a backup file means the backup was successful?

This is not precise. It checks whether the last backup was run a defined timespan ago. Are you sure that existence of a backup file means the backup was successful?
Owner

Can/should we derive this fro mthe configured backup plan somehow?

Can/should we derive this fro mthe configured backup plan somehow?
Author
Owner

yes

yes
Author
Owner

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4866&start_sha=8810d6760ac131b4ebcc44a3e8c370ddefe20950#358275720d816cfb0e6a4145a70f46181f8b430e_27_27)
Author
Owner

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4866&start_sha=8810d6760ac131b4ebcc44a3e8c370ddefe20950#358275720d816cfb0e6a4145a70f46181f8b430e_46_46)
Author
Owner

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4866&start_sha=8810d6760ac131b4ebcc44a3e8c370ddefe20950#358275720d816cfb0e6a4145a70f46181f8b430e_6_6)
Author
Owner

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4866&start_sha=8810d6760ac131b4ebcc44a3e8c370ddefe20950#358275720d816cfb0e6a4145a70f46181f8b430e_36_35)
Author
Owner

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4866&start_sha=8810d6760ac131b4ebcc44a3e8c370ddefe20950#358275720d816cfb0e6a4145a70f46181f8b430e_55_54)
Author
Owner

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4866&start_sha=8810d6760ac131b4ebcc44a3e8c370ddefe20950#358275720d816cfb0e6a4145a70f46181f8b430e_47_46)
Author
Owner

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4866&start_sha=8810d6760ac131b4ebcc44a3e8c370ddefe20950#358275720d816cfb0e6a4145a70f46181f8b430e_28_27)
Author
Owner

added 1 commit

  • 5535258f - Update health_checks.py

Compare with previous version

added 1 commit <ul><li>5535258f - Update health_checks.py</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4866&start_sha=8810d6760ac131b4ebcc44a3e8c370ddefe20950)
Author
Owner

resolved all threads

resolved all threads
Author
Owner

added 31 commits

  • 5535258f...d23be3b6 - 30 commits from branch master
  • 97148608 - Merge branch 'master' into 378-add-health-check-for-dbbackup

Compare with previous version

added 31 commits <ul><li>5535258f...d23be3b6 - 30 commits from branch <code>master</code></li><li>97148608 - Merge branch &#39;master&#39; into 378-add-health-check-for-dbbackup</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4867&start_sha=5535258fad13fd076ea1a200e7a630cdf0ca3516)
Author
Owner

added 1 commit

  • b42c86f8 - Merge branch 'master' into 378-add-health-check-for-dbbackup

Compare with previous version

added 1 commit <ul><li>b42c86f8 - Merge branch &#39;master&#39; into 378-add-health-check-for-dbbackup</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4868&start_sha=971486084cde7f677c83c3a3e06b79fe7ce313bf)
Author
Owner

added 1 commit

  • d777cf92 - Merge branch 'master' into 378-add-health-check-for-dbbackup

Compare with previous version

added 1 commit <ul><li>d777cf92 - Merge branch &#39;master&#39; into 378-add-health-check-for-dbbackup</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4869&start_sha=b42c86f8d1ca6caac248e7a33ce6d8b82c6d5c02)
Author
Owner

added 1 commit

  • b7d863a1 - Add check if last job was successful

Compare with previous version

added 1 commit <ul><li>b7d863a1 - Add check if last job was successful</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4871&start_sha=d777cf922f8c3f0577dd380b218ce357e45e61b7)
Author
Owner

added 1 commit

  • a172aa90 - Add check if last job was successful

Compare with previous version

added 1 commit <ul><li>a172aa90 - Add check if last job was successful</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4872&start_sha=b7d863a1545dec38c71e4f6d58f13f5b8c7b4c36)
Author
Owner

added 1 commit

  • 7dc85759 - Add check if last job was successful

Compare with previous version

added 1 commit <ul><li>7dc85759 - Add check if last job was successful</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4873&start_sha=a172aa904057f4ec922a468a43e7a0f1294cc984)
Owner

I second that.

I second that.
Owner

Why query the database twice?

Why query the database twice?
Owner

This will fail with the code above if no task is found (NameError because task was never set). Fixing the comment above by just calling last() unconditionally implicitly solves this.

This will fail with the code above if no task is found (`NameError` because task was never set). Fixing the comment above by just calling `last()` unconditionally implicitly solves this.
Owner

Why?

Why?
Author
Owner

changed this line in version 11 of the diff

changed this line in [version 11 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4885&start_sha=7dc857593513fb8c5f53b403b52865206c0369cf#358275720d816cfb0e6a4145a70f46181f8b430e_74_71)
Author
Owner

changed this line in version 11 of the diff

changed this line in [version 11 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4885&start_sha=7dc857593513fb8c5f53b403b52865206c0369cf#358275720d816cfb0e6a4145a70f46181f8b430e_77_74)
Author
Owner

added 1 commit

  • edcd4b25 - Fix check of TaskResult

Compare with previous version

added 1 commit <ul><li>edcd4b25 - Fix check of TaskResult</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4885&start_sha=7dc857593513fb8c5f53b403b52865206c0369cf)
Author
Owner

changed this line in version 12 of the diff

changed this line in [version 12 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4886&start_sha=edcd4b25ede782717b744efcf670c0d40d755f0e#358275720d816cfb0e6a4145a70f46181f8b430e_58_55)
Author
Owner

resolved all threads

resolved all threads
Author
Owner

added 1 commit

  • ff9daec6 - Add health checks for media and db backups. Closes #378

Compare with previous version

added 1 commit <ul><li>ff9daec6 - Add health checks for media and db backups. Closes #378</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4886&start_sha=edcd4b25ede782717b744efcf670c0d40d755f0e)
Author
Owner

added 1 commit

  • eb80b193 - Add health checks for media and db backups. Closes #378

Compare with previous version

added 1 commit <ul><li>eb80b193 - Add health checks for media and db backups. Closes #378</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4887&start_sha=ff9daec69fea5a2c6976cf028045eb948881b5ec)
Owner

Please split this up for db backup and media backup.

Please split this up for db backup and media backup.
Author
Owner

done

done
Author
Owner

added 1 commit

  • cc241b84 - Use seperate settings for db and media backup

Compare with previous version

added 1 commit <ul><li>cc241b84 - Use seperate settings for db and media backup</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4909&start_sha=eb80b19383c4e0459b83ff489fcff0b5d6132da1)
Author
Owner

resolved all threads

resolved all threads
Owner

approved this merge request

approved this merge request
Owner

This should be part of the sub class. Inheritance was invented to encapsulate code within its respective class, not to do sub-class logic in a base class.

This should be part of the sub class. Inheritance was invented to encapsulate code within its respective class, not to do sub-class logic in a base class.
Owner

Didn't you say this was unnecessary?

Didn't you say this was unnecessary?
Owner

Didn't you say this was unnecessary?

Didn't you say this was unnecessary?
Owner

Didn't you say this was unnecessary?

Didn't you say this was unnecessary?
Owner

This still uses the same parameter from the config file.

This still uses the same parameter from the config file.
Author
Owner

changed this line in version 15 of the diff

changed this line in [version 15 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4921&start_sha=cc241b84266269603e5335ebe3fa9bb30ab77ccf#abc9d828b78bd9d36e05475d0bc91119bc5bc6b0_733_733)
Author
Owner

changed this line in version 15 of the diff

changed this line in [version 15 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4921&start_sha=cc241b84266269603e5335ebe3fa9bb30ab77ccf#358275720d816cfb0e6a4145a70f46181f8b430e_52_51)
Author
Owner

changed this line in version 15 of the diff

changed this line in [version 15 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4921&start_sha=cc241b84266269603e5335ebe3fa9bb30ab77ccf#358275720d816cfb0e6a4145a70f46181f8b430e_61_57)
Author
Owner

changed this line in version 15 of the diff

changed this line in [version 15 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4921&start_sha=cc241b84266269603e5335ebe3fa9bb30ab77ccf#358275720d816cfb0e6a4145a70f46181f8b430e_79_71)
Author
Owner

changed this line in version 15 of the diff

changed this line in [version 15 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4921&start_sha=cc241b84266269603e5335ebe3fa9bb30ab77ccf#abc9d828b78bd9d36e05475d0bc91119bc5bc6b0_734_733)
Author
Owner

added 1 commit

  • bacd3df6 - Use seperate settings for db and media backup

Compare with previous version

added 1 commit <ul><li>bacd3df6 - Use seperate settings for db and media backup</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4921&start_sha=cc241b84266269603e5335ebe3fa9bb30ab77ccf)
Author
Owner

resolved all threads

resolved all threads
Author
Owner

changed this line in version 16 of the diff

changed this line in [version 16 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4922&start_sha=bacd3df64640ccaa0a0cafaa5f2032d9b3f813fc#358275720d816cfb0e6a4145a70f46181f8b430e_39_37)
Author
Owner

added 1 commit

  • 1727a4a3 - Use seperate settings for db and media backup

Compare with previous version

added 1 commit <ul><li>1727a4a3 - Use seperate settings for db and media backup</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4922&start_sha=bacd3df64640ccaa0a0cafaa5f2032d9b3f813fc)
Owner

Potential use before assignment ­set to a dummy value in this class.

Also, what if there is no last backup?

Potential use before assignment ­set to a dummy value in this class. Also, what if there is no last backup?
Owner

s/settings/seconds/?

`s/settings/seconds/`?
Owner

Pull the .last() up here, as it is used in every access later on.

Pull the `.last()` up here, as it is used in every access later on.
Owner

Please use the configuration namespaces properly, as for the keep option.

Please use the configuration namespaces properly, as for the `keep` option.
Author
Owner

changed this line in version 17 of the diff

changed this line in [version 17 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4928&start_sha=1727a4a3f24322645fa4449a881d4bc58bf127db#358275720d816cfb0e6a4145a70f46181f8b430e_47_52)
Author
Owner

changed this line in version 17 of the diff

changed this line in [version 17 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4928&start_sha=1727a4a3f24322645fa4449a881d4bc58bf127db#abc9d828b78bd9d36e05475d0bc91119bc5bc6b0_734_733)
Author
Owner

added 1 commit

  • 5834ee44 - Use seperate settings for db and media backup

Compare with previous version

added 1 commit <ul><li>5834ee44 - Use seperate settings for db and media backup</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4928&start_sha=1727a4a3f24322645fa4449a881d4bc58bf127db)
Author
Owner

resolved all threads

resolved all threads
Author
Owner

changed this line in version 18 of the diff

changed this line in [version 18 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4929&start_sha=5834ee44e5ab278518f9996dfd16499657c3db17#358275720d816cfb0e6a4145a70f46181f8b430e_68_68)
Author
Owner

added 1 commit

  • a2a593e4 - Use seperate settings for db and media backup

Compare with previous version

added 1 commit <ul><li>a2a593e4 - Use seperate settings for db and media backup</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4929&start_sha=5834ee44e5ab278518f9996dfd16499657c3db17)
Owner

Also, what if there is no last backup?

> Also, what if there is no last backup?
Owner
        elif task and task.status != "SUCCESS":
```suggestion:-0+0 elif task and task.status != "SUCCESS": ```
Owner

Please be clearer in the option name (e.g. timeout_seconds)

Please be clearer in the option name (e.g. `timeout_seconds`)
Author
Owner

changed this line in version 19 of the diff

changed this line in [version 19 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4930&start_sha=a2a593e420c9b0940182d7b659778b8428c02617#358275720d816cfb0e6a4145a70f46181f8b430e_36_36)
Author
Owner

changed this line in version 19 of the diff

changed this line in [version 19 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4930&start_sha=a2a593e420c9b0940182d7b659778b8428c02617#abc9d828b78bd9d36e05475d0bc91119bc5bc6b0_734_733)
Author
Owner

added 1 commit

  • 3e2a9284 - Use seperate settings for db and media backup

Compare with previous version

added 1 commit <ul><li>3e2a9284 - Use seperate settings for db and media backup</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4930&start_sha=a2a593e420c9b0940182d7b659778b8428c02617)
Author
Owner

resolved all threads

resolved all threads
Author
Owner

changed this line in version 20 of the diff

changed this line in [version 20 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4931&start_sha=3e2a92845d4f8e099ac3eca9448779c41e136a45#358275720d816cfb0e6a4145a70f46181f8b430e_74_74)
Author
Owner

added 1 commit

  • d86bf0e7 - Use seperate settings for db and media backup

Compare with previous version

added 1 commit <ul><li>d86bf0e7 - Use seperate settings for db and media backup</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4931&start_sha=3e2a92845d4f8e099ac3eca9448779c41e136a45)
Owner
            last_backup = backups[-1]
```suggestion:-0+0 last_backup = backups[-1] ```
Author
Owner

resolved all threads

resolved all threads
Author
Owner

changed this line in version 21 of the diff

changed this line in [version 21 of the diff](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4932&start_sha=d86bf0e7427ebcea953d0aebc52f962f8adfe00f#358275720d816cfb0e6a4145a70f46181f8b430e_38_38)
Author
Owner

added 1 commit

  • ca269aa6 - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

added 1 commit <ul><li>ca269aa6 - Apply 1 suggestion(s) to 1 file(s)</li></ul> [Compare with previous version](/AlekSIS/official/AlekSIS-Core/-/merge_requests/495/diffs?diff_id=4932&start_sha=d86bf0e7427ebcea953d0aebc52f962f8adfe00f)
nik scheduled this pull request to auto merge when all checks succeed 2021-02-21 21:00:53 +01:00
nik merged commit d50b445b86 into master 2021-02-21 21:04:11 +01:00
Owner

mentioned in commit d50b445b86

mentioned in commit d50b445b86faf8a5263cf05102558a7d64d2f011
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
aleksis/AlekSIS-Core!2140
No description provided.