Page MenuHomeSoftware Heritage

cli: register the 'journal' cli subcommand and improve it
ClosedPublic

Authored by douardda on Wed, Jun 5, 12:17 PM.

Details

Summary

also set the main group name to 'journal' so the main 'swh' cli tool
can generate shell completion properly.

  • add more doc strings to main cli commands,
  • adapt the main cli group to be used as a subcommand of the swh.core's main 'swh' command,
  • fix handling of the configuration file loading mechanism,
  • rename the 'consumer_id' argument as group_id, since this is indeed the group_id argument used to instantiate the underlying KafkaConsumer,
  • make the 'prefix' argument optional and default to the global variable swh.journal.DEFAULT_PREFIX = 'swh.journal.objects'.
  • hide several options of the 'replay' command ; these are now loaded preferably from the config file.

Diff Detail

Repository
rDJNL Journal infrastructure
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.Wed, Jun 5, 12:17 PM

lgtm. A few nitpicks in inline comments.

swh/journal/backfill.py
369 ↗(On Diff #5075)

This is probably outside the scope of this diff, but storage_dbconn could be renamed storage_dsn, to match psycopg2/libpq's terminology.

swh/journal/cli.py
87–88

I'm not sure, but we may want to check it's a list. It's going to give a very weird error if it's a string because pykafka will iterate over it

90–94

same comment as on D1541

96

We should document what --dry-run means in a few words.

douardda added inline comments.Wed, Jun 5, 4:26 PM
swh/journal/backfill.py
369 ↗(On Diff #5075)

I've noticed this, but going this way, I would rather use the "standard" swh config structure for services, aka:

storage:
  cls: local
  args:
    db: <dsn>
swh/journal/cli.py
87–88

good catch

96

if only I knew...

douardda updated this revision to Diff 5100.Thu, Jun 6, 10:12 AM

Several refactorings and git history cleanups

should also address all (most) vlorentz's comments...

douardda updated this revision to Diff 5130.Fri, Jun 7, 1:32 PM

fix an error (brokers can be a tuple) making test_cli fails

douardda updated this revision to Diff 5136.Fri, Jun 7, 2:31 PM

ok ok, I did split the kafka mock class in 2

vlorentz accepted this revision.Fri, Jun 7, 2:37 PM
This revision is now accepted and ready to land.Fri, Jun 7, 2:37 PM
This revision was automatically updated to reflect the committed changes.