Build is green

Patch application report for D4376 (id=15480)

Could not rebase; Attempt merge onto eefe1b6336...

Updating eefe1b6..016ad26
Fast-forward
 requirements-db.txt           |  2 +-
 requirements-test-db.txt      |  2 +-
 swh/core/cli/db.py            | 54 +++++++++++++++++++++++++++++++++++++------
 swh/core/db/pytest_plugin.py  | 10 ++++++--
 swh/core/db/tests/test_cli.py |  6 +++--
 5 files changed, 61 insertions(+), 13 deletions(-)

Changes applied before test

commit 016ad26170fa86bca6a32dd9dc14812d5ef42d05
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Oct 29 09:20:32 2020 +0100

    db.pytest_plugin: Make the pytest-postgresql an optional runtime dependency
    
    Currently, this pulls the postgresql installation when installing swh.core
    which is not something wanted. That bypasses the current production pinning
    done in puppet and installs the latest postgresql-13 for example.
    
    That particular dependency should be installed alongside swh modules which
    needs the core db pytest-plugin for their tests.

commit 745448399ebda14f08a9286b08fb6dd008d9504b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Oct 28 18:27:46 2020 +0100

    cli.db: Open init-admin subcmd to initialize superuser-level scripts
    
    In staging, the db is already created by puppet, the current create database
    stanza forced within the create-db cannot work.
    
    Ths subsequent steps of postgresql extensions installation is required though.
    
    This will allow it.
    
    Related to T2736

See https://jenkins.softwareheritage.org/job/DCORE/job/tests-on-diff/122/ for more details.

Build is green

Patch application report for D4376 (id=15481)

Could not rebase; Attempt merge onto eefe1b6336...

Updating eefe1b6..272f91b
Fast-forward
 requirements-db.txt           |  2 +-
 requirements-test-db.txt      |  2 +-
 swh/core/cli/db.py            | 54 +++++++++++++++++++++++++++++++++++++------
 swh/core/db/pytest_plugin.py  | 10 ++++++--
 swh/core/db/tests/test_cli.py |  6 +++--
 5 files changed, 61 insertions(+), 13 deletions(-)

Changes applied before test

commit 272f91b939af9846dbc9c26172b1ead36885e156
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Oct 29 09:20:32 2020 +0100

    db.pytest_plugin: Make pytest-postgresql an optional runtime dependency
    
    Currently, this pulls the postgresql installation when installing swh.core
    which is not something wanted. That bypasses the current production pinning
    done in puppet and installs the latest postgresql-13 for example (even on node
    without postgresql).
    
    That particular dependency should be installed alongside swh modules which
    needs the core db pytest-plugin for their tests.

commit 745448399ebda14f08a9286b08fb6dd008d9504b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Oct 28 18:27:46 2020 +0100

    cli.db: Open init-admin subcmd to initialize superuser-level scripts
    
    In staging, the db is already created by puppet, the current create database
    stanza forced within the create-db cannot work.
    
    Ths subsequent steps of postgresql extensions installation is required though.
    
    This will allow it.
    
    Related to T2736

See https://jenkins.softwareheritage.org/job/DCORE/job/tests-on-diff/123/ for more details.

Where is the requirements-test-db.txt file used?

Having to declare transitive dependencies in modules using this pytest plugin sounds quite wrong. And having an extra version of postgresql installed isn't an issue at all, the postgresql Debian packages have been designed to be coinstallable without issues.

Having to declare transitive dependencies in modules using this pytest plugin sounds quite wrong.

Yes, I am not entirely satisfied with this.
But the pb described in the description is real and I don't know how to prevent it more properly than this...

And having an extra version of postgresql installed isn't an issue at all, the postgresql Debian packages have been designed to be coinstallable without issues.

It created actual issues on db (staging/vagrant) servers (which currently pins the 12 versions).
A 13 (new default version) one got installed unexpectedly (well for me ¯\_(ツ)_/¯).

Somehow both servers ran on the same port which did not bode well.
Or sometimes, one did not start or the wrong one did... or some combination of badness.

Also, without this, this means than any node installing a swh module requiring swh.core (all?) will install postgresql.
In my mind, that's not a desired behavior.

In D4376#108926, @ardumont wrote:

Having to declare transitive dependencies in modules using this pytest plugin sounds quite wrong.

Yes, I am not entirely satisfied with this.
But the pb described in the description is real and I don't know how to prevent it more properly than this...

And having an extra version of postgresql installed isn't an issue at all, the postgresql Debian packages have been designed to be coinstallable without issues.

It created actual issues on db (staging/vagrant) servers (which currently pins the 12 versions).
A 13 (new default version) one got installed unexpectedly (well for me ¯\_(ツ)_/¯).

Somehow both servers ran on the same port which did not bode well.
Or sometimes, one did not start or the wrong one did... or some combination of badness.

These issues need to be fixed on the puppet side anyway. If this change lands, nothing will prevent another dependency from pulling the postgresql-13 package and wreaking havoc. I guess the postgresql server puppet profile should just ship a /etc/postgresql-common/createcluster.conf file which would avoid the postgres package install creating the default postgres clusters (and order the creation of that file before any package install).

Also, without this, this means than any node installing a swh module requiring swh.core (all?) will install postgresql.
In my mind, that's not a desired behavior.

If that's really a problem, then we should separate the swh.core pytest plugin in a separate Debian package that would only be installed as a build-dependency of the modules that need it for their tests.

These issues need to be fixed on the puppet side anyway. If this change lands, nothing will prevent another dependency from pulling the postgresql-13 package and wreaking havoc

Indeed.
But it will prevent today to deploy massively postgresql everywhere it's not required.
So that's a first step.

I guess the postgresql server puppet profile should just ship a /etc/postgresql-common/createcluster.conf file which would
avoid the postgres package install creating the default postgres clusters (and order the creation of that file before any package install).

(iiuc what you suggest) That will just prevent that misbehavior on db nodes only...
So ok, this is needed for the db nodes.

Nonetheless, that's not enough for the other nodes...

Or do you want to add this file to every node? That would be surprising to have a /etc/postgresql-common/... configuration directory on nodes without postgresql...
So i'm a bit dubious...

If that's really a problem,

Installing postgres where it's not required is a problem to me.
If it's not required, it's not to be installed.

then we should separate the swh.core pytest plugin in a separate Debian package that would only be installed as a build-dependency of the modules that need it for their tests.

ok, i think i understand.
And then, python-side, we can add thed correct dependency in requirements-test.txt from the swh modules that need it (scheduler, ...)
And debian side, as you said, update the debian/control (swh.core) with that new package and for those swh modules (scheduler...), add in their debian/control build-dependency that very swh.core.pytest (or something) debian package.