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

Do not fail on missing fail2ban config during the backup #558

Merged
merged 3 commits into from Nov 23, 2018

Conversation

Projects
None yet
4 participants
@maniackcrudelis
Contributor

maniackcrudelis commented Oct 10, 2018

The problem

Especially with the migration to stretch, it happens quite often that a file is missing in an app for php or fail2ban.
But, if a fail2ban config is missing, the app will still be working.
The current behavior can break an upgrade without any restoration of the backup, because a file is missing during the backup.

Solution

Print a warning but do not fail if a file is missing, except if this file is really important for the app.

PR Status

Tested on a VM.
Can be reviewed.

How to test

Replace the helper, and remove a file used during the backup.

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@alexAubin

Uh okay, why not ô.O

Are there any opinion on this ?

@zamentur

Ok to merge that if there is people with app that disappear. But this PR could declare some bad backup has correctly made, it could prevent some admin notices there backup are broken

return 1
else
echo "The missing file will be replaced by a dummy one for the backup !!!" >&2
echo "This file is dummy... Please contact the maintainer of this app..." | tee "${SRC_PATH}" > /dev/null

This comment has been minimized.

@zamentur

zamentur Nov 20, 2018

Contributor

I suggest to remove this line. It could break the system by creating a conf file with a bad content not interpretable by the service that manage this conf file

This comment has been minimized.

@maniackcrudelis

maniackcrudelis Nov 20, 2018

Contributor

Indeed, forgot that...
Still, an admin should be informed that there were an error with the backup.

This comment has been minimized.

@Josue-T

Josue-T Nov 20, 2018

Contributor

Mybe we just can inform the admin that the file is missing and explain how to create a dummy file.

This comment has been minimized.

@maniackcrudelis

maniackcrudelis Nov 20, 2018

Contributor

Create a dummy file is what is going to save the process. So we can't avoid that otherwise this PR has no meaning.
But the question was more, how to inform an admin ?

@maniackcrudelis

This comment has been minimized.

Contributor

maniackcrudelis commented Nov 20, 2018

Well, I change this PR to not fail only on fail2ban config files, and replace it by blank files, which is not going to cause any issues with fail2ban.
I tried that fix with wordpress. It works as expected.

@maniackcrudelis maniackcrudelis changed the title from Do not fail on missing file during the backup to Do not fail on missing fail2ban config during the backup Nov 20, 2018

@alexAubin

This comment has been minimized.

Member

alexAubin commented Nov 23, 2018

So shall we merge this for 3.3 stable ?

@maniackcrudelis

This comment has been minimized.

Contributor

maniackcrudelis commented Nov 23, 2018

This should be merged as soon as possible. To prevent any further failures for our users.

@alexAubin alexAubin merged commit 6bfcedf into stretch-unstable Nov 23, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@alexAubin alexAubin deleted the donotfailbackup branch Nov 23, 2018

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