Page MenuHomeSoftware Heritage
Differential D3693

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

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

Details

Reviewers
anlambert
Group Reviewers
Reviewers
Maniphest Tasks
T645: Type swh-storage endpoints with swh.model objects
Commits
rDWAPPS8f1e003bc78f: service: Adapt according to the latest storage.content_find changes
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 14260
Build 21927: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 21926: arc lint + arc unit

Unit TestsFailed
View All

TimeTest
189 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.web.tests.common.test_identifiers::Tests / Python tests / test_resolve_swhids_snapshot_context
client = <django.test.client.Client object at 0x7f394d94b668> archive_data = <swh.web.tests.conftest._ArchiveData object at 0x7f3954243898>
214 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.web.tests.admin.test_origin_save::Tests / Python tests / test_accept_pending_save_request
662 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.web.tests.admin.test_origin_save::Tests / Python tests / test_add_authorized_origin_url
116 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.web.tests.admin.test_origin_save::Tests / Python tests / test_add_unauthorized_origin_url
123 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.web.tests.admin.test_origin_save::Tests / Python tests / test_reject_pending_save_request
View Full Test Results (1 Failed · 543 Passed · 7 Skipped)

Event Timeline

ardumont created this revision.Aug 4 2020, 11:15 AM
Herald added a reviewer: Reviewers. · View Herald TranscriptAug 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 ;)

swh-public-ci added a comment.Aug 4 2020, 11:19 AM

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

Harbormaster failed remote builds in B14252: Diff 13010!Aug 4 2020, 11:19 AM
ardumont mentioned this in D3692: storage*: Type content_find(...) -> List[Content].Aug 4 2020, 11:22 AM
ardumont edited the test plan for this revision. (Show Details)Aug 4 2020, 11:28 AM
anlambert added a subscriber: anlambert.Aug 4 2020, 1:27 PM
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)
swh-public-ci added a comment.Aug 4 2020, 1:48 PM

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

Harbormaster failed remote builds in B14260: Diff 13018!Aug 4 2020, 1:48 PM
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
anlambert added a comment.Aug 4 2020, 2:07 PM

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

ardumont added a comment.Aug 4 2020, 2:08 PM

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

ardumont added a comment.Aug 4 2020, 2:11 PM

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
anlambert added a comment.Aug 4 2020, 2:14 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.

ardumont added a subscriber: vlorentz.Aug 4 2020, 2:20 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

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

swh-public-ci added a comment.Aug 4 2020, 2:24 PM

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

anlambert added a comment.Aug 4 2020, 2:24 PM
In D3693#91386, @ardumont wrote:

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.

ardumont added a comment.Aug 4 2020, 2:25 PM

Build has FAILED

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

ardumont added a comment.Aug 4 2020, 2:27 PM

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.

anlambert added a comment.Aug 4 2020, 2:34 PM
In D3693#91390, @ardumont wrote:

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

anlambert added a comment.Aug 4 2020, 2:42 PM

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.

swh-public-ci added a comment.Aug 4 2020, 2:43 PM

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

swh-public-ci added a comment.Aug 4 2020, 2:49 PM

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.

Harbormaster completed remote builds in B14263: Diff 13020.Aug 4 2020, 2:49 PM
Closed by commit rDWAPPS8f1e003bc78f: service: Adapt according to the latest storage.content_find changes (authored by ardumont). · Explain WhyAug 4 2020, 2:51 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDWAPPS8f1e003bc78f: service: Adapt according to the latest storage.content_find changes.
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.

ardumont added a comment.Aug 4 2020, 4:47 PM
In D3693#91391, @ardumont wrote:

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.

ardumont added a comment.Aug 4 2020, 5:01 PM

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.

Revision Contents
Changeset List

PathSize
swh/
web/
common/
67 lines
tests/
6 lines

Diff 13018

View Options

swh/web/common/service.py

Loading...
View Options

swh/web/tests/conftest.py

Loading...
About · Developer information · Legal