core/scripts: sanitize and clean up scripts, improve bareos-config-lib.sh, remove make_catalog_backup.pl, use make_catalog_backup by bruno-at-bareos · Pull Request #1081 · bareos/bareos

bruno-at-bareos · 2022-02-23T09:41:18Z

This PR is about cleaning the scripts used in core/scripts and core/src/cats and replacing the old perl make_catalog_backup.pl by the posix sh script make_catalog_backup.

Breaking changes are the following

make_catalog_backup script signature changed, and it uses the get_database_ functions from bareos-config-lib.sh
make_catalog_backup.pl string was used in the default configuration example in job/BackupCatalog.conf (if the package manager doesn't handle or the user changed the default, the line has to be manually changed according to the new documentation).
database managing scripts got the option --host= removed for remote database. We use instead the standardized PostgreSQL export PGVAR. Documentation has been updated in that sense.

A new systemtests has been created and enabled to run the default BackupCatalog job.
The unused systemtests/tests/*/etc/bareos/bareos-dir.d/job/BackupCatalog.conf.in file has been removed except for bareos test.

The perl dependency has been removed on all platform builds.
For convenience a compatible wrapper script make_backup_catalog.pl is delivered until Bareos version 23. This script emit a warning about the deprecated status in job log when used.

Please check

Short description and the purpose of this PR is present above this paragraph
Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

PR name is meaningful
Purpose of the PR is understood
Separate commit for this PR in the CHANGELOG.md, PR number referenced is same
Commit descriptions are understandable and well formatted
[ ] If backport: add original PR number and target branch at top of this file: Backport of PR#000 to bareos-2x

Source code quality

Source code changes are understandable
Variable and function names are meaningful
Code comments are correct (logically and spelling)
Required documentation changes are present and part of the PR
bareos-check-sources --since-merge does not report any problems
git status should not report modifications in the source tree after building and testing

Tests

Decision taken that a system- or unittest is required (if not, then remove this paragraph)
The decision towards a systemtest is reasonable compared to a unittest
Testname matches exactly what is being tested
Output of the test leads quickly to the origin of the fault

pstorz

Just some comments.

core/src/cats/make_catalog_backup.in

systemtests/tests/bareos/testrunner-simple-make-catalog-backup

joergsteffens

This change can/will break existing installations, as the existing BackupCatalog.conf might not get replaced during an update (at least on Debian). Therefore it would prefer if make_catalog_backup.pl got replaced by a symlink to make_catalog_backup or a wrapper script to it additionally issuing a warning.

systemtests/tests/acl/etc/bareos/bareos-dir.d/job/BackupCatalog.conf.in

bruno-at-bareos · 2022-03-03T15:25:14Z

This change can/will break existing installations, as the existing BackupCatalog.conf might not get replaced during an update (at least on Debian). Therefore it would prefer if make_catalog_backup.pl got replaced by a symlink to make_catalog_backup or a wrapper script to it additionally issuing a warning.

If the file is not replaced by the packaging that mean the user has changed it, so it out of our responsibility and we can't do much about that. Maybe you don't see, this PR in changelog will be in breaking changes for 22 (and can be also mentioned in the release note). I don't believe trying to hide the change by another layer is the right thing to do.

joergsteffens · 2022-03-03T15:38:57Z

This change can/will break existing installations, as the existing BackupCatalog.conf might not get replaced during an update (at least on Debian). Therefore it would prefer if make_catalog_backup.pl got replaced by a symlink to make_catalog_backup or a wrapper script to it additionally issuing a warning.

If the file is not replaced by the packaging that mean the user has changed it, so it out of our responsibility and we can't do much about that. Maybe you don't see, this PR in changelog will be in breaking changes for 22 (and can be also mentioned in the release note). I don't believe trying to hide the change by another layer is the right thing to do.

As I tried to say, on Debian the file will not be replaced, even if it is unchanged by the user. I don't think it is good practice that all Debian installation will fail to start after an update. Creating a wrapper script with a warning might be a good compromise.

bruno-at-bareos · 2022-03-07T09:07:00Z

This change can/will break existing installations, as the existing BackupCatalog.conf might not get replaced during an update (at least on Debian). Therefore it would prefer if make_catalog_backup.pl got replaced by a symlink to make_catalog_backup or a wrapper script to it additionally issuing a warning.

If the file is not replaced by the packaging that mean the user has changed it, so it out of our responsibility and we can't do much about that. Maybe you don't see, this PR in changelog will be in breaking changes for 22 (and can be also mentioned in the release note). I don't believe trying to hide the change by another layer is the right thing to do.

As I tried to say, on Debian the file will not be replaced, even if it is unchanged by the user. I don't think it is good practice that all Debian installation will fail to start after an update. Creating a wrapper script with a warning might be a good compromise.

Debian packaging policy state it will handle it reference here
https://www.debian.org/doc/debian-policy/ap-pkg-conffiles.html#automatic-handling-of-configuration-files-by-dpkg

If Bareos Debian packages are not doing it according then they have to be fixed.

bruno-at-bareos · 2022-03-22T09:30:55Z

Everything works correctly last build failed F35 when it was not before, a rerun will certainly fix it.

joergsteffens

The file docs/manuals/source/TasksAndConcepts/CatalogMaintenance.rst contains references to the old perl script:

    :file:`make_catalog_backup.pl`    backup         backup the Bareos database, default on Linux
    :file:`make_catalog_backup` backup backup the Bareos database for systems without Perl

Best remove them.

core/src/cats/make_catalog_backup.in

core/scripts/bareos-config-lib.sh.in

docs/manuals/source/TasksAndConcepts/CatalogMaintenance.rst

docs/manuals/source/IntroductionAndTutorial/InstallingBareosWebui.rst

bruno-at-bareos · 2022-03-24T13:27:20Z

@joergsteffens notice

What is the status about the fallback for make_catalog_backup.pl?
This will be evaluated once we can test the packages.

core/scripts/bareos-config-lib.sh.in

core/src/cats/make_catalog_backup.pl.in

bruno-at-bareos · 2022-04-06T14:05:26Z

The script has its signature changed as it doesn't support anymore additional parameters like the initial .pl was

# TODO: use just ENV and drop the pg_service.conf file
sub dump_pgsql
{
    my %args = @_;
    umask(0077);

    if ($args{db_address}) {
        $ENV{PGHOST}=$args{db_address};
    }
    if ($args{db_socket}) {
        $ENV{PGHOST}=$args{db_socket};
    }
    if ($args{db_port}) {
        $ENV{PGPORT}=$args{db_port};
    }
    if ($args{db_user}) {
        $ENV{PGUSER}=$args{db_user};
    }
    if ($args{db_password}) {
        $ENV{PGPASSWORD}=$args{db_password};
    }
    $ENV{PGDATABASE}=$args{db_name};
    exec("HOME='$wd' pg_dump -c > '$wd/$args{db_name}.sql'");
    print "Error while executing postgres dump $!\n";
    return 1;               # in case of error
}

joergsteffens · 2022-04-06T14:29:24Z

It is a long time ago that I had to program in Perl, but I think, in this context @_ are the function parameters, but the script parameters.

joergsteffens · 2022-04-12T15:17:49Z

core/src/tests/configs/bareos-configparser-tests/bareos-dir.d/job/BackupCatalog.conf

  # This sends the bootstrap via mail for disaster recovery.
  # Should be sent to another system, please change recipient accordingly
-  Write Bootstrap = "|/tmp/bin/bsmtp -h smtp_host -f \"\(Bareos\) \" -s \"Bootstrap for Job %j\" root@localhost" # (#01)
+  Write Bootstrap = "|@bindir@/bsmtp -h @smtp_host@ -f \"\(Bareos\) \" -s \"Bootstrap for Job %j\" @job_email@"


I probably doesn't matter, but this is not an *.in file, so no replacement of @ variables.

joergsteffens

I'm not finished with the review, however here are my first remarks.

core/scripts/bareos-config-lib.sh.in

core/src/cats/make_catalog_backup.in

core/src/cats/make_catalog_backup.pl.in

core/src/cats/update_bareos_tables.in

debian/bareos-database-common.config.in

docs/manuals/source/TasksAndConcepts/CatalogMaintenance.rst

bruno-at-bareos · 2022-05-17T07:37:13Z

@joergsteffens the final review should be ok now with commit squashed, and last changelog.

- core/scripts: sanitize and cleanup code, improve bareos-config-lib.sh. - systemtests/tests: remove unused job/BackupCatalog.conf.in files. - packaging: debian review and improve database-common dbconfig scripts. - breaking changes: make_bareos_catalog script signature has changed + usage is now make_catalog_backup <catalog-name> + if more than one args is passed a warning is emit. + removed multi database driver to use only PostreSQL. + removed bareos-dbcheck dependency. + use of bareos-config-lib get_database_ functions to setup the command line with PG env vars. script can then be run without a .pgpass. - create systemtests/tests/bareos/testrunner-test-make-catalog-backup to test the default BackupCatalog job. + backup the catalog. + store versionid. + restore the backup. + drop all tables. + restore the catalog with the restored dump script. + check number of tables and versionid. + restart Bareos ... and compare output. by this, we make sure, that Bareos is able to start again and also leave this test in the same state as the other tests (running Bareos daemons). - bareos-config-lib.sh remove obsolete or non used functions. + get_database_driver + initialize_database_driver + get_database_utility_path + get_databases_installed + fix unbounded vars in bareos-config-lib.sh. - remove support of multiple database items. - add new function handle_database_scripts_command_line_parameter - add Bareos License to bareos-config. - remove usage of external parameters (like psql calls ($*)). - add check to failed if $1 is not postgresql, warn user about deprecated usage. - suppress dbcheck dependency. - get_database_param is using new bareos-dir -xcRessourceName functionality - use shellcheck -s sh to sanitize code. - make delete_catalog_backup work the same as make_catalog_backup (using CatalogName) - make apply_dbconfig supporting remote dbaddress - update sed expression to only use * for POSIX compliance - databases scripts (create,drop,make,delete,grant,update): + standardized messages. + fix handle_database_scripts_command_line_parameter call. + enforce -u and -e. - make_catalog_backup.pl is now a wrapper sh script which warn about its deprecated usage (see breaking above) - docs: CatalogMaintenance.rst chapter renew: + use actual BackupCatalog.conf in example. + replace mysql related content with PostgreSQL equivalent. + remove make_catalog_backup.pl reference. + refresh remote database section with new instructions. Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com> Co-authored-by: Jörg Steffens <joergsteffens@users.noreply.github.com>

joergsteffens

Good work!

bruno-at-bareos added the draft label Feb 23, 2022

bruno-at-bareos self-assigned this Feb 23, 2022

arogge assigned pstorz Feb 24, 2022

pstorz reviewed Feb 24, 2022

View reviewed changes

core/src/cats/make_catalog_backup.in Show resolved Hide resolved

systemtests/tests/bareos/testrunner-simple-make-catalog-backup Outdated Show resolved Hide resolved

joergsteffens reviewed Mar 3, 2022

View reviewed changes

systemtests/tests/acl/etc/bareos/bareos-dir.d/job/BackupCatalog.conf.in Show resolved Hide resolved

bruno-at-bareos force-pushed the dev/bruno/based_on_1073/replace_make_catalog_backup.pl branch 4 times, most recently from 6af5df1 to e27b669 Compare March 15, 2022 14:00

bruno-at-bareos removed the draft label Mar 15, 2022

bruno-at-bareos force-pushed the dev/bruno/based_on_1073/replace_make_catalog_backup.pl branch 2 times, most recently from d2052c1 to 9867483 Compare March 15, 2022 14:22

bruno-at-bareos changed the title ~~remove make_catalog_backup.pl and use make_catalog_backup posix script~~ core/scripts: sanitize and clean up scripts, improve bareos-config-lib.sh, remove make_catalog_backup.pl, use make_catalog_backup Mar 15, 2022

bruno-at-bareos marked this pull request as ready for review March 15, 2022 14:33

bruno-at-bareos force-pushed the dev/bruno/based_on_1073/replace_make_catalog_backup.pl branch 2 times, most recently from 6a534f6 to aaf5bc3 Compare March 21, 2022 12:39

bruno-at-bareos requested review from joergsteffens and pstorz March 22, 2022 09:31

joergsteffens requested changes Mar 23, 2022

View reviewed changes

joergsteffens reviewed Mar 28, 2022

View reviewed changes

core/scripts/bareos-config-lib.sh.in Outdated Show resolved Hide resolved

core/scripts/bareos-config-lib.sh.in Outdated Show resolved Hide resolved

pstorz assigned joergsteffens and unassigned pstorz Mar 31, 2022

bruno-at-bareos force-pushed the dev/bruno/based_on_1073/replace_make_catalog_backup.pl branch from 07eca25 to b052150 Compare March 31, 2022 12:12

bruno-at-bareos requested a review from joergsteffens April 4, 2022 07:57

bruno-at-bareos force-pushed the dev/bruno/based_on_1073/replace_make_catalog_backup.pl branch from 1a0f47d to 7a5782d Compare April 4, 2022 11:23

joergsteffens reviewed Apr 6, 2022

View reviewed changes

core/src/cats/make_catalog_backup.pl.in Show resolved Hide resolved

bruno-at-bareos requested a review from joergsteffens April 6, 2022 14:06

bruno-at-bareos force-pushed the dev/bruno/based_on_1073/replace_make_catalog_backup.pl branch from cdba137 to 1943a6a Compare April 13, 2022 07:33

joergsteffens reviewed Apr 13, 2022

View reviewed changes

bruno-at-bareos force-pushed the dev/bruno/based_on_1073/replace_make_catalog_backup.pl branch 4 times, most recently from f609ae8 to 2b0caa8 Compare May 2, 2022 12:58

bruno-at-bareos force-pushed the dev/bruno/based_on_1073/replace_make_catalog_backup.pl branch from ec277a4 to f3c69eb Compare May 3, 2022 14:00

pstorz requested a review from joergsteffens May 5, 2022 09:33

joergsteffens reviewed May 9, 2022

View reviewed changes

core/scripts/bareos-config-lib.sh.in Outdated Show resolved Hide resolved

core/scripts/bareos-config-lib.sh.in Outdated Show resolved Hide resolved

core/scripts/bareos-config-lib.sh.in Outdated Show resolved Hide resolved

joergsteffens reviewed May 9, 2022

View reviewed changes

bruno-at-bareos force-pushed the dev/bruno/based_on_1073/replace_make_catalog_backup.pl branch 2 times, most recently from bad4882 to f3862d5 Compare May 16, 2022 14:50

bruno-at-bareos requested a review from joergsteffens May 17, 2022 07:37

bruno-at-bareos force-pushed the dev/bruno/based_on_1073/replace_make_catalog_backup.pl branch 2 times, most recently from 9fb9eaf to 35cb123 Compare May 18, 2022 08:49

bruno-at-bareos and others added 3 commits June 8, 2022 19:39

Update CHANGELOG.md

af637b6

core/scripts: cleanup

a5fa265

joergsteffens force-pushed the dev/bruno/based_on_1073/replace_make_catalog_backup.pl branch from 0fc6c65 to a5fa265 Compare June 8, 2022 17:43

joergsteffens approved these changes Jun 9, 2022

View reviewed changes

joergsteffens merged commit 4a0a945 into bareos:master Jun 10, 2022

bruno-at-bareos deleted the dev/bruno/based_on_1073/replace_make_catalog_backup.pl branch July 4, 2022 11:49

Conversation

bruno-at-bareos commented Feb 23, 2022 • edited by joergsteffens Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please check

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

Source code quality

Tests

Uh oh!

pstorz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joergsteffens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bruno-at-bareos commented Mar 3, 2022

Uh oh!

joergsteffens commented Mar 3, 2022

Uh oh!

bruno-at-bareos commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bruno-at-bareos commented Mar 22, 2022

Uh oh!

joergsteffens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bruno-at-bareos commented Mar 24, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bruno-at-bareos commented Apr 6, 2022

Uh oh!

joergsteffens commented Apr 6, 2022

Uh oh!

joergsteffens Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

joergsteffens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bruno-at-bareos commented May 17, 2022

Uh oh!

joergsteffens left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

bruno-at-bareos commented Feb 23, 2022 •

edited by joergsteffens

Loading

bruno-at-bareos commented Mar 7, 2022 •

edited

Loading