Fix backup/rotation with multiple excluded databases#1610
Merged
Conversation
BuJo
requested review from
a team,
alexjfisher and
bastelfreak
as code owners
|
|
BuJo
force-pushed
the
backup-fix-excluded-dbs
branch
from
5a8b607 to
45a2a45
Compare
Contributor
Author
|
Branch is working in production without problems. |
Contributor
Author
|
Should I rebase this branch on top of the current |
BuJo
force-pushed
the
backup-fix-excluded-dbs
branch
from
019f8f0 to
8f61315
Compare
bastelfreak
reviewed
Jul 24, 2024
Collaborator
|
I was wondering why no tests catched the issue you described. Turns out there's a test, but only for ancient mysql: #1642. |
Contributor
Author
|
Should I try and port the test forward before merging @bastelfreak ? |
* When using multiple excluded databases, the list of databases is filtered using `grep -v`. i.e. `grep -v '^\(information_schema|performance_schema\)$` * When using Basic vs Extended Regular Expressions, the characters `(` and `|` lose their special meaning, the backslashed versions have to be used. For the group (`()`) the escaping has been done, however the alternation is unescaped. Leading to: * All the excluded databases will be backed up. * In case a database is not backuppable (which is why it had been excluded), this leads to the cleanup not being run at all, as it depends on the backup having been successful. This MR aims to fix this issue, by revising the regular expression and specifying that behaviour in the respective class spec.
BuJo
force-pushed
the
backup-fix-excluded-dbs
branch
from
8f61315 to
7d90921
Compare
Contributor
Author
|
@bastelfreak I rebased the branch on top of current |
bastelfreak
approved these changes
Oct 1, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Right now, excluding multiple databases does not work.
This MR aims to fix excluding multiple databases, by revising the respective regular expression.
Additional Context
grep -v. i.e.grep -v '^\(information_schema|performance_schema\)$(and|lose their special meaning, the backslashed versions have to be used. For the group (()) the escaping has been done, however the alternation is unescaped.Leading to:
Checklist