Page MenuHomeSoftware Heritage

Simplify `swh deposit upload` cli options
ClosedPublic

Authored by douardda on Nov 6 2020, 11:15 AM.

Details

Summary

Get rid of the --archive_deposit and --metadata_deposit flags, these can
be deduced from the presence of --archive and --metadata.

Depends on D4431

Diff Detail

Event Timeline

Build is green

Patch application report for D4432 (id=15688)

Could not rebase; Attempt merge onto 0f4ec31168...

Updating 0f4ec311..021531bf
Fast-forward
 swh/deposit/cli/client.py               | 138 +++++++-------------------------
 swh/deposit/client.py                   | 112 ++++++++++++++++++--------
 swh/deposit/tests/cli/test_client.py    |  84 ++++---------------
 swh/deposit/tests/loader/test_client.py |  99 +++++++++--------------
 4 files changed, 162 insertions(+), 271 deletions(-)
Changes applied before test
commit 021531bfc7fa6a3e9d359fc9f6d11eccd2bc11ec
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Nov 3 11:42:05 2020 +0100

    Simplify `swh deposit upload` cli options
    
    Get rid of the --archive_deposit and --metadata_deposit flags, these can
    be deduced from the presence of --archive and --metadata.

commit 77900028c2c694fc67b1c3fa1046de60aa5e80f5
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Nov 2 14:39:42 2020 +0100

    Refactor BaseApiDepositClient to get rid of the _client argument
    
    This argument was only there to make it easier to mock the HTTP calls
    (via a mocked requests). Thanks to the mockup capabilities, this is in
    fact not necessary. So get rid of it and replace its usage in tests by
    proper use of the mocker pytest fixture.
    
    Also:
    - refactor this BaseApiDepositClient constructor to acces directly
      "url" and "auth" kwargs instead of an unspecified generic config dict
      (still supported but deprecated),
    - get rid of a few actually not really needed fixtures (deposit_config)
    - rewrite PrivateApiDepositClientStatusUpdateTest as a couple of pytest
      functions,
    - get rid of onliners functions in  swh.deposit.cli.client (_client(),
      deposit_create, deposit_update).

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/286/ for more details.

Are we sure there isn't a script that relies on them at one of SWH's partners?

swh/deposit/cli/client.py
175–181

you missed the quotes in these three lines

As i mentioned in irc during the week, I'm pretty sure those are to be used alongside
the --replace flag. So not sure if it's wise or not to remove those...

But i guess your argument about deduction out of --archive and --metadata flags stands.

Also, kinda in the same vein as vlorentz's comment, i just don't know who is using those
nor how to actually check that.

Maybe deprecating those flags, then actually explains that they
are redundant with --archive and --metadata would be best as a first step?

Build is green

Patch application report for D4432 (id=15727)

Could not rebase; Attempt merge onto 7148a257b2...

Updating 7148a257..abec2f3f
Fast-forward
 swh/deposit/cli/client.py               | 138 +++++++-------------------------
 swh/deposit/client.py                   | 112 ++++++++++++++++++--------
 swh/deposit/tests/cli/test_client.py    |  84 ++++---------------
 swh/deposit/tests/loader/test_client.py |  99 +++++++++--------------
 4 files changed, 162 insertions(+), 271 deletions(-)
Changes applied before test
commit abec2f3f73c0c9f5136525aae598db863e6b13a7
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Nov 3 11:42:05 2020 +0100

    Simplify `swh deposit upload` cli options
    
    Get rid of the --archive_deposit and --metadata_deposit flags, these can
    be deduced from the presence of --archive and --metadata.

commit 5e05355a83ae33a03d2daa60052dda3c600ac9c2
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Nov 2 14:39:42 2020 +0100

    Refactor BaseApiDepositClient to get rid of the _client argument
    
    This argument was only there to make it easier to mock the HTTP calls
    (via a mocked requests). Thanks to the mockup capabilities, this is in
    fact not necessary. So get rid of it and replace its usage in tests by
    proper use of the mocker pytest fixture.
    
    Also:
    - refactor this BaseApiDepositClient constructor to acces directly
      "url" and "auth" kwargs instead of an unspecified generic config dict
      (still supported but deprecated),
    - get rid of a few actually not really needed fixtures (deposit_config)
    - rewrite PrivateApiDepositClientStatusUpdateTest as a couple of pytest
      functions,
    - get rid of onliners functions in  swh.deposit.cli.client (_client(),
      deposit_create, deposit_update).

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/296/ for more details.

Build is green

Patch application report for D4432 (id=15736)

Could not rebase; Attempt merge onto 7148a257b2...

Updating 7148a257..62c1001a
Fast-forward
 swh/deposit/cli/client.py               | 135 ++++++--------------------------
 swh/deposit/client.py                   | 107 ++++++++++++++++---------
 swh/deposit/tests/cli/test_client.py    |  84 ++++----------------
 swh/deposit/tests/loader/test_client.py |  95 +++++++++-------------
 4 files changed, 150 insertions(+), 271 deletions(-)
Changes applied before test
commit 62c1001a8cafa9de7b8fb8c2ebc10f05f4fbe354
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Nov 3 11:42:05 2020 +0100

    Simplify `swh deposit upload` cli options
    
    Get rid of the --archive_deposit and --metadata_deposit flags, these can
    be deduced from the presence of --archive and --metadata.

commit 7e69a87f813630102696a2a0a4238cf5cf7545ff
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Nov 2 14:39:42 2020 +0100

    Refactor BaseApiDepositClient to get rid of the _client argument
    
    This argument was only there to make it easier to mock the HTTP calls
    (via a mocked requests). Thanks to the mockup capabilities, this is in
    fact not necessary. So get rid of it and replace its usage in tests by
    proper use of the mocker pytest fixture.
    
    Also:
    - refactor this BaseApiDepositClient constructor to acces directly
      "url" and "auth" kwargs instead of an unspecified generic config dict
      (still supported but deprecated),
    - get rid of a few actually not really needed fixtures (deposit_config)
    - rewrite PrivateApiDepositClientStatusUpdateTest as a couple of pytest
      functions,
    - get rid of onliners functions in  swh.deposit.cli.client (_client(),
      deposit_create, deposit_update).

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/300/ for more details.

fix formatting in the docstring of client_command_parse_input()

Build is green

Patch application report for D4432 (id=15738)

Could not rebase; Attempt merge onto 7148a257b2...

Updating 7148a257..3039fed9
Fast-forward
 swh/deposit/cli/client.py               | 137 ++++++--------------------------
 swh/deposit/client.py                   | 107 +++++++++++++++++--------
 swh/deposit/tests/cli/test_client.py    |  84 ++++----------------
 swh/deposit/tests/loader/test_client.py |  95 +++++++++-------------
 4 files changed, 151 insertions(+), 272 deletions(-)
Changes applied before test
commit 3039fed92730cc95ec5193aa79c316f667b74b85
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Nov 3 11:42:05 2020 +0100

    Simplify `swh deposit upload` cli options
    
    Get rid of the --archive_deposit and --metadata_deposit flags, these can
    be deduced from the presence of --archive and --metadata.

commit 7e69a87f813630102696a2a0a4238cf5cf7545ff
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Nov 2 14:39:42 2020 +0100

    Refactor BaseApiDepositClient to get rid of the _client argument
    
    This argument was only there to make it easier to mock the HTTP calls
    (via a mocked requests). Thanks to the mockup capabilities, this is in
    fact not necessary. So get rid of it and replace its usage in tests by
    proper use of the mocker pytest fixture.
    
    Also:
    - refactor this BaseApiDepositClient constructor to acces directly
      "url" and "auth" kwargs instead of an unspecified generic config dict
      (still supported but deprecated),
    - get rid of a few actually not really needed fixtures (deposit_config)
    - rewrite PrivateApiDepositClientStatusUpdateTest as a couple of pytest
      functions,
    - get rid of onliners functions in  swh.deposit.cli.client (_client(),
      deposit_create, deposit_update).

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/302/ for more details.

Build is green

Patch application report for D4432 (id=15781)

Could not rebase; Attempt merge onto 018500c195...

Updating 018500c1..d0ea3402
Fast-forward
 swh/deposit/cli/client.py               | 137 ++++++--------------------------
 swh/deposit/client.py                   | 107 +++++++++++++++++--------
 swh/deposit/tests/cli/test_client.py    |  84 ++++----------------
 swh/deposit/tests/loader/test_client.py |  95 +++++++++-------------
 4 files changed, 151 insertions(+), 272 deletions(-)
Changes applied before test
commit d0ea340251cdae349463c8f2fc192eaf692e6bec
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Nov 3 11:42:05 2020 +0100

    Simplify `swh deposit upload` cli options
    
    Get rid of the --archive_deposit and --metadata_deposit flags, these can
    be deduced from the presence of --archive and --metadata.

commit c7e397e527a13fc09ffb314c3a2cc9df69712160
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Nov 2 14:39:42 2020 +0100

    Refactor BaseApiDepositClient to get rid of the _client argument
    
    This argument was only there to make it easier to mock the HTTP calls
    (via a mocked requests). Thanks to the mockup capabilities, this is in
    fact not necessary. So get rid of it and replace its usage in tests by
    proper use of the mocker pytest fixture.
    
    Also:
    - refactor this BaseApiDepositClient constructor to acces directly
      "url" and "auth" kwargs instead of an unspecified generic config dict
      (still supported but deprecated),
    - get rid of a few actually not really needed fixtures (deposit_config)
    - rewrite PrivateApiDepositClientStatusUpdateTest as a couple of pytest
      functions,
    - get rid of onliners functions in  swh.deposit.cli.client (_client(),
      deposit_create, deposit_update).

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/314/ for more details.

As i mentioned in irc during the week, I'm pretty sure those are to be used alongside
the --replace flag. So not sure if it's wise or not to remove those...

But i guess your argument about deduction out of --archive and --metadata flags stands.

Also, kinda in the same vein as vlorentz's comment, i just don't know who is using those
nor how to actually check that.

Maybe deprecating those flags, then actually explains that they
are redundant with --archive and --metadata would be best as a first step?

trying to figure this out. Rereading the original code, I don't see how these flags have any impact on the way --replace behaves.

Honestly they do not seem to do much beside checking they are not given them both and enforce "archive=None" or "metadata=None", but I see no reason to do so when you can just not pass an archive file or a metadata one.

But I agree it would be best to keep them (deprecated) for now, but I would be very surprise if there is any user of these somewhere into the wild...

swh/deposit/cli/client.py
243

BTW @ardumont I did not understand this test. Why the partial ?

What's the meaning of a non-partial deposit with no archive and no metadata? Is it somehow to support the "closing" of a complete but still marked in-progress deposit? (which currently does not work but for other reasons).

keep the cli options but mark them as deprecated (and ignored)

Build is green

Patch application report for D4432 (id=15802)

Could not rebase; Attempt merge onto c815bbf19a...

Updating c815bbf1..74218875
Fast-forward
 swh/deposit/cli/client.py               | 169 ++++++++++----------------------
 swh/deposit/client.py                   | 107 +++++++++++++-------
 swh/deposit/tests/cli/test_client.py    |  84 ++++------------
 swh/deposit/tests/loader/test_client.py |  95 +++++++-----------
 4 files changed, 178 insertions(+), 277 deletions(-)
Changes applied before test
commit 7421887510ba497519e3650a836cc4708c4686ce
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Nov 3 11:42:05 2020 +0100

    Simplify `swh deposit upload` cli options
    
    Get rid of the --archive_deposit and --metadata_deposit flags, these can
    be deduced from the presence of --archive and --metadata.
    
    Keep the cli options and advertize them as deprecated.

commit 1129a092e6718c1c7bfc8bd467d4f389066f6b65
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Nov 2 14:39:42 2020 +0100

    Refactor BaseApiDepositClient to get rid of the _client argument
    
    This argument was only there to make it easier to mock the HTTP calls
    (via a mocked requests). Thanks to the mockup capabilities, this is in
    fact not necessary. So get rid of it and replace its usage in tests by
    proper use of the mocker pytest fixture.
    
    Also:
    - refactor this BaseApiDepositClient constructor to acces directly
      "url" and "auth" kwargs instead of an unspecified generic config dict
      (still supported but deprecated),
    - get rid of a few actually not really needed fixtures (deposit_config)
    - rewrite PrivateApiDepositClientStatusUpdateTest as a couple of pytest
      functions,
    - get rid of onliners functions in  swh.deposit.cli.client (_client(),
      deposit_create, deposit_update).

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/319/ for more details.

but I would be very surprise if there is any user of these somewhere into the wild...

life is full of surprises !

Yeah, i'd be too ¯\_(ツ)_/¯

swh/deposit/cli/client.py
243

I don't remember.

But your 2nd paragraph is what immediately cames to as an answer to the question
(prior to reading your second paragraph that is). So that might be it in the end.

This revision is now accepted and ready to land.Nov 10 2020, 5:41 PM