Page MenuHomeSoftware Heritage

Deprecate /origin/content route
ClosedPublic

Authored by jayeshv on Nov 5 2021, 1:25 PM.

Details

Summary

This fix deprecates and redirects /origin/content?origin_url=<url> and
/origin/<url>/content to /content/<sha1_git>

  • test fixes
  • documentation changes

Related to T3608
Fixes T3697

Diff Detail

Repository
rDWAPPS Web applications
Branch
origin-content
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25005
Build 39072: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 39071: arc lint + arc unit

Event Timeline

jayeshv retitled this revision from Deprecate /origin/content route to [WIP] Deprecate /origin/content route.Nov 5 2021, 1:26 PM
jayeshv edited the summary of this revision. (Show Details)

Build has FAILED

Patch application report for D6615 (id=24018)

Rebasing onto 1e106c798e...

Current branch diff-target is up to date.
Changes applied before test
commit 32a49a41e9fb7cb0bbbe9ec1da557121957ef5ec
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Nov 5 13:21:35 2021 +0100

    Deprecate /origin/content route
    
    This fix deprecates and redirects /origin/content?origin_url=<url> and
    /origin/<url>/content to /content/<sha1_git>
    - test fixes
    - documentation changes
    
    Related to T3608
    Fixes T3697

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

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 5 2021, 1:47 PM
Harbormaster failed remote builds in B24891: Diff 24018!

redirecting snapshot/content route

Build has FAILED

Patch application report for D6615 (id=24122)

Rebasing onto 1e106c798e...

Current branch diff-target is up to date.
Changes applied before test
commit d9fd08080fa26486d8ab4c13048fb056d367e65b
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Wed Nov 10 14:53:51 2021 +0100

    Deprecate /origin/content and /snapshot/content routes
    
    This fix deprecates and redirects /origin/content?origin_url=<url>,
    /origin/<url>/content, /snapshot/<id>/content and /snapshot/<id>/content/<path>
    to /content/<sha1_git>
    
    - test fixes
    - documentation changes
    
    Related to T3608
    Fixes T3697

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

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 10 2021, 3:19 PM
Harbormaster failed remote builds in B25001: Diff 24122!

Build was aborted

Patch application report for D6615 (id=24126)

Rebasing onto 1e106c798e...

Current branch diff-target is up to date.
Changes applied before test
commit 0164aa332d18ae8c9af23c2c94c7b3b0dfbd6355
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Wed Nov 10 16:06:11 2021 +0100

    Deprecate /origin/content and /snapshot/content routes
    
    This fix deprecates and redirects /origin/content?origin_url=<url>,
    /origin/<url>/content, /snapshot/<id>/content and /snapshot/<id>/content/<path>
    to /content/<sha1_git>
    
    - test fixes
    - documentation changes
    
    Related to T3608
    Fixes T3697

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

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 10 2021, 4:34 PM
Harbormaster failed remote builds in B25005: Diff 24126!

Build has FAILED

Patch application report for D6615 (id=24131)

Rebasing onto fe8c3b38bb...

First, rewinding head to replay your work on top of it...
Applying: Deprecate /origin/content and /snapshot/content routes
Changes applied before test
commit 191063ec569d568de13f1d8619ec3ae5c850b76b
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Mon Nov 15 12:20:42 2021 +0100

    Deprecate /origin/content and /snapshot/content routes
    
    This fix deprecates and redirects /origin/content?origin_url=<url>,
    /origin/<url>/content, /snapshot/<id>/content and /snapshot/<id>/content/<path>
    to /content/<sha1_git>
    
    - test fixes
    - documentation changes
    
    Related to T3608
    Fixes T3697

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

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 15 2021, 12:36 PM
Harbormaster failed remote builds in B25011: Diff 24131!

Build is green

Patch application report for D6615 (id=24135)

Rebasing onto e2659cfed5...

Current branch diff-target is up to date.
Changes applied before test
commit cfb782b486f2df1d92f747614045ec68a6c6abdc
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Mon Nov 15 15:57:25 2021 +0100

    Deprecate /origin/content and /snapshot/content routes
    
    This fix deprecates and redirects /origin/content?origin_url=<url>,
    /origin/<url>/content, /snapshot/<id>/content and /snapshot/<id>/content/<path>
    to /content/<sha1_git>
    
    - test fixes
    - documentation changes
    
    Related to T3608
    Fixes T3697

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

Build is green

Patch application report for D6615 (id=24140)

Rebasing onto e2659cfed5...

Current branch diff-target is up to date.
Changes applied before test
commit f9579e8e57430c153bc22092db3f8ccf2630de2c
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Tue Nov 16 11:59:12 2021 +0100

    Deprecate /origin/content and /snapshot/content routes
    
    This fix deprecates and redirects /origin/content?origin_url=<url>,
    /origin/<url>/content, /snapshot/<id>/content and /snapshot/<id>/content/<path>
    to /content/<sha1_git>
    
    - test fixes
    - documentation changes
    
    Related to T3608
    Fixes T3697

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

jayeshv retitled this revision from [WIP] Deprecate /origin/content route to Deprecate /origin/content route.Nov 16 2021, 12:18 PM
jayeshv edited the summary of this revision. (Show Details)

Build is green

Patch application report for D6615 (id=24141)

Rebasing onto e2659cfed5...

Current branch diff-target is up to date.
Changes applied before test
commit b317e5ab617f0ea3bb9c7895c6ec119eb490c0b9
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Tue Nov 16 12:16:14 2021 +0100

    Deprecate /origin/content and /snapshot/content routes
    
    This fix deprecates and redirects /origin/content?origin_url=<url>,
    /origin/<url>/content, /snapshot/<id>/content and /snapshot/<id>/content/<path>
    to /content/<sha1_git>
    
    - test fixes
    - documentation changes
    
    Related to T3608
    Fixes T3697

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

Build is green

Patch application report for D6615 (id=24156)

Rebasing onto 189d204e9b...

First, rewinding head to replay your work on top of it...
Applying: Deprecate /origin/content and /snapshot/content routes
Changes applied before test
commit fa8be2b76b411ebe3825abeea91a2cad7f15a6ae
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Tue Nov 16 16:24:53 2021 +0100

    Deprecate /origin/content and /snapshot/content routes
    
    This fix deprecates and redirects /origin/content?origin_url=<url>,
    /origin/<url>/content, /snapshot/<id>/content and /snapshot/<id>/content/<path>
    to /content/
    
    - test fixes
    - documentation changes
    
    Related to T3608
    Fixes T3697

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

anlambert added a subscriber: anlambert.

Overall looks good to me ! I added a couple of nitpick comments to handle before landing this.

assets/src/bundles/guided_tour/index.js
69

Why did you not use ${Urls.browse_content()}?origin_url=${originUrl}&path=Example/example.c here ?

docs/uri-scheme-browse-content.rst
43 ↗(On Diff #24156)

Could you add the doc (copy paste from below) for the following accepted query parameters: origin_url, snapshot, branch, release, timestamp ?

102–103 ↗(On Diff #24156)

this is optional too, HEAD branch will be used if not provided.

104–105 ↗(On Diff #24156)

also optional

swh/web/browse/views/content.py
175

The path query parameter must be provided.

179

The origin_url or snapshot query parameters must be provided.

210–211

this case happens when redirected either from /browse/origin/content or /browse/snapshot/content

swh/web/common/utils.py
421–426

Too many lines in that docstring imho, you should make it more compact as you can use up to 88 columns per line.

This revision now requires changes to proceed.Nov 17 2021, 12:09 PM

Build was aborted

Patch application report for D6615 (id=24166)

Rebasing onto 9c874cd715...

Current branch diff-target is up to date.
Changes applied before test
commit 8bbca7dde29426d317818f0eb313edc949c898f3
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Wed Nov 17 13:33:24 2021 +0100

    Deprecate /origin/content and /snapshot/content routes
    
    This fix deprecates and redirects /origin/content?origin_url=<url>,
    /origin/<url>/content, /snapshot/<id>/content and /snapshot/<id>/content/<path>
    to /content/
    
    - test fixes
    - documentation changes
    
    Related to T3608
    Fixes T3697

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

jayeshv added inline comments.
assets/src/bundles/guided_tour/index.js
69

This is going to an infinite redirect loop without the contentSha1. It is because the js wrapper function that controls the guided tour page is not designed to work with server redirects. It will restart the js script each time it reloads and continue the loop.
Other possible fix is to explicitly pop guided_tour param from query during redirect.

assets/src/bundles/guided_tour/index.js
69

Just figured that out too, let's keep it that way then.

Build is green

Patch application report for D6615 (id=24169)

Rebasing onto 9c874cd715...

Current branch diff-target is up to date.
Changes applied before test
commit c491d4e46a3f3c96c2e2831515d77e0ac92b90ff
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Wed Nov 17 14:07:27 2021 +0100

    Deprecate /origin/content and /snapshot/content routes
    
    This fix deprecates and redirects /origin/content?origin_url=<url>,
    /origin/<url>/content, /snapshot/<id>/content and /snapshot/<id>/content/<path>
    to /content/
    
    - test fixes
    - documentation changes
    
    Related to T3608
    Fixes T3697

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

Bye bye duplicated code related to browsing contents, thanks a lot for that !

This revision is now accepted and ready to land.Nov 17 2021, 2:38 PM