Page MenuHomeSoftware Heritage

swh.deposit.cli: Transform create_user into a main cli (+ docs updates)
ClosedPublic

Authored by ardumont on Feb 24 2019, 9:46 AM.

Diff Detail

Repository
rDDEP Push deposit
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4419
Build 5845: tox-on-jenkinsJenkins
Build 5844: arc lint + arc unit

Event Timeline

ardumont added inline comments.
setup.py
55

context: Deposit is split in 3 debian packages (python3-swh.deposit, python3-swh.deposit.client, python3-swh.deposit.loader) so this will be installed broken for some of those (i think).

The swh-deposit-client cli is packaged in a dedicated debian package so i expect this to not be broken for that package only (i could be wrong).

@douardda In any case, it might be worth considering to merge that in the main cli?

  • settings.production: Improve error message when missing information
ardumont added inline comments.
docs/sys-info.rst
44–46

I wonder how to explain this in the cli...
Also, that might be a possible improvment to add that check... I don't know how to be 'dry' though.
That check is already done in the settings.production...

nevermind, i'll update the diff.

ardumont added inline comments.
docs/sys-info.rst
44–46

In D1184#25162, i raise a more explicit message for the missing tested case (env variable not set).

  • swh.manage: Migrate manage to swh/deposit subfolder

Unrelated but since i fixed issues already. I noticed that the
manage.py module in questions ends up in the wrong root tree once
installed.

I'm globally OK with this, but please add support for a --config-file cmd line option in parallel with the environment variable support, as other swh-xxx commands do.

And we might want to refactor the swh-deposit-client tool as a swh-deposit client subcommand instead, but that will be another diff.

docs/dev-info.rst
121–122

described

122–123

maybe add a line describing what are these private configs for?

docs/sys-info.rst
44–47

For a command line example like this, I'd rather use something like:

swh-deposit -C /etc/softwareheritage/deposit/server.yml --platform production \
[...]

rather than having to define the env var...

Note: I've not verified whether the swh-deposit command support a -C option, but you get the point.

setup.py
55

@douardda In any case, it might be worth considering to merge that in the main cli?

yes this is something we might want at some point, but it's not that easy to do properly, we might need lazy loading mechanisms to prevent the tool to be too slugish for example...

55

context: Deposit is split in 3 debian packages (python3-swh.deposit, python3-swh.deposit.client, python3-swh.deposit.loader) so this will be installed broken for some of those (i think).

I guess both python3-swh.deposit.client and python3-swh.deposit.loader depends on python3-swh.deposit, so why not have a single swh-deposit entry point that comes with this later?

Then, a simple

try:
    import swh.deposit.client.cli
except ImportError:
    pass

in swh.deposit.cli should do the trick, I guess. Sure, the swh.deposit.client.cli should be modified to use the swh.deposit.cli.cli main group instead of redefinig one. Same applies to swh.deposit.loader.

This revision now requires changes to proceed.Mar 8 2019, 11:35 AM

I'm globally OK with this, but please add support for a --config-file cmd line option in parallel with the environment variable support, as other swh-xxx commands do.

I planned on doing that in another diff.
I'm doing stuff incrementally. That's easier to review.

Here the goal was to align with other modules to have a swh.<module>.cli as before the script was there but dedicated to one action (that's what the title tried to convey).
Now, with this it's aligned.

I added the collection support which should have been in another diff already ;)

setup.py
55

I guess both python3-swh.deposit.client and python3-swh.deposit.loader depends on python3-swh.deposit,

No they don't, description is:

  • python3-swh.deposit: public exposed sword api, private api/rpc server (for loaders)
  • python3-swh.deposit.loader: loader, they use a client to discuss with the private api of the server
  • python3-swh.deposit.client: http client to discuss with the public api sword of the server

Dependency is only:
python3-swh.deposit.loader -depends-on-> python3-swh.deposit.client

Ok to go incrementally like that, but ensure a task/diff making this cli more consistent with other swh commands exists.

This revision is now accepted and ready to land.Mar 12 2019, 2:30 PM
ardumont edited the summary of this revision. (Show Details)

Ok to go incrementally like that, but ensure a task/diff making this cli more consistent with other swh commands exists.

T1580!

Thanks for the review.

This revision was automatically updated to reflect the committed changes.