From 89d485723d741c32735d88028913c467d674bf15 Mon Sep 17 00:00:00 2001
From: David Douard <david.douard@sdfa3.org>
Date: Thu, 6 Jul 2023 18:22:57 +0200
Subject: [PATCH] Make `swh db init` command work properly with the --dbname
option
This allows to run the command `swh db init --dname DBURI` without
giving a configuration file while still populating the dbversion table
in the database.
Note that this may not work for all swh packages, when the postgresql
backend for the given swh package needs more configuration entries than
just the psql connection string.
This more or less drops support for "old style" db sql init scripts:
- the `swh db init` will still work for an 'old style' package (in which
the sql init scripts do create dbversion table and initialize it) but
will display an error message,
- the --initial-version option has been dropped, since this made sense
only for old style packages,
- the test data 'cli_new' has been renamed as 'cli' (in place of the
previous "old style" one)
---
swh/core/cli/db.py | 106 +++++++++---------
swh/core/db/tests/conftest.py | 4 +-
swh/core/db/tests/data/cli/sql/30-schema.sql | 7 --
swh/core/db/tests/data/cli/sql/50-data.sql | 3 -
.../{cli_new => cli}/sql/upgrades/001.sql | 0
.../{cli_new => cli}/sql/upgrades/002.sql | 0
.../{cli_new => cli}/sql/upgrades/003.sql | 0
.../{cli_new => cli}/sql/upgrades/004.sql | 0
.../{cli_new => cli}/sql/upgrades/005-bis.sql | 0
.../{cli_new => cli}/sql/upgrades/005.sql | 0
.../{cli_new => cli}/sql/upgrades/006.sql | 0
.../data/cli_new/sql/0-superuser-init.sql | 1 -
.../db/tests/data/cli_new/sql/15-flavor.sql | 22 ----
.../db/tests/data/cli_new/sql/30-schema.sql | 6 -
.../db/tests/data/cli_new/sql/40-funcs.sql | 6 -
.../db/tests/data/cli_new/sql/50-data.sql | 2 -
swh/core/db/tests/test_cli.py | 49 +++++++-
swh/core/db/tests/test_db_utils.py | 54 ++++-----
18 files changed, 123 insertions(+), 137 deletions(-)
rename swh/core/db/tests/data/{cli_new => cli}/sql/upgrades/001.sql (100%)
rename swh/core/db/tests/data/{cli_new => cli}/sql/upgrades/002.sql (100%)
rename swh/core/db/tests/data/{cli_new => cli}/sql/upgrades/003.sql (100%)
rename swh/core/db/tests/data/{cli_new => cli}/sql/upgrades/004.sql (100%)
rename swh/core/db/tests/data/{cli_new => cli}/sql/upgrades/005-bis.sql (100%)
rename swh/core/db/tests/data/{cli_new => cli}/sql/upgrades/005.sql (100%)
rename swh/core/db/tests/data/{cli_new => cli}/sql/upgrades/006.sql (100%)
delete mode 100644 swh/core/db/tests/data/cli_new/sql/0-superuser-init.sql
delete mode 100644 swh/core/db/tests/data/cli_new/sql/15-flavor.sql
delete mode 100644 swh/core/db/tests/data/cli_new/sql/30-schema.sql
delete mode 100644 swh/core/db/tests/data/cli_new/sql/40-funcs.sql
delete mode 100644 swh/core/db/tests/data/cli_new/sql/50-data.sql
diff --git a/swh/core/cli/db.py b/swh/core/cli/db.py
index 294476d4..a1e8d055 100755
--- a/swh/core/cli/db.py
+++ b/swh/core/cli/db.py
@@ -140,16 +140,13 @@ def db_init_admin(module: str, dbname: str) -> None:
help="Database flavor.",
default=None,
)
-@click.option(
- "--initial-version", help="Database initial version.", default=1, show_default=True
-)
@click.pass_context
-def db_init(ctx, module, dbname, flavor, initial_version):
+def db_init(ctx, module, dbname, flavor):
"""Initialize a database for the Software Heritage <module>.
- The database connection string comes from the configuration file (see
- option ``--config-file`` in ``swh db --help``) in the section named after
- the MODULE argument.
+ The database connection string can come from the --dbname option, or from
+ the configuration file (see option ``--config-file`` in ``swh db --help``)
+ in the section named after the MODULE argument.
Example::
@@ -164,9 +161,8 @@ def db_init(ctx, module, dbname, flavor, initial_version):
\b
$ swh db -C conf.yml init storage # or
$ SWH_CONFIG_FILENAME=conf.yml swh db init storage
-
- Note that the connection string can also be passed directly using the
- '--db-name' option, but this usage is about to be deprecated.
+ $ # or
+ $ swh db init --dbname postgresql://user:passwd@pghost:5433/swh-storage storage
"""
from swh.core.db.db_utils import (
@@ -194,54 +190,56 @@ def db_init(ctx, module, dbname, flavor, initial_version):
initialized, dbversion, dbflavor = populate_database_for_package(
module, dbname, flavor
)
- if dbversion is None:
- if cfg is not None:
- # db version has not been populated by sql init scripts (new style),
- # let's do it; instantiate the data source to retrieve the current
- # (expected) db version
- datastore_factory = getattr(import_swhmodule(module), "get_datastore", None)
- if datastore_factory:
- datastore = datastore_factory(**cfg)
- if not hasattr(datastore, "current_version"):
- logger.warning(
- "Datastore %s does not declare the "
- "'current_version' attribute",
- datastore,
- )
- else:
- code_version = datastore.current_version
- logger.info(
- "Initializing database version to %s from the %s datastore",
- code_version,
- module,
- )
- swh_set_db_version(dbname, code_version, desc="DB initialization")
+ if dbversion is not None:
+ click.secho(
+ "ERROR: the database version has been populated by sql init scripts. "
+ "This is now deprecated and should not happen any more"
+ )
+ else:
+ # db version has not been populated by sql init scripts (new style),
+ # let's do it; instantiate the data source to retrieve the current
+ # (expected) db version
+ if cfg is None:
+ cfg = {"cls": "postgresql", "db": dbname}
+ datastore_factory = getattr(import_swhmodule(module), "get_datastore", None)
+ if datastore_factory:
+ datastore = datastore_factory(**cfg)
+ if not hasattr(datastore, "current_version"):
+ logger.warning(
+ "Datastore %s does not declare the " "'current_version' attribute",
+ datastore,
+ )
+ else:
+ code_version = datastore.current_version
+ logger.info(
+ "Initializing database version to %s from the %s datastore",
+ code_version,
+ module,
+ )
+ swh_set_db_version(dbname, code_version, desc="DB initialization")
dbversion = get_database_info(dbname)[1]
if dbversion is None:
- logger.info(
- "Initializing database version to %s "
- "from the command line option --initial-version",
- initial_version,
+ click.secho(
+ "ERROR: database for {} {}{} BUT db version could not be set".format(
+ module,
+ "initialized" if initialized else "exists",
+ f" (flavor {dbflavor})" if dbflavor is not None else "",
+ ),
+ fg="red",
+ bold=True,
+ )
+ else:
+ click.secho(
+ "DONE database for {} {}{} at version {}".format(
+ module,
+ "initialized" if initialized else "exists",
+ f" (flavor {dbflavor})" if dbflavor is not None else "",
+ dbversion,
+ ),
+ fg="green",
+ bold=True,
)
- swh_set_db_version(dbname, initial_version, desc="DB initialization")
-
- dbversion = get_database_info(dbname)[1]
- assert dbversion is not None
-
- # TODO: Ideally migrate the version from db_version to the latest
- # db version
-
- click.secho(
- "DONE database for {} {}{} at version {}".format(
- module,
- "initialized" if initialized else "exists",
- f" (flavor {dbflavor})" if dbflavor is not None else "",
- dbversion,
- ),
- fg="green",
- bold=True,
- )
if flavor is not None and dbflavor != flavor:
click.secho(
diff --git a/swh/core/db/tests/conftest.py b/swh/core/db/tests/conftest.py
index 10dcecde..08e709a8 100644
--- a/swh/core/db/tests/conftest.py
+++ b/swh/core/db/tests/conftest.py
@@ -48,7 +48,7 @@ def mock_import_swhmodule(mocker, datadir):
set to `<mod>` and __file__ pointing to `data/<mod>/__init__.py`.
The Mock object also defines a `get_datastore()` attribute on which the
- `current_version` attribute is set to 42.
+ `current_version` attribute is set to 3.
Typical usage::
@@ -67,7 +67,7 @@ def mock_import_swhmodule(mocker, datadir):
dirname = modname.split(".", 1)[1]
def get_datastore(*args, **kw):
- return mock(current_version=42)
+ return mock(current_version=3)
return mock(
__name__=modname,
diff --git a/swh/core/db/tests/data/cli/sql/30-schema.sql b/swh/core/db/tests/data/cli/sql/30-schema.sql
index fcbd581c..64289f78 100644
--- a/swh/core/db/tests/data/cli/sql/30-schema.sql
+++ b/swh/core/db/tests/data/cli/sql/30-schema.sql
@@ -1,10 +1,3 @@
--- schema version table which won't get truncated
-create table dbversion (
- version int primary key,
- release timestamptz,
- description text
-);
-
-- origin table
create table if not exists origin (
id bigserial not null,
diff --git a/swh/core/db/tests/data/cli/sql/50-data.sql b/swh/core/db/tests/data/cli/sql/50-data.sql
index f6564e07..a0120a67 100644
--- a/swh/core/db/tests/data/cli/sql/50-data.sql
+++ b/swh/core/db/tests/data/cli/sql/50-data.sql
@@ -1,5 +1,2 @@
-insert into dbversion(version, release, description)
-values (10, '2016-02-22 15:56:28.358587+00', 'Work In Progress');
-
insert into origin(url, hash)
values ('https://forge.softwareheritage.org', hash_sha1('https://forge.softwareheritage.org'));
diff --git a/swh/core/db/tests/data/cli_new/sql/upgrades/001.sql b/swh/core/db/tests/data/cli/sql/upgrades/001.sql
similarity index 100%
rename from swh/core/db/tests/data/cli_new/sql/upgrades/001.sql
rename to swh/core/db/tests/data/cli/sql/upgrades/001.sql
diff --git a/swh/core/db/tests/data/cli_new/sql/upgrades/002.sql b/swh/core/db/tests/data/cli/sql/upgrades/002.sql
similarity index 100%
rename from swh/core/db/tests/data/cli_new/sql/upgrades/002.sql
rename to swh/core/db/tests/data/cli/sql/upgrades/002.sql
diff --git a/swh/core/db/tests/data/cli_new/sql/upgrades/003.sql b/swh/core/db/tests/data/cli/sql/upgrades/003.sql
similarity index 100%
rename from swh/core/db/tests/data/cli_new/sql/upgrades/003.sql
rename to swh/core/db/tests/data/cli/sql/upgrades/003.sql
diff --git a/swh/core/db/tests/data/cli_new/sql/upgrades/004.sql b/swh/core/db/tests/data/cli/sql/upgrades/004.sql
similarity index 100%
rename from swh/core/db/tests/data/cli_new/sql/upgrades/004.sql
rename to swh/core/db/tests/data/cli/sql/upgrades/004.sql
diff --git a/swh/core/db/tests/data/cli_new/sql/upgrades/005-bis.sql b/swh/core/db/tests/data/cli/sql/upgrades/005-bis.sql
similarity index 100%
rename from swh/core/db/tests/data/cli_new/sql/upgrades/005-bis.sql
rename to swh/core/db/tests/data/cli/sql/upgrades/005-bis.sql
diff --git a/swh/core/db/tests/data/cli_new/sql/upgrades/005.sql b/swh/core/db/tests/data/cli/sql/upgrades/005.sql
similarity index 100%
rename from swh/core/db/tests/data/cli_new/sql/upgrades/005.sql
rename to swh/core/db/tests/data/cli/sql/upgrades/005.sql
diff --git a/swh/core/db/tests/data/cli_new/sql/upgrades/006.sql b/swh/core/db/tests/data/cli/sql/upgrades/006.sql
similarity index 100%
rename from swh/core/db/tests/data/cli_new/sql/upgrades/006.sql
rename to swh/core/db/tests/data/cli/sql/upgrades/006.sql
diff --git a/swh/core/db/tests/data/cli_new/sql/0-superuser-init.sql b/swh/core/db/tests/data/cli_new/sql/0-superuser-init.sql
deleted file mode 100644
index 480018c8..00000000
--- a/swh/core/db/tests/data/cli_new/sql/0-superuser-init.sql
+++ /dev/null
@@ -1 +0,0 @@
-create extension if not exists pgcrypto;
diff --git a/swh/core/db/tests/data/cli_new/sql/15-flavor.sql b/swh/core/db/tests/data/cli_new/sql/15-flavor.sql
deleted file mode 100644
index 8cd1cdc3..00000000
--- a/swh/core/db/tests/data/cli_new/sql/15-flavor.sql
+++ /dev/null
@@ -1,22 +0,0 @@
--- database flavor
-create type database_flavor as enum (
- 'default',
- 'flavorA',
- 'flavorB'
-);
-comment on type database_flavor is 'Flavor of the current database';
-
-create table dbflavor (
- flavor database_flavor,
- single_row char(1) primary key default 'x',
- check (single_row = 'x')
-);
-comment on table dbflavor is 'Database flavor storage';
-comment on column dbflavor.flavor is 'Database flavor currently deployed';
-comment on column dbflavor.single_row is 'Bogus column to force the table to have a single row';
-
-create or replace function swh_get_dbflavor() returns database_flavor language sql stable as $$
- select coalesce((select flavor from dbflavor), 'default');
-$$;
-
-comment on function swh_get_dbflavor is 'Get the flavor of the database currently deployed';
diff --git a/swh/core/db/tests/data/cli_new/sql/30-schema.sql b/swh/core/db/tests/data/cli_new/sql/30-schema.sql
deleted file mode 100644
index 64289f78..00000000
--- a/swh/core/db/tests/data/cli_new/sql/30-schema.sql
+++ /dev/null
@@ -1,6 +0,0 @@
--- origin table
-create table if not exists origin (
- id bigserial not null,
- url text not null,
- hash text not null
-);
diff --git a/swh/core/db/tests/data/cli_new/sql/40-funcs.sql b/swh/core/db/tests/data/cli_new/sql/40-funcs.sql
deleted file mode 100644
index d4dd410b..00000000
--- a/swh/core/db/tests/data/cli_new/sql/40-funcs.sql
+++ /dev/null
@@ -1,6 +0,0 @@
-create or replace function hash_sha1(text)
- returns text
- language sql strict immutable
-as $$
- select encode(public.digest($1, 'sha1'), 'hex')
-$$;
diff --git a/swh/core/db/tests/data/cli_new/sql/50-data.sql b/swh/core/db/tests/data/cli_new/sql/50-data.sql
deleted file mode 100644
index a0120a67..00000000
--- a/swh/core/db/tests/data/cli_new/sql/50-data.sql
+++ /dev/null
@@ -1,2 +0,0 @@
-insert into origin(url, hash)
-values ('https://forge.softwareheritage.org', hash_sha1('https://forge.softwareheritage.org'));
diff --git a/swh/core/db/tests/test_cli.py b/swh/core/db/tests/test_cli.py
index 57d2b01c..8590427f 100644
--- a/swh/core/db/tests/test_cli.py
+++ b/swh/core/db/tests/test_cli.py
@@ -214,7 +214,7 @@ def test_cli_swh_db_create_and_init_db_new_api(
cli_runner, postgresql, mock_import_swhmodule, mocker, tmp_path
):
"""Create a db then initializing it should be ok for a "new style" datastore"""
- module_name = "test.cli_new"
+ module_name = "test.cli"
conninfo = craft_conninfo(postgresql)
@@ -239,7 +239,7 @@ def test_cli_swh_db_create_and_init_db_new_api(
def test_cli_swh_db_upgrade_new_api(cli_runner, postgresql, datadir, mocker, tmp_path):
"""Upgrade scenario for a "new style" datastore"""
- module_name = "test.cli_new"
+ module_name = "test.cli"
# the `current_version` variable is the version that will be returned by
# any call to `get_current_version()` in this test session, thanks to the
@@ -330,12 +330,55 @@ def test_cli_swh_db_upgrade_new_api(cli_runner, postgresql, datadir, mocker, tmp
assert result.exit_code == 0
assert "Warning: the database does not have a dbmodule table." in result.output
assert (
- "Write the module information (test.cli_new) in the database? [Y/n]"
+ "Write the module information (test.cli) in the database? [Y/n]"
in result.output
)
assert swh_db_module(conninfo) == module_name
+def test_cli_swh_db_init_version_ok(cli_runner, postgresql, datadir, mocker, tmp_path):
+ """Upgrade scenario for a "new style" datastore"""
+ module_name = "test.cli"
+
+ # the `current_version` variable is the version that will be returned by
+ # any call to `get_current_version()` in this test session, thanks to the
+ # local mocked version of import_swhmodule() below.
+ current_version = 5
+
+ # custom version of the mockup to make it easy to change the
+ # current_version returned by get_current_version()
+ # TODO: find a better solution for this...
+ def import_swhmodule_mock(modname):
+ if modname.startswith("test."):
+ dirname = modname.split(".", 1)[1]
+
+ def get_datastore(cls, **kw):
+ return mocker.MagicMock(current_version=current_version)
+
+ return mocker.MagicMock(
+ __name__=modname,
+ __file__=os.path.join(datadir, dirname, "__init__.py"),
+ name=modname,
+ get_datastore=get_datastore,
+ )
+
+ return import_swhmodule(modname)
+
+ mocker.patch("swh.core.db.db_utils.import_swhmodule", import_swhmodule_mock)
+ conninfo = craft_conninfo(postgresql)
+
+ # call the db init stuff WITHOUT a config file
+ result = cli_runner.invoke(swhdb, ["init-admin", module_name, "--dbname", conninfo])
+ assert result.exit_code == 0, f"Unexpected output: {result.output}"
+ result = cli_runner.invoke(swhdb, ["init", module_name, "--dbname", conninfo])
+
+ assert (
+ result.exit_code == 0
+ ), f"Unexpected output: {traceback.print_tb(result.exc_info[2])}"
+
+ assert swh_db_version(conninfo) == current_version
+
+
def test_cli_swh_db_version(swh_db_cli, mock_import_swhmodule, postgresql):
module_name = "test.cli"
cli_runner, db_params = swh_db_cli
diff --git a/swh/core/db/tests/test_db_utils.py b/swh/core/db/tests/test_db_utils.py
index 38080987..6283f783 100644
--- a/swh/core/db/tests/test_db_utils.py
+++ b/swh/core/db/tests/test_db_utils.py
@@ -3,7 +3,7 @@
# License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information
-from datetime import datetime, timedelta
+from datetime import timedelta
from os import path
import pytest
@@ -24,7 +24,7 @@ from swh.core.db.db_utils import (
from .test_cli import craft_conninfo
-@pytest.mark.parametrize("module", ["test.cli", "test.cli_new"])
+@pytest.mark.parametrize("module", ["test.cli"])
def test_get_sql_for_package(mock_import_swhmodule, module):
files = get_sql_for_package(module)
assert files
@@ -37,22 +37,19 @@ def test_get_sql_for_package(mock_import_swhmodule, module):
]
-@pytest.mark.parametrize("module", ["test.cli", "test.cli_new"])
+@pytest.mark.parametrize("module", ["test.cli"])
def test_db_utils_versions(cli_runner, postgresql, mock_import_swhmodule, module):
"""Check get_database_info, swh_db_versions and swh_db_module work ok
- This test checks db versions for both a db with "new style" set of sql init
- scripts (i.e. the dbversion table is not created in these scripts, but by
- the populate_database_for_package() function directly, via the 'swh db
- init' command) and an "old style" set (dbversion created in the scripts)S.
+ This test checks db versions is properly initialized by the cli db init
+ script.
+ mock_import_swhmodule should set the initial version to 3.
"""
conninfo = craft_conninfo(postgresql)
result = cli_runner.invoke(swhdb, ["init-admin", module, "--dbname", conninfo])
assert result.exit_code == 0, f"Unexpected output: {result.output}"
- result = cli_runner.invoke(
- swhdb, ["init", module, "--dbname", conninfo, "--initial-version", 10]
- )
+ result = cli_runner.invoke(swhdb, ["init", module, "--dbname", conninfo])
assert result.exit_code == 0, f"Unexpected output: {result.output}"
# check the swh_db_module() function
@@ -64,44 +61,38 @@ def test_db_utils_versions(cli_runner, postgresql, mock_import_swhmodule, module
versions = swh_db_versions(conninfo)
assert dbmodule == module
- assert dbversion == 10
+ assert dbversion == 3
assert dbflavor == "default"
# check also the swh_db_versions() function
versions = swh_db_versions(conninfo)
assert len(versions) == 1
- assert versions[0][0] == 10
- if module == "test.cli":
- assert versions[0][1] == datetime.fromisoformat(
- "2016-02-22T15:56:28.358587+00:00"
- )
- assert versions[0][2] == "Work In Progress"
- else:
- # new scheme but with no datastore (so no version support from there)
- assert versions[0][2] == "DB initialization"
+ assert versions[0][0] == 3
+ assert versions[0][2] == "DB initialization"
# add a few versions in dbversion
cnx = BaseDb.connect(conninfo)
with cnx.transaction() as cur:
cur.executemany(
"insert into dbversion(version, release, description) values (%s, %s, %s)",
- [(i, now(), f"Upgrade to version {i}") for i in range(11, 15)],
+ [(i, now(), f"Upgrade to version {i}") for i in range(4, 8)],
)
dbmodule, dbversion, dbflavor = get_database_info(conninfo)
assert dbmodule == module
- assert dbversion == 14
+ assert dbversion == 7
assert dbflavor == "default"
versions = swh_db_versions(conninfo)
assert len(versions) == 5
+
for i, (version, ts, desc) in enumerate(versions):
- assert version == (14 - i) # these are in reverse order
- if version > 10:
+ assert version == (7 - i) # these are in reverse order
+ if version > 3:
assert desc == f"Upgrade to version {version}"
assert (now() - ts) < timedelta(seconds=1)
-@pytest.mark.parametrize("module", ["test.cli_new"])
+@pytest.mark.parametrize("module", ["test.cli"])
def test_db_utils_upgrade(
cli_runner, postgresql, mock_import_swhmodule, module, datadir
):
@@ -112,7 +103,7 @@ def test_db_utils_upgrade(
result = cli_runner.invoke(swhdb, ["init", module, "--dbname", conninfo])
assert result.exit_code == 0, f"Unexpected output: {result.output}"
- assert swh_db_version(conninfo) == 1
+ assert swh_db_version(conninfo) == 3
new_version = swh_db_upgrade(conninfo, module)
assert new_version == 6
assert swh_db_version(conninfo) == 6
@@ -120,12 +111,12 @@ def test_db_utils_upgrade(
versions = swh_db_versions(conninfo)
# get rid of dates to ease checking
versions = [(v[0], v[2]) for v in versions]
- assert versions[-1] == (1, "DB initialization")
+ assert versions[-1] == (3, "DB initialization")
sqlbasedir = path.join(datadir, module.split(".", 1)[1], "sql", "upgrades")
assert versions[1:-1] == [
(i, f"Upgraded to version {i} using {sqlbasedir}/{i:03d}.sql")
- for i in range(5, 1, -1)
+ for i in range(5, 3, -1)
]
assert versions[0] == (6, "Updated version from upgrade script")
@@ -133,7 +124,8 @@ def test_db_utils_upgrade(
with cnx.transaction() as cur:
cur.execute("select url from origin where url like 'version%'")
result = cur.fetchall()
- assert result == [("version%03d" % i,) for i in range(2, 7)]
+
+ assert result == [("version%03d" % i,) for i in range(4, 7)]
cur.execute(
"select url from origin where url = 'this should never be executed'"
)
@@ -141,7 +133,7 @@ def test_db_utils_upgrade(
assert not result
-@pytest.mark.parametrize("module", ["test.cli_new"])
+@pytest.mark.parametrize("module", ["test.cli"])
def test_db_utils_swh_db_upgrade_sanity_checks(
cli_runner, postgresql, mock_import_swhmodule, module, datadir
):
@@ -186,7 +178,7 @@ def test_db_utils_swh_db_upgrade_sanity_checks(
swh_db_upgrade(conninfo, module)
-@pytest.mark.parametrize("module", ["test.cli", "test.cli_new"])
+@pytest.mark.parametrize("module", ["test.cli"])
@pytest.mark.parametrize("flavor", [None, "default", "flavorA", "flavorB"])
def test_db_utils_flavor(cli_runner, postgresql, mock_import_swhmodule, module, flavor):
"""Check populate_database_for_package handle db flavor properly"""
--
GitLab