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
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
douardda updated this revision to Diff 3496.Feb 8 2019, 5:37 PM

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

ardumont requested changes to this revision.Feb 11 2019, 3:22 PM

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

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

douardda updated this revision to Diff 3516.Feb 12 2019, 11:01 AM

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

douardda updated this revision to Diff 3584.Feb 14 2019, 12:17 PM

fix typo reported by ardumont

douardda updated this revision to Diff 3589.Feb 14 2019, 2:11 PM

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
ardumont added inline comments.Feb 14 2019, 2:15 PM
swh/vault/tests/conftest.py
20 ↗(On Diff #3516)

thx and thx!

ardumont accepted this revision.Feb 14 2019, 2:20 PM
douardda updated this revision to Diff 3592.Feb 14 2019, 2:38 PM

Fix tests

This revision was automatically updated to reflect the committed changes.