Details

Reviewers

Group Reviewers

Commits

rDVAU45d95d0ea2fd: Rewrite tests using pytest's fixtures and adapt them to recent refactorings
rDVAU8a3aad37c42e: Normalize the configuration of VaultBackend and cooker
rDVAUf0ad1f01a741: Refactor the VaultBackend to use BaseDb and pool-based db access
rDVAUf4ad96907d81: Make it possible to specify the config file via SWH_CONFIG_FILENAME env var
rDVAU62963319751f: Add a swh.vault.api.wsgi module to instanciate the (singleton) wsgi app object
rDVAU47ad8a9cc24c: Extract the main cli command in a dedicated cli.py module and make it an…

Summary

Extract the main cli command in a dedicated cli.py module and make it an entrypoint

Refactor the VaultBackend to use BaseDb and pool-based db access

Normalize the configuration of VaultBackend and cooker

ensure every service for which an (remote or local) access is needed uses the same config structure (dict with 'cls' and 'args' keys).

Rewrite tests using pytest's fuxture and adapt them to recent refactorings

Diff Detail

Repository

rDVAU Software Heritage Vault

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

douardda created this revision.Feb 8 2019, 5:31 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptFeb 8 2019, 5:31 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DVAU/job/tox/27/
See console output for more information: https://jenkins.softwareheritage.org/job/DVAU/job/tox/27/console

Harbormaster failed remote builds in B4125: Diff 3495!Feb 8 2019, 5:33 PM

Add missing hunks in tox.ini and requirement-test.txt

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DVAU/job/tox/33/
See console output for more information: https://jenkins.softwareheritage.org/job/DVAU/job/tox/33/console

Harbormaster failed remote builds in B4126: Diff 3496!Feb 8 2019, 5:39 PM

vlorentz mentioned this in T1495: Move swh.storage.db.BaseDb class in swh.core.Feb 11 2019, 11:38 AM

I still did not finish the review but, as far as i could see already, we are missing requirements bump.

It might not be useful for pip (since it uses the latest) but keeping the requirements updated can help for the debian build.
At least the minimum bump requirements, let's say following a migration ;)

I'd say here we'd need a bump in the requirements to at least swh.core 0.0.51 (as this draws the swh.storage.db.basedb migration from storage to the core).

swh/vault/backend.py
72 ↗	(On Diff #3496)	this todo can go away with this, can't it?
swh/vault/tests/test_backend.py
70 ↗	(On Diff #3496)	At some point (prior 09/2018 i think), i don't remember exactly on which diff though (possibly related to deposit), we agreed it was best to use `self.assertEqual` instead. Even though, that's not what is really tested here. On the positive argument that message from `self.assertEqual` convey better information on the error than the assert one.

On the account of discussing the bump requirements and possibly the assert vs self.assertEqual ;)

This revision now requires changes to proceed.Feb 11 2019, 3:22 PM

vlorentz added a subscriber: vlorentz.Feb 11 2019, 3:53 PM

vlorentz added inline comments.

swh/vault/tests/test_backend.py
70 ↗	(On Diff #3496)	These are pytest-style tests; there is no `self`. And pytest does introspection magic to print the values of both operands when the test fails.

ardumont added inline comments.Feb 11 2019, 3:54 PM

swh/vault/tests/test_backend.py
70 ↗	(On Diff #3496)	Right, thx. I did not see that.

vlorentz mentioned this in D1111: Add tests for swh.core.db..Feb 11 2019, 4:29 PM

I'm (re)working this diff, so wait a bit before spending time reviewing it.

Important rework of the config handling

mainly by extracting the instanciation of remote service proxies (storage etc.)
from the VaultBackend so there is no need for sreanding the config file
management all over the place (well, sort of).

Now, a typical config file for the server will look like:

vault:
  cls: local
  args:
    db: postgresql:///?service=swh-vault
storage:
  cls: remote
  args:
    url: http://swh-storage:5002/
scheduler:
  cls: remote
  args:
    url: http://swh-scheduler-api:5008/
cache:
  cls: pathslicing
  args:
    root: /srv/softwareheritage/vault
    slicing: 0:5

note that it can also be written as:

vault:
  cls: local
  args:
    db: postgresql:///?service=swh-vault
    storage:
      cls: remote
      args:
        url: http://swh-storage:5002/
    scheduler:
      cls: remote
      args:
        url: http://swh-scheduler-api:5008/
    cache:
      cls: pathslicing
      args:
        root: /srv/softwareheritage/vault
        slicing: 0:5

ie with service proxies defined within the vault config. These inner config
takes precedence over the global ones.

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DVAU/job/tox/34/
See console output for more information: https://jenkins.softwareheritage.org/job/DVAU/job/tox/34/console

Harbormaster failed remote builds in B4147: Diff 3516!Feb 12 2019, 11:03 AM

ardumont accepted this revision.Feb 14 2019, 11:03 AM

ardumont added inline comments.

swh/vault/__init__.py
38 ↗	(On Diff #3516)	Instantiating
swh/vault/tests/conftest.py
20 ↗	(On Diff #3516)	what's that for? retrocompatibility initialization voodoo required i guess.

This revision is now accepted and ready to land.Feb 14 2019, 11:03 AM

douardda marked an inline comment as done.Feb 14 2019, 12:08 PM

douardda added inline comments.

swh/vault/tests/conftest.py
20 ↗	(On Diff #3516)	the tmp_path fixture appeared in pytest 3.9, so yes, it's there so that we do not depend on pytest >=3.9 (especially for debian packaging, since stable is 3.0.6 and there is not backport).