Page MenuHomeSoftware Heritage
Differential D7836

Add utility to sanitize and retrieve canonical github urls
ClosedPublic
Actions

Authored by ardumont on May 16 2022, 3:03 PM.

Details

Reviewers
anlambert
Group Reviewers
Reviewers
Maniphest Tasks
T4232: Listers: Canonicalize listed github origins
Commits
rDCORE60f384d35ad8: Add utility function to retrieve canonical github urls
Summary

Based on the work in webapp [1]

This new code is within a new arborescence as some more code will get moved alongside in
a new commit (the new session github currently in swh.lister module).

Related to T4232

[1] https://forge.softwareheritage.org/source/swh-web/browse/master/assets/src/utils/functions.js$118-160

Diff Detail

Repository
rDCORE Foundations and core functionalities
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.May 16 2022, 3:03 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptMay 16 2022, 3:03 PM
swh-public-ci added a comment.May 16 2022, 3:05 PM

Build is green

Patch application report for D7836 (id=28302)

Rebasing onto dae963440b...

Current branch diff-target is up to date.
Changes applied before test
commit dd1158bb1f308da55e8dd852a1916d60867e839b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon May 16 15:01:25 2022 +0200

    Add utility to sanitize and retrieve canonical github urls
    
    Related to T4232

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

Harbormaster completed remote builds in B29404: Diff 28302.May 16 2022, 3:05 PM
ardumont requested review of this revision.May 16 2022, 3:05 PM
anlambert added a subscriber: anlambert.May 16 2022, 3:11 PM
anlambert added inline comments.
swh/core/utils.py
219–233 ↗(On Diff #28302)

I think you should add support to pass GitHub API token in Authorization HTTP header, there is good chances we hit the rate limit without it.

ardumont added inline comments.May 16 2022, 3:22 PM
swh/core/utils.py
219–233 ↗(On Diff #28302)

Right, i had it in mind when i started and then I forgot it...

ardumont updated this revision to Diff 28307.May 16 2022, 3:51 PM

Move code in a dedicated module (explanation will be added in the diff description)

ardumont edited the summary of this revision. (Show Details)May 16 2022, 3:51 PM
swh-public-ci added a comment.May 16 2022, 3:53 PM

Build is green

Patch application report for D7836 (id=28307)

Rebasing onto dae963440b...

Current branch diff-target is up to date.
Changes applied before test
commit b9252925472099fe28714da568fe462a46ba70b7
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon May 16 15:01:25 2022 +0200

    Add utility to sanitize and retrieve canonical github urls
    
    This new code is within a new arborescence as some more code will get moved alongside in
    a new commit (the new session github currently in swh.lister module).
    
    Related to T4232

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

Harbormaster completed remote builds in B29409: Diff 28307.May 16 2022, 3:53 PM
ardumont added inline comments.May 16 2022, 3:54 PM
swh/core/utils.py
219–233 ↗(On Diff #28302)

I'll plug this in another diff.
I need to refactor [1] out of swh.lister here ^.

[1] https://forge.softwareheritage.org/source/swh-lister/browse/master/swh/lister/github/utils.py

vlorentz added a subscriber: vlorentz.May 16 2022, 3:54 PM
vlorentz added inline comments.
swh/core/github/utils.py
22–26

simpler, and also catches the "https://github.com/Foo/Bar/.git" case

ardumont added inline comments.May 16 2022, 4:04 PM
swh/core/github/utils.py
22–26

sneaky!

ardumont added inline comments.May 16 2022, 4:12 PM
swh/core/github/utils.py
22–26

ymmv, i don't find it easier in the end and, currently:

return re.match(r"(.*?)(.git)?/?", url.lower())[1]

fails on the first test case though:

E       AssertionError: assert '' == 'https://github.com/user/repo'
E         - https://github.com/user/repo
ardumont updated this revision to Diff 28308.May 16 2022, 4:18 PM
ardumont edited the summary of this revision. (Show Details)

Take care of the extra case not dealt with yet

I could not make the suggested improvment work though ¯\_(ツ)_/¯

anlambert accepted this revision.May 16 2022, 4:19 PM

Looks good to me.

This revision is now accepted and ready to land.May 16 2022, 4:19 PM
swh-public-ci added a comment.May 16 2022, 4:20 PM

Build is green

Patch application report for D7836 (id=28308)

Rebasing onto dae963440b...

Current branch diff-target is up to date.
Changes applied before test
commit a5ac30bc5d39e33fd5dfc4d0052d486b49d54bfb
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon May 16 15:01:25 2022 +0200

    Add utility to sanitize and retrieve canonical github urls
    
    This new code is within a new arborescence as some more code will get moved alongside in
    a new commit (the new session github currently in swh.lister module).
    
    Related to T4232

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

Harbormaster completed remote builds in B29410: Diff 28308.May 16 2022, 4:20 PM
ardumont updated this revision to Diff 28309.May 16 2022, 4:47 PM
  • Add missing copyright headers
  • Simplify a bit more the current sanitization code (no regexp though)
  • Improve docstring (to mention the anonymous http code <- for now)
swh-public-ci added a comment.May 16 2022, 4:49 PM

Build is green

Patch application report for D7836 (id=28309)

Rebasing onto dae963440b...

Current branch diff-target is up to date.
Changes applied before test
commit 0d2b85f7f9af818c387a8b19a8ee15be01c341e2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon May 16 15:01:25 2022 +0200

    Add utility to sanitize and retrieve canonical github urls
    
    This new code is within a new arborescence as some more code will get moved alongside in
    a new commit (the new session github currently in swh.lister module).
    
    Related to T4232

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

Harbormaster completed remote builds in B29411: Diff 28309.May 16 2022, 4:49 PM
ardumont updated this revision to Diff 28310.May 16 2022, 4:51 PM

Improve docstring and commit message further

swh-public-ci added a comment.May 16 2022, 4:53 PM

Build is green

Patch application report for D7836 (id=28310)

Rebasing onto dae963440b...

Current branch diff-target is up to date.
Changes applied before test
commit 60f384d35ad8d1e1fe442ea031d63262a5cafa22
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon May 16 15:01:25 2022 +0200

    Add utility function to retrieve canonical github urls
    
    This new code is within a new arborescence as some more code will get moved alongside in
    a new commit (the new session github currently in swh.lister module).
    
    That current code is making anonymous requests to the github api for now.
    
    Related to T4232

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

Harbormaster completed remote builds in B29412: Diff 28310.May 16 2022, 4:53 PM
Closed by commit rDCORE60f384d35ad8: Add utility function to retrieve canonical github urls (authored by ardumont). · Explain WhyMay 16 2022, 5:00 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDCORE60f384d35ad8: Add utility function to retrieve canonical github urls.
ardumont mentioned this in T4232: Listers: Canonicalize listed github origins.May 17 2022, 10:27 AM

Revision Contents
Changeset List

PathSize
swh/
core/
github/
48 lines
tests/
51 lines

Diff 28311

View Options

swh/core/github/__init__.py

# Copyright (C) 2018-2020 The Software Heritage developers # Copyright (C) 2018-2021 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution # See the AUTHORS file at the top-level directory of this distribution
# License: GNU General Public License version 3, or any later version # License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information # See top-level LICENSE file for more information
import copy import copy
import datetime import datetime
import os.path import os.path
from unittest import TestCase from unittest import TestCase
▲ Show 20 Lines • Show All 151 Lines • ▼ Show 20 Lines def test_load_unchanged(self):
assert_last_visit_matches( assert_last_visit_matches(
self.loader.storage, self.loader.storage,
self.repo_url, self.repo_url,
status="full", status="full",
type="git", type="git",
snapshot=SNAPSHOT1.id, snapshot=SNAPSHOT1.id,
) )
def test_load_visit_without_snapshot_so_status_failed(self):
# unfortunately, monkey-patch the hard way, self.loader is already instantiated
# (patching won't work self.loader is already instantiated)
# Make get_contents fail for some reason
self.loader.get_contents = None
res = self.loader.load()
assert res == {"status": "failed"}
assert_last_visit_matches(
self.loader.storage,
self.repo_url,
status="failed",
type="git",
snapshot=None,
)
def test_load_visit_with_snapshot_so_status_partial(self):
# unfortunately, monkey-patch the hard way, self.loader is already instantiated
# (patching won't work self.loader is already instantiated)
# fake store_metadata raising for some reason, so we could have a snapshot id
# at this point in time
self.loader.store_metadata = None
# fake having a snapshot so the visit status is partial
self.loader.loaded_snapshot_id = hash_to_bytes(
"a23699280a82a043f8c0994cf1631b568f716f95"
)
res = self.loader.load()
assert res == {"status": "failed"}
assert_last_visit_matches(
self.loader.storage,
self.repo_url,
status="partial",
type="git",
snapshot=None,
)
class FullGitLoaderTests(CommonGitLoaderTests): class FullGitLoaderTests(CommonGitLoaderTests):
"""Tests for GitLoader (from disk or not). Includes the common ones, and """Tests for GitLoader (from disk or not). Includes the common ones, and
add others that only work with a local dir. add others that only work with a local dir.
""" """
def test_load_changed(self): def test_load_changed(self):
▲ Show 20 Lines • Show All 228 Lines • Show Last 20 Lines
View Options

swh/core/github/utils.py

# Copyright (C) 2018-2020 The Software Heritage developers # Copyright (C) 2018-2021 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution # See the AUTHORS file at the top-level directory of this distribution
# License: GNU General Public License version 3, or any later version # License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information # See top-level LICENSE file for more information
import copy import copy
import datetime import datetime
import os.path import os.path
from unittest import TestCase from unittest import TestCase
▲ Show 20 Lines • Show All 151 Lines • ▼ Show 20 Lines def test_load_unchanged(self):
assert_last_visit_matches( assert_last_visit_matches(
self.loader.storage, self.loader.storage,
self.repo_url, self.repo_url,
status="full", status="full",
type="git", type="git",
snapshot=SNAPSHOT1.id, snapshot=SNAPSHOT1.id,
) )
def test_load_visit_without_snapshot_so_status_failed(self):
# unfortunately, monkey-patch the hard way, self.loader is already instantiated
# (patching won't work self.loader is already instantiated)
# Make get_contents fail for some reason
self.loader.get_contents = None
res = self.loader.load()
assert res == {"status": "failed"}
assert_last_visit_matches(
self.loader.storage,
self.repo_url,
status="failed",
type="git",
snapshot=None,
)
def test_load_visit_with_snapshot_so_status_partial(self):
# unfortunately, monkey-patch the hard way, self.loader is already instantiated
# (patching won't work self.loader is already instantiated)
# fake store_metadata raising for some reason, so we could have a snapshot id
# at this point in time
self.loader.store_metadata = None
# fake having a snapshot so the visit status is partial
self.loader.loaded_snapshot_id = hash_to_bytes(
"a23699280a82a043f8c0994cf1631b568f716f95"
)
res = self.loader.load()
assert res == {"status": "failed"}
assert_last_visit_matches(
self.loader.storage,
self.repo_url,
status="partial",
type="git",
snapshot=None,
)
class FullGitLoaderTests(CommonGitLoaderTests): class FullGitLoaderTests(CommonGitLoaderTests):
"""Tests for GitLoader (from disk or not). Includes the common ones, and """Tests for GitLoader (from disk or not). Includes the common ones, and
add others that only work with a local dir. add others that only work with a local dir.
""" """
def test_load_changed(self): def test_load_changed(self):
▲ Show 20 Lines • Show All 228 Lines • Show Last 20 Lines
View Options

swh/core/tests/test_github_utils.py

# Copyright (C) 2018-2020 The Software Heritage developers # Copyright (C) 2018-2021 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution # See the AUTHORS file at the top-level directory of this distribution
# License: GNU General Public License version 3, or any later version # License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information # See top-level LICENSE file for more information
import copy import copy
import datetime import datetime
import os.path import os.path
from unittest import TestCase from unittest import TestCase
▲ Show 20 Lines • Show All 151 Lines • ▼ Show 20 Lines def test_load_unchanged(self):
assert_last_visit_matches( assert_last_visit_matches(
self.loader.storage, self.loader.storage,
self.repo_url, self.repo_url,
status="full", status="full",
type="git", type="git",
snapshot=SNAPSHOT1.id, snapshot=SNAPSHOT1.id,
) )
def test_load_visit_without_snapshot_so_status_failed(self):
# unfortunately, monkey-patch the hard way, self.loader is already instantiated
# (patching won't work self.loader is already instantiated)
# Make get_contents fail for some reason
self.loader.get_contents = None
res = self.loader.load()
assert res == {"status": "failed"}
assert_last_visit_matches(
self.loader.storage,
self.repo_url,
status="failed",
type="git",
snapshot=None,
)
def test_load_visit_with_snapshot_so_status_partial(self):
# unfortunately, monkey-patch the hard way, self.loader is already instantiated
# (patching won't work self.loader is already instantiated)
# fake store_metadata raising for some reason, so we could have a snapshot id
# at this point in time
self.loader.store_metadata = None
# fake having a snapshot so the visit status is partial
self.loader.loaded_snapshot_id = hash_to_bytes(
"a23699280a82a043f8c0994cf1631b568f716f95"
)
res = self.loader.load()
assert res == {"status": "failed"}
assert_last_visit_matches(
self.loader.storage,
self.repo_url,
status="partial",
type="git",
snapshot=None,
)
class FullGitLoaderTests(CommonGitLoaderTests): class FullGitLoaderTests(CommonGitLoaderTests):
"""Tests for GitLoader (from disk or not). Includes the common ones, and """Tests for GitLoader (from disk or not). Includes the common ones, and
add others that only work with a local dir. add others that only work with a local dir.
""" """
def test_load_changed(self): def test_load_changed(self):
▲ Show 20 Lines • Show All 228 Lines • Show Last 20 Lines
About · Developer information · Legal