Details

Reviewers

vlorentz
anlambert

Group Reviewers

Reviewers

Maniphest Tasks

T645: Type swh-storage endpoints with swh.model objects

Commits

rDWAPPS9c41d9f90f58: Adapt to latest storage release_get api change

Summary

Related to T645

Test Plan

locally:

tox (amended with local storage) ok
pytest

= 420 passed, 7 skipped in 15.20s =
_ summary _
...
  flake8: commands succeeded
  mypy: commands succeeded
  py3: commands succeeded

(somehow black always want to reformat everything now).

ci:

tox (failing until release swh.storage > 0.13.2)

Diff Detail

Repository

rDWAPPS Web applications

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Aug 31 2020, 4:44 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptAug 31 2020, 4:44 PM

You could change converters.from_release while you're at it, the three calls you changed to add .to_dict() are the only ones

ardumont added inline comments.Aug 31 2020, 4:47 PM

swh/web/common/service.py
485	The existing code below expects the release to actually exist. So I'm making this apparent in the code and in the docstring.
swh/web/tests/conftest.py
208	why doesn't mypy say something? it's not a ReleaseMetadata, it's a Dict... I recall it annoyed me elsewhere and I had to change.

Build has FAILED

Patch application report for D3854 (id=13613)

Rebasing onto d53a5188c0...

Current branch diff-target is up to date.

Changes applied before test

commit 91cba481d1235efbc8137fed02251641d3dc48ac
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Aug 31 16:44:39 2020 +0200

    Adapt to latest storage release_get api change
    
    Related to T645

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/346/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/346/console

Harbormaster failed remote builds in B14845: Diff 13613!Aug 31 2020, 4:47 PM

vlorentz added inline comments.Aug 31 2020, 4:48 PM

swh/web/tests/conftest.py
208	because `converters.from_release` isn't typed

You could change converters.from_release while you're at it, the three calls you changed to add .to_dict() are the only ones

Right, thanks. I'll check that and the other parts we discussed.

vlorentz requested changes to this revision.Sep 1 2020, 10:21 AM

This revision now requires changes to proceed.Sep 1 2020, 10:21 AM

requested changes

yeah, i'm on it,
it's just there are more tests to change if we touch the converters module.

;)

Adapt according to suggestions:

Adapt converters.from_release to deal with Release model objects
Fix mistyped method

Tests should still fail because new storage release is needed

Build has FAILED

Patch application report for D3854 (id=13617)

Rebasing onto d53a5188c0...

Current branch diff-target is up to date.

Changes applied before test

commit e84827cbd5d0e7df8692a53a5435e6b976ca63cd
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Aug 31 16:44:39 2020 +0200

    Adapt to latest storage release_get api change
    
    Related to T645

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/347/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/347/console

Harbormaster failed remote builds in B14849: Diff 13617!Sep 1 2020, 10:50 AM

ardumont edited the test plan for this revision. (Show Details)Sep 1 2020, 10:50 AM

vlorentz accepted this revision.Sep 1 2020, 12:38 PM

This revision is now accepted and ready to land.Sep 1 2020, 12:38 PM

anlambert added a subscriber: anlambert.Sep 1 2020, 1:11 PM

anlambert added inline comments.

swh/web/common/service.py

485

Previous behavior of the lookup_release_multiple function was to yield None when a release does nos exist, see below:

Python 3.7.3 (default, Apr  3 2019, 05:39:12) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from swh.web.common import service
>>> list(service.lookup_release_multiple(['5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8c', '5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8d']))
>>> list(service.lookup_release_multiple(['5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8c', '5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8d']))
[{'name': 'v2.6.11-tree', 'message': "This is the 2.6.11 tree object.\n\nNOTE! There's no commit for this, since it happened before I started with git.\nEventually we'll import some sort of history, and that should tie this tree\nobject up to a real commit. In the meantime, this acts as an anchor point for\ndoing diffs etc under git.\n-----BEGIN PGP SIGNATURE-----\nVersion: GnuPG v1.2.4 (GNU/Linux)\n\niD8DBQBCeV/eF3YsRnbiHLsRAl+SAKCVp8lVXwpUhMEvy8N5jVBd16UCmACeOtP6\nKLMHist5yj0sw1E4hDTyQa0=\n=/bIK\n-----END PGP SIGNATURE-----\n", 'target': 'c39ae07f393806ccf406ef966e9a15afc43cc36a', 'target_type': 'directory', 'synthetic': False, 'author': None, 'date': None, 'id': '5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8c'}, None]

After arc patching storage with D3852 and web with that diff, an invalid release is now filtered out, see below:

Python 3.7.3 (default, Apr  3 2019, 05:39:12) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from swh.web.common import service
>>> list(service.lookup_release_multiple(['5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8c', '5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8d']))
[{'name': 'v2.6.11-tree', 'message': "This is the 2.6.11 tree object.\n\nNOTE! There's no commit for this, since it happened before I started with git.\nEventually we'll import some sort of history, and that should tie this tree\nobject up to a real commit. In the meantime, this acts as an anchor point for\ndoing diffs etc under git.\n-----BEGIN PGP SIGNATURE-----\nVersion: GnuPG v1.2.4 (GNU/Linux)\n\niD8DBQBCeV/eF3YsRnbiHLsRAl+SAKCVp8lVXwpUhMEvy8N5jVBd16UCmACeOtP6\nKLMHist5yj0sw1E4hDTyQa0=\n=/bIK\n-----END PGP SIGNATURE-----\n", 'target': 'c39ae07f393806ccf406ef966e9a15afc43cc36a', 'target_type': 'directory', 'synthetic': False, 'author': None, 'date': None, 'id': '5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8c'}]
>>>

I think we should still yield None when an invalid release is provided in the input list.

This breaks the previous behavior of the lookup_release_multiple function, see my inline comment.

This revision now requires changes to proceed.Sep 1 2020, 1:13 PM

ardumont added inline comments.Sep 1 2020, 1:19 PM

swh/web/common/service.py
485	Thanks! (I was gonna ask you for a cross-check ;)

anlambert added inline comments.Sep 1 2020, 1:19 PM

swh/web/common/service.py
485	Turns out that function is not tested at all ... On the contrary `lookup_revision_multiple` is and capture the yield None behavior (search for `test_lookup_revision_multiple_none_found`)

ardumont added inline comments.Sep 1 2020, 1:34 PM

swh/web/common/service.py
485	It's tested alright. I don't get why I did not have failures locally... Adaptations on its way.

ardumont edited the test plan for this revision. (Show Details)Sep 1 2020, 1:35 PM

ardumont edited the test plan for this revision. (Show Details)Sep 1 2020, 1:39 PM

ardumont edited the test plan for this revision. (Show Details)

ardumont added inline comments.Sep 1 2020, 1:42 PM

swh/web/common/service.py
485	rah, got it, revision, not release ¯\_(ツ)_/¯ .oO(d'oh, sleep ardu, sleep!) Ack, i'll add the equivalent tests...

Adaptation, unknown release should be returned as None in lookup_multiple_release

(remains tests to add on that function)

Build has FAILED

Patch application report for D3854 (id=13621)

Rebasing onto d53a5188c0...

Current branch diff-target is up to date.

Changes applied before test

commit eca8ce48d56f837a06bfbbf3432a8d839b5f5b5b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Aug 31 16:44:39 2020 +0200

    Adapt to latest storage release_get api change
    
    Related to T645

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/348/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/348/console

Harbormaster failed remote builds in B14853: Diff 13621!Sep 1 2020, 1:46 PM

Add tests to service.lookup_release_multiple function

ardumont added inline comments.Sep 1 2020, 2:12 PM

swh/web/tests/conftest.py
208	right, thanks for reminding me! (and it is typed now ;)

Build has FAILED

Patch application report for D3854 (id=13622)

Rebasing onto d53a5188c0...

Current branch diff-target is up to date.

Changes applied before test

commit 9c41d9f90f58f0e1c645cfcd2839abacfd564bc1
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Aug 31 16:44:39 2020 +0200

    Adapt to latest storage release_get api change
    
    This also adds tests to the `service.lookup_release_multiple` function.
    
    Related to T645

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/349/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/349/console

Harbormaster failed remote builds in B14854: Diff 13622!Sep 1 2020, 2:12 PM

ardumont edited the test plan for this revision. (Show Details)Sep 1 2020, 2:15 PM

ardumont mentioned this in D3852: storage*: release_get(...) -> List[Optional[Release]].

Build is green

Patch application report for D3854 (id=13622)

Rebasing onto d53a5188c0...

Current branch diff-target is up to date.

Changes applied before test

commit 9c41d9f90f58f0e1c645cfcd2839abacfd564bc1
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Aug 31 16:44:39 2020 +0200

    Adapt to latest storage release_get api change
    
    This also adds tests to the `service.lookup_release_multiple` function.
    
    Related to T645

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

Harbormaster completed remote builds in B14854: Diff 13622.Sep 1 2020, 2:56 PM

anlambert accepted this revision.Sep 1 2020, 3:08 PM

This revision is now accepted and ready to land.Sep 1 2020, 3:08 PM

Closed by commit rDWAPPS9c41d9f90f58: Adapt to latest storage release_get api change (authored by ardumont). · Explain WhySep 1 2020, 3:11 PM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDWAPPS9c41d9f90f58: Adapt to latest storage release_get api change.

swh.web: Adapt to latest storage release_get api change
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Patch application report for D3854 (id=13613)

Changes applied before test

Patch application report for D3854 (id=13617)

Changes applied before test

Patch application report for D3854 (id=13621)

Changes applied before test

Patch application report for D3854 (id=13622)

Changes applied before test

Patch application report for D3854 (id=13622)

Changes applied before test

Revision Contents
Changeset List

Diff 13626

swh/web/browse/snapshot_context.py

swh/web/common/converters.py

swh/web/common/service.py

swh/web/tests/common/test_converters.py

swh/web/tests/common/test_service.py

swh/web/tests/conftest.py

swh/web/tests/data.py

swh/web/tests/strategies.py

swh.web: Adapt to latest storage release_get api changeClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D3854 (id=13613)

Changes applied before test

Patch application report for D3854 (id=13617)

Changes applied before test

Patch application report for D3854 (id=13621)

Changes applied before test

Patch application report for D3854 (id=13622)

Changes applied before test

Patch application report for D3854 (id=13622)

Changes applied before test

Revision ContentsChangeset List

Diff 13626

swh/web/browse/snapshot_context.py

swh/web/common/converters.py

swh/web/common/service.py

swh/web/tests/common/test_converters.py

swh/web/tests/common/test_service.py

swh/web/tests/conftest.py

swh/web/tests/data.py

swh/web/tests/strategies.py

swh.web: Adapt to latest storage release_get api change
ClosedPublic
Actions

Revision Contents
Changeset List