Page MenuHomeSoftware Heritage

service: Adapt according to the latest storage.content_find changes
ClosedPublic

Authored by ardumont on Aug 4 2020, 11:15 AM.

Details

Summary

storage.content_find now returns List[Content].

This also adds types annotation to the impacted methods.

Related to T645

Related to D3692

Test Plan

tox
(failing until release > 0.11.9)

locally ok:

  • pytest ok
  • tox patched with D3692 ok ( .tox/py3/bin/pip install -e ../swh-storage

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 4 2020, 11:15 AM
ardumont added inline comments.Aug 4 2020, 11:19 AM
swh/web/common/service.py
837–838

This follows a recent discussion about name clashes with standard keyword message [1]

[1] https://forge.softwareheritage.org/D3684#inline-25462

Tell me if you disagree (I see i did not modify everywhere so i can always revert, and it's not per say the subject of the diff ;)

Build has FAILED

Patch application report for D3693 (id=13010)

Rebasing onto 325d820f62...

Current branch diff-target is up to date.
Changes applied before test
commit 1088d4514dbc471296fc9ffc13342417e6c2db9e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Aug 4 11:13:21 2020 +0200

    service: Adapt according to the latest changes in storage
    
    `storage.content_find` now returns a sequence of Content
    
    Related to T645

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

ardumont edited the test plan for this revision. (Show Details)Aug 4 2020, 11:28 AM
anlambert added inline comments.
swh/web/common/service.py
837–838

I am good with that convention. There must be plenty of other ones to fix (like dir) in swh-web codebase.

ardumont edited the summary of this revision. (Show Details)Aug 4 2020, 1:28 PM
ardumont added inline comments.Aug 4 2020, 1:30 PM
swh/web/common/service.py
837–838

ok, so i'll change the other impacted functions with this as well then (if you still agree with that thinking ;)

ardumont updated this revision to Diff 13018.Aug 4 2020, 1:44 PM
  • Rename some more variables
  • improve commit message
ardumont retitled this revision from service: Adapt according to the latest changes in storage to service: Adapt according to the latest storage.content_find changes.Aug 4 2020, 1:44 PM
ardumont edited the test plan for this revision. (Show Details)

Build has FAILED

Patch application report for D3693 (id=13018)

Rebasing onto 325d820f62...

Current branch diff-target is up to date.
Changes applied before test
commit 30641e98c1f176a902a039742d65c6e52b228385
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Aug 4 11:13:21 2020 +0200

    service: Adapt according to the latest storage.content_find changes
    
    `storage.content_find` now returns List[Content]. This adapts the service layer
    to convert it to something manageable by the webapp.
    
    Related to T645

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

anlambert accepted this revision.Aug 4 2020, 2:02 PM

LGTM. Can you tag storage to unstuck Jenkins builds of swh-web ?

This revision is now accepted and ready to land.Aug 4 2020, 2:02 PM

Also you should update required storage version in requirements-swh.txt.

LGTM.

cool ;)

Note that I modified the webapp along the migration I did in storage to introduce types.
But I did not refactor too much within the webapp.

Can you tag storage to unstuck Jenkins builds of swh-web ?

Yes, ok.

Note that it will unstuck the tox build not the debian one though (due to a cassandra deps issue on the unstable distribution).

Also you should update required storage version in requirements-swh.txt.

yes! I do it but gets in the way of running locally stuff through tox (especially the "patched" one as described in the tox plan).

ardumont edited the test plan for this revision. (Show Details)Aug 4 2020, 2:11 PM

Note that it will unstuck the tox build not the debian one though (due to a cassandra deps issue on the unstable distribution).

I do not intend to tag swh-web today so fixing debian builds can wait.

Note that it will unstuck the tox build not the debian one though (due to a cassandra deps issue on the unstable distribution).

I do not intend to tag swh-web today so fixing debian builds can wait

tbc, it's in progress while we are discussing ;)

(my understanding of the debian build issue is that it's out of our hands as @vlorentz concluded yesterday in irc ;)

Build has FAILED

Patch application report for D3693 (id=13018)

Rebasing onto 325d820f62...

Current branch diff-target is up to date.
Changes applied before test
commit 30641e98c1f176a902a039742d65c6e52b228385
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Aug 4 11:13:21 2020 +0200

    service: Adapt according to the latest storage.content_find changes
    
    `storage.content_find` now returns List[Content]. This adapts the service layer
    to convert it to something manageable by the webapp.
    
    Related to T645

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

Note that it will unstuck the tox build not the debian one though (due to a cassandra deps issue on the unstable distribution).

I do not intend to tag swh-web today so fixing debian builds can wait

tbc, it's in progress while we are discussing ;)

(my understanding of the debian build issue is that it's out of our hands as @vlorentz concluded yesterday in irc ;)

We can still skip the debian unstable build by manually running the debian/automatic-backport Jenkins job with the right parameters.

Build has FAILED

This, i did not expect...
I'll dig in.

We can still skip the debian unstable build by manually running the debian/automatic-backport Jenkins job with the right parameters.

I did not realize, thanks for the tips.

Build has FAILED

This, i did not expect...
I'll dig in.

Restarting the build should fix it now storage is tagged.

ardumont added a comment.EditedAug 4 2020, 2:36 PM

Restarting the build should fix it now storage is tagged.

Yeah, slowly realizing that the build must have run with storage 0.11.9... [1]
trying to check the build logs...

[1] that's it swh.storage==0.11.9

ardumont updated this revision to Diff 13020.Aug 4 2020, 2:42 PM

Update requirements-swh.txt

Build failure in https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/300/ is not related to that diff.

Looks like a flaky test is remaining as I also encountered it in D3695 builds.

Build has FAILED

Patch application report for D3693 (id=13018)

Rebasing onto 325d820f62...

Current branch diff-target is up to date.
Changes applied before test
commit 30641e98c1f176a902a039742d65c6e52b228385
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Aug 4 11:13:21 2020 +0200

    service: Adapt according to the latest storage.content_find changes
    
    `storage.content_find` now returns List[Content]. This adapts the service layer
    to convert it to something manageable by the webapp.
    
    Related to T645

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

vlorentz added inline comments.Aug 4 2020, 2:47 PM
swh/web/common/service.py
837–838

hash is a builtin function, not a keyword, though

Build is green

Patch application report for D3693 (id=13020)

Rebasing onto 325d820f62...

Current branch diff-target is up to date.
Changes applied before test
commit 8f1e003bc78f58f917277e8601a54aad0111b7ec
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Aug 4 11:13:21 2020 +0200

    service: Adapt according to the latest storage.content_find changes
    
    `storage.content_find` now returns List[Content]. This adapts the service layer
    to convert it to something manageable by the webapp.
    
    Related to T645

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

ardumont added inline comments.Aug 4 2020, 4:40 PM
swh/web/common/service.py
837–838

does that change anything?

(it's highlighted as a keyword by my editor with plain hash, now it's better ;)

anlambert added inline comments.Aug 4 2020, 4:44 PM
swh/web/common/service.py
837–838

it's highlighted as a keyword by my editor with plain hash, now it's better ;)

Same behavior with my editor too, that highlighting is confusing.

We can still skip the debian unstable build by manually running the debian/automatic-backport Jenkins job with the right parameters.

I did not realize, thanks for the tips.

Thanks, it's currently building for stable btw.

Same behavior with my editor too, that highlighting is confusing.

Totally, gets ce confused a split second each time ;)

vlorentz added inline comments.Aug 5 2020, 8:47 AM
swh/web/common/service.py
837–838

The difference is that it's only variable shadowing instead of a SyntaxError.