Page MenuHomeSoftware Heritage

Refactor Vault a bit, mainly pytest, config and BaseDb-based VaultBackend
ClosedPublic

Authored by douardda on Feb 8 2019, 5:31 PM.

Details

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
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4228
Build 5577: tox-on-jenkinsJenkins
Build 5576: arc lint + arc unit

Event Timeline

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

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
69–70

this todo can go away with this, can't it?

swh/vault/tests/test_backend.py
69

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 inline comments.
swh/vault/tests/test_backend.py
69

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.

swh/vault/tests/test_backend.py
69

Right, thx. I did not see that.

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.

ardumont added inline comments.
swh/vault/__init__.py
38

Instantiating

swh/vault/tests/conftest.py
20

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 added inline comments.
swh/vault/tests/conftest.py
20

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).

fix typo reported by ardumont

Few additions to the original diff

  • Add a swh.vault.api.wsgi module for keeping a singleton app instance. Doing so, we don't need the swh.vault.api.server.api_cfg config cache any more.
  • Use the SWH_CONFIG_FILENAME, if any, as config file name
swh/vault/tests/conftest.py
20

thx and thx!

This revision was automatically updated to reflect the committed changes.