Skip to content

cats: remove dynamic catalog backends#1392

Merged
arogge merged 14 commits intobareos:masterfrom
alaaeddineelamri:dev/alaaeddineelamri/master/remove-cats-backends
Mar 27, 2023
Merged

cats: remove dynamic catalog backends#1392
arogge merged 14 commits intobareos:masterfrom
alaaeddineelamri:dev/alaaeddineelamri/master/remove-cats-backends

Conversation

@alaaeddineelamri
Copy link
Contributor

@alaaeddineelamri alaaeddineelamri commented Feb 23, 2023

Thank you for contributing to the Bareos Project!

Description

This PR removes the catalog backends mechanism as it is no longer necessary since we now only support PostgreSQL.

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)

Make sure you check/merge the PR using devtools/pr-tool to have some simple automated checks run and a proper changelog record added.

General
  • Is the PR title usable as CHANGELOG entry?
  • Purpose of the PR is understood
  • Commit descriptions are understandable and well formatted
  • Check backport line
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

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/remove-cats-backends branch 19 times, most recently from 6de9372 to af9b9f1 Compare March 2, 2023 11:21
@alaaeddineelamri alaaeddineelamri marked this pull request as ready for review March 2, 2023 13:49
@alaaeddineelamri alaaeddineelamri requested a review from arogge March 2, 2023 16:46
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/remove-cats-backends branch from 04edadf to a25db5f Compare March 3, 2023 09:50
Copy link
Member

@arogge arogge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks mostly great. I'm unhappy with the removed test - we should probably have a replacement for that.
Also we might want to discuss if we deprecate "Backend Directories" in 22.1, so people don't trip when upgrading to 23.

add_library(dird_objects STATIC ${DIRD_OBJECTS_SRCS})
target_link_libraries(
dird_objects PRIVATE bareos ${OPENSSL_LIBRARIES} Threads::Threads
dird_objects PRIVATE bareos bareossql ${OPENSSL_LIBRARIES} Threads::Threads
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this should suddenly require bareossql. Did you run into any issues while building?

Comment on lines -276 to -295
void test_CFG_TYPE_STR_VECTOR_OF_DIRS(DirectorResource* me)
{
EXPECT_EQ(me->backend_directories.size(), 9);
/*
* WIN32:
* cmake uses some value for PATH_BAREOS_BACKENDDIR,
* which ends up in the configuration files
* but this is later overwritten in the Director Daemon with ".".
* Therefore we skip this test.
*/
#if !defined(HAVE_WIN32)
EXPECT_EQ(me->backend_directories.at(0), PATH_BAREOS_BACKENDDIR);
#endif
}

TEST(ConfigParser_Dir, CFG_TYPE_STR_VECTOR_OF_DIRS)
{
test_config_directive_type(test_CFG_TYPE_STR_VECTOR_OF_DIRS);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while we obviously cannot test with backend_directories anymore, we should probably still test that type.

The main purpose of :program:`bareos-dbcopy` is to migrate an existing Bareos
installation from |mysql| to |postgresql|. Therefore the required
:config:option:`dir/catalog/DbDriver` in the ``<sourcecatalog>`` is |mysql|, and
``dir/catalog/DbDriver`` in the ``<sourcecatalog>`` is |mysql|, and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we remove dbcopy in an earlier PR? Why do we still keep its manpage?
We should definitely still point people at the tool, that existed in prior versions, but I guess it is sufficient if the documentation is present in that version...

@arogge arogge added the removal this PR removes functionality label Mar 6, 2023
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/remove-cats-backends branch 2 times, most recently from 65d7cff to 89000a8 Compare March 13, 2023 17:21
@alaaeddineelamri alaaeddineelamri requested a review from arogge March 14, 2023 12:30
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/remove-cats-backends branch from 89000a8 to 28a2a11 Compare March 23, 2023 08:53
@arogge
Copy link
Member

arogge commented Mar 27, 2023

I tried installing just bareos-database-postgresql on Debian 11. With your changes I can just install it on its own. The old behaviour was to pull in at least bareos-database-common and bareos-common.

Old behaviour:

The following additional packages will be installed:
  bareos-common bareos-database-common dbconfig-common dbconfig-pgsql gawk libjansson4 liblzo2-2 libmpfr6 libpq5 libsigsegv2 netbase
  postgresql-client postgresql-client-13 postgresql-client-common sensible-utils ucf
Suggested packages:
  postgresql gawk-doc postgresql-13 postgresql-doc-13
The following NEW packages will be installed:
  bareos-common bareos-database-common bareos-database-postgresql dbconfig-common dbconfig-pgsql gawk libjansson4 liblzo2-2 libmpfr6 libpq5
  libsigsegv2 netbase postgresql-client postgresql-client-13 postgresql-client-common sensible-utils ucf

New behaviour:

The following additional packages will be installed:
  dbconfig-common dbconfig-pgsql libpq5 netbase postgresql-client postgresql-client-13 postgresql-client-common sensible-utils ucf
Suggested packages:
  postgresql postgresql-13 postgresql-doc-13
The following NEW packages will be installed:
  bareos-database-postgresql dbconfig-common dbconfig-pgsql libpq5 netbase postgresql-client postgresql-client-13 postgresql-client-common
  sensible-utils ucf

I'm not saying that's a problem. It might just be a change we were not (yet) aware of.

@arogge arogge merged commit b817969 into bareos:master Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

removal this PR removes functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants