Page MenuHomeSoftware Heritage

Save code now: Check origin exist prior to save request
ClosedPublic

Authored by ardumont on Apr 30 2021, 12:36 PM.

Details

Summary

This slightly modifies the current save code now workflow. Prior to register a new
origin url for "save code now", this first ensures said origin exists (through a HEAD
request done server side).

If the origin does not exist, an explanation notification is provided to the user.
Otherwise, the origin is submitted for registration as usual.

This should decrease the number of loading visits ending up in not_found status. This
also prepares work for the incoming new bundle visit type (which needs both that check
and the HEAD request anyway to fetch some more information).

Related to T3213

Test Plan

tox
make test-frontend

Diff Detail

Repository
rDWAPPS Web applications
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21185
Build 32885: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 32884: arc lint + arc unit

Event Timeline

ardumont retitled this revision from Open endpoint to check origin submitted for save code now exists to Save code now: Check origin exist prior to save request.Apr 30 2021, 12:40 PM

Build has FAILED

Patch application report for D5653 (id=20187)

Rebasing onto 03cfd89a1d...

First, rewinding head to replay your work on top of it...
Applying: Open endpoint to check origin submitted for save code now exists
Changes applied before test
commit 9721e72f76b96c173ad6348cda2ae9fcb1a81520
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Apr 29 19:03:09 2021 +0200

    Open endpoint to check origin submitted for save code now exists
    
    This slightly modifies the current save code now workflow. Prior to register a new
    origin url for "save code now", this first ensures said origin exists (through a HEAD
    request done server side).
    
    If the origin does not exist, an explanation notification is provided to the user.
    Otherwise, the origin is submitted for registration as usual.
    
    This should decrease the number of visit with not_found status. This also prepares work
    for the incoming new bundle visit type (which needs that check).
    
    Related to T3213

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

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 30 2021, 12:44 PM
Harbormaster failed remote builds in B21177: Diff 20187!
  • Rework commit message
  • Fix forgotten updates on tests server side

Build has FAILED

Patch application report for D5653 (id=20193)

Rebasing onto 03cfd89a1d...

First, rewinding head to replay your work on top of it...
Applying: Save code now: Check origin exist prior to save request
Changes applied before test
commit 6a28f79d0f6bb9771237725ba4a3ea4e3ce50bdd
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Apr 29 19:03:09 2021 +0200

    Save code now: Check origin exist prior to save request
    
    This slightly modifies the current save code now workflow. Prior to register a new
    origin url for "save code now", this first ensures said origin exists (through a HEAD
    request done server side).
    
    If the origin does not exist, an explanation notification is provided to the user.
    Otherwise, the origin is submitted for registration as usual.
    
    This should decrease the number of visit with not_found status. This also prepares work
    for the incoming new bundle visit type (which needs that check).
    
    Related to T3213

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

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 30 2021, 2:31 PM
Harbormaster failed remote builds in B21182: Diff 20193!
assets/src/bundles/save/index.js
202

For some reason, my docker experiment showed the actual desired error message (the one
raised from the python code) was in the reason key [1] and not the detail key as the
other conditional demonstrates.

[1]

Fix js test (it's not as easy as it sounds or so i'm experiencing ¯\_(ツ)_/¯ ;)

assets/src/bundles/save/index.js
202

also, we could do that to avoid modifying the utility function test below (to open the reason key)...

Build has FAILED

Patch application report for D5653 (id=20196)

Rebasing onto 03cfd89a1d...

First, rewinding head to replay your work on top of it...
Applying: Save code now: Check origin exist prior to save request
Changes applied before test
commit fe65a748aedab14bff09007d28d70cc0ea519f61
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Apr 29 19:03:09 2021 +0200

    Save code now: Check origin exist prior to save request
    
    This slightly modifies the current save code now workflow. Prior to register a new
    origin url for "save code now", this first ensures said origin exists (through a HEAD
    request done server side).
    
    If the origin does not exist, an explanation notification is provided to the user.
    Otherwise, the origin is submitted for registration as usual.
    
    This should decrease the number of visit with not_found status. This also prepares work
    for the incoming new bundle visit type (which needs that check).
    
    Related to T3213

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

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 30 2021, 3:19 PM
Harbormaster failed remote builds in B21185: Diff 20196!

Build has FAILED

lol, now what?!

Jenkins > should show not found for content::Tests / Cypress tests / Run cypress tests / Test origin-search Test invalid SWHIDs should show not found for content

ok, unrelated.

Build is green

Patch application report for D5653 (id=20196)

Rebasing onto 03cfd89a1d...

First, rewinding head to replay your work on top of it...
Applying: Save code now: Check origin exist prior to save request
Changes applied before test
commit d7aae1aa5ee7618a454232b8bfa395f312a00092
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Apr 29 19:03:09 2021 +0200

    Save code now: Check origin exist prior to save request
    
    This slightly modifies the current save code now workflow. Prior to register a new
    origin url for "save code now", this first ensures said origin exists (through a HEAD
    request done server side).
    
    If the origin does not exist, an explanation notification is provided to the user.
    Otherwise, the origin is submitted for registration as usual.
    
    This should decrease the number of visit with not_found status. This also prepares work
    for the incoming new bundle visit type (which needs that check).
    
    Related to T3213

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

assets/src/bundles/save/index.js
202

Hmm, this detail field looks weird to me, I have the feeling that it should be reason everywhere, let me check the git history.

assets/src/bundles/save/index.js
202

i agree.
It feels like i should align everything to that.

I did not check though and unshamingly relying on your memory for this ;)

203

also note that i did not want to actually change that ^ (it just stayed and i missed to remove it).
Since the build finally passed green, i waited for some comments first to update it...

assets/src/bundles/save/index.js
203

Turns out I removed the python code that was returning the dict with the detail field in a really recent commit: https://forge.softwareheritage.org/rDWAPPSf9b0b666d512564fddfc2fda65b2975fa0ea75c3#change-hmkCToN48Oc7

Now that we are using the Web API endpoint to create requests, the error detail field is now reason for all error responses.

assets/src/bundles/save/index.js
203

great then, i'll unify along the way (including the tests).

Looks good to me, some test comments need to be fixed though.

swh/web/common/origin_save.py
169

I think you can drop the is True.

swh/web/tests/common/test_origin_save.py
337

Comment is not correct here.

353

same here

This revision is now accepted and ready to land.Apr 30 2021, 4:55 PM
ardumont edited the summary of this revision. (Show Details)
  • Rebase

Adapt according to review:

  • Adapt the reason error inconsistency
  • Fix test docstring
  • Simplify the is True stanza
swh/web/common/origin_save.py
169

indeed, i wanted it to stand out but the simple version sounds enough today ;)

Build is green

Patch application report for D5653 (id=20198)

Rebasing onto 03cfd89a1d...

Current branch diff-target is up to date.
Changes applied before test
commit 450988b969007c9c8264bfe973c806bf5a68425f
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Apr 29 19:03:09 2021 +0200

    Save code now: Check origin exist prior to save request
    
    This slightly modifies the current save code now workflow. Prior to register a new
    origin url for "save code now", this first ensures said origin exists (through a HEAD
    request done server side).
    
    If the origin does not exist, an explanation notification is provided to the user.
    Otherwise, the origin is submitted for registration as usual.
    
    This should decrease the number of loading visits ending up in not_found status. This
    also prepares work for the incoming new bundle visit type (which needs both that check
    and the HEAD request anyway to fetch some more information).
    
    Related to T3213

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