Resolve "Add health check for dbbackup" #2140
No reviewers
Labels
No labels
Security
TeX
auto-update
board
done
board
ready
board
todo
check
delete-eslint-rc-js
check
update-builddeps-package-json
check
update-eslint-rc-js
check
update-gitignore
check
update-merge-request-template
check
update-prettier-ignore
check
update-pyproject-toml
check
update-renovate-json
check
update-tox-ini
part
backend
part
ci
part
docs
part
frontend
part
i18n
part
non-technical
part
packaging
prio
1
prio
2
prio
3
release-mr-5.x
size
large
size
medium
size
small
source
customer
source
customer::fsmw
source
customer::fss
source
customer::teckids
source
downstream
type
breaking
type
bug
type
feature
type
refactoring
workflow
blocked
workflow
confirmed
workflow
current-todo
workflow
discussing
workflow
new-app
workflow
wontfix
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
aleksis/AlekSIS-Core!2140
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "378-add-health-check-for-dbbackup"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #378
added 1 commit
Compare with previous version
requested review from @hansegucker
assigned to @nik and unassigned @debdolph
marked this merge request as ready
approved this merge request
unapproved this merge request
As the two newly added system checks share a lot of code, I would suggest to use a common base class.
added 1 commit
Compare with previous version
added 1 commit
Compare with previous version
This should not inherit from
BaseDbBackupCommandas this is not a management command.This should not inherit from
BaseDbBackupCommandas this is not a management command.This import name is to generic.
Please import as
dbbackup_utilsor, even better, just importdbbackupitself and use the rest of it namespaced.Is this
DBBACKUP_SECONDSby intention. i.e. timeout is intended to be the same for dbbackup and media backup?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?
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?
Can/should we derive this fro mthe configured backup plan somehow?
yes
changed this line in version 4 of the diff
changed this line in version 4 of the diff
changed this line in version 4 of the diff
changed this line in version 4 of the diff
changed this line in version 4 of the diff
changed this line in version 4 of the diff
changed this line in version 4 of the diff
added 1 commit
Compare with previous version
resolved all threads
added 31 commits
masterCompare with previous version
added 1 commit
Compare with previous version
added 1 commit
Compare with previous version
added 1 commit
Compare with previous version
added 1 commit
Compare with previous version
added 1 commit
Compare with previous version
I second that.
Why query the database twice?
This will fail with the code above if no task is found (
NameErrorbecause task was never set). Fixing the comment above by just callinglast()unconditionally implicitly solves this.Why?
changed this line in version 11 of the diff
changed this line in version 11 of the diff
added 1 commit
Compare with previous version
changed this line in version 12 of the diff
resolved all threads
added 1 commit
Compare with previous version
added 1 commit
eb80b193- Add health checks for media and db backups. Closes #378Compare with previous version
Please split this up for db backup and media backup.
done
added 1 commit
Compare with previous version
resolved all threads
approved this merge request
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.
Didn't you say this was unnecessary?
Didn't you say this was unnecessary?
Didn't you say this was unnecessary?
This still uses the same parameter from the config file.
changed this line in version 15 of the diff
changed this line in version 15 of the diff
changed this line in version 15 of the diff
changed this line in version 15 of the diff
changed this line in version 15 of the diff
added 1 commit
Compare with previous version
resolved all threads
changed this line in version 16 of the diff
added 1 commit
Compare with previous version
Potential use before assignment set to a dummy value in this class.
Also, what if there is no last backup?
s/settings/seconds/?Pull the
.last()up here, as it is used in every access later on.Please use the configuration namespaces properly, as for the
keepoption.changed this line in version 17 of the diff
changed this line in version 17 of the diff
added 1 commit
Compare with previous version
resolved all threads
changed this line in version 18 of the diff
added 1 commit
Compare with previous version
Please be clearer in the option name (e.g.
timeout_seconds)changed this line in version 19 of the diff
changed this line in version 19 of the diff
added 1 commit
Compare with previous version
resolved all threads
changed this line in version 20 of the diff
added 1 commit
d86bf0e7- Use seperate settings for db and media backupCompare with previous version
resolved all threads
changed this line in version 21 of the diff
added 1 commit
ca269aa6- Apply 1 suggestion(s) to 1 file(s)Compare with previous version
mentioned in commit
d50b445b86