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
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14252
Build 21912: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 21911: arc lint + arc unit

Unit TestsFailed

TimeTest
0 msJenkins > "before all" hook for "Should display properly entries"::Tests / Cypress tests / Run cypress tests / Test admin deposit page "before all" hook for "Should display properly entries"
CypressError: `cy.request()` failed on: http://localhost:5004/browse/content/sha1_git:30fda27bc2833697ebb7df1fa4be4ce6a6f4c27d/raw/?filename=tess.h
0 msJenkins > "before all" hook for "should be hidden when on top"::Tests / Cypress tests / Run cypress tests / Back-to-top button tests "before all" hook for "should be hidden when on top"
CypressError: `cy.request()` failed on: http://localhost:5004/browse/content/sha1_git:30fda27bc2833697ebb7df1fa4be4ce6a6f4c27d/raw/?filename=tess.h
0 msJenkins > "before all" hook for "should display accepted message when accepted"::Tests / Cypress tests / Run cypress tests / Origin Save Tests "before all" hook for "should display accepted message when accepted"
CypressError: `cy.request()` failed on: http://localhost:5004/browse/content/sha1_git:30fda27bc2833697ebb7df1fa4be4ce6a6f4c27d/raw/?filename=tess.h
0 msJenkins > "before all" hook for "should display all files and directories"::Tests / Cypress tests / Run cypress tests / Directory Tests "before all" hook for "should display all files and directories"
CypressError: `cy.request()` failed on: http://localhost:5004/browse/content/sha1_git:30fda27bc2833697ebb7df1fa4be4ce6a6f4c27d/raw/?filename=tess.h
0 msJenkins > "before all" hook for "should display correct file name"::Tests / Cypress tests / Run cypress tests / Test File Rendering "before all" hook for "should display correct file name"
CypressError: `cy.request()` failed on: http://localhost:5004/browse/content/sha1_git:30fda27bc2833697ebb7df1fa4be4ce6a6f4c27d/raw/?filename=tess.h
View Full Test Results (47 Failed · 388 Passed · 7 Skipped)

Event Timeline

swh/web/common/service.py
837

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

anlambert added inline comments.
swh/web/common/service.py
837

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

swh/web/common/service.py
837

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

  • 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

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).

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.

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

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

swh/web/common/service.py
837

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.

swh/web/common/service.py
837

does that change anything?

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

swh/web/common/service.py
837

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 ;)

swh/web/common/service.py
837

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