Page MenuHomeSoftware Heritage
Differential D4843

Reimplement Bitbucket lister using new Lister API
ClosedPublic
Actions

Authored by tenma on Jan 12 2021, 2:27 PM.

Details

Reviewers
anlambert
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T2955: Port Bitbucket lister to the new Lister API
T2442: Provide a unified API for listers to interact with the scheduler
Commits
rDLS565e7423e39f: Reimplement Bitbucket lister using new Lister API
Summary

The new lister has incremental and full listing capability.
It can request the Bitbucket API in anonymous and HTTP basic authentication modes.
Rate-limiting is not aggressive and is handled.

Related to T2955

Test Plan

Covered: incremental and full modes, tasks, authentication
Not covered: some exceptional cases (see below)

tox

Diff Detail

Repository
rDLS Listers
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
swh-public-ci added a comment.Jan 13 2021, 11:08 AM

Build has FAILED

Patch application report for D4843 (id=17172)

Rebasing onto c782275296...

First, rewinding head to replay your work on top of it...
Applying: WIP new Bitbucket lister
Changes applied before test
commit 4afe9f7f6661fffec6e10fb7515dcf0405fc49f5
Author: tenma <tenma+swh@mailbox.org>
Date:   Tue Jan 12 18:08:53 2021 +0100

    WIP new Bitbucket lister

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

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 13 2021, 11:08 AM
Harbormaster failed remote builds in B18316: Diff 17172!
anlambert updated this revision to Diff 17175.Jan 13 2021, 11:46 AM

Fix failing test after changes in query parameters to Bitbucket API requests.

swh-public-ci added a comment.Jan 13 2021, 11:49 AM

Build is green

Patch application report for D4843 (id=17175)

Rebasing onto c782275296...

First, rewinding head to replay your work on top of it...
Applying: WIP new Bitbucket lister
Changes applied before test
commit 5748bf904b35e799d2328725d1b4ef8b37ba0969
Author: tenma <tenma+swh@mailbox.org>
Date:   Tue Jan 12 18:08:53 2021 +0100

    WIP new Bitbucket lister

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

Harbormaster completed remote builds in B18319: Diff 17175.Jan 13 2021, 11:49 AM
tenma requested review of this revision.Jan 13 2021, 11:49 AM
tenma updated this revision to Diff 17177.Jan 13 2021, 12:12 PM

refactor auth and backoff

swh-public-ci added a comment.Jan 13 2021, 12:16 PM

Build is green

Patch application report for D4843 (id=17177)

Rebasing onto c782275296...

First, rewinding head to replay your work on top of it...
Applying: WIP new Bitbucket lister
Changes applied before test
commit 5dfbe59051996bfe4338ec991fdc4a5ae59098ee
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Jan 13 11:54:18 2021 +0100

    WIP new Bitbucket lister
    
    Reviewers: #reviewers
    
    Subscribers: anlambert
    
    Tags: #bitbucket_lister, #lister
    
    Differential Revision: https://forge.softwareheritage.org/D4843

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

Harbormaster completed remote builds in B18321: Diff 17177.Jan 13 2021, 12:16 PM
anlambert updated this revision to Diff 17181.Jan 13 2021, 3:39 PM

Update:

  • add and improve tests
  • rename some variables and methods of the lister
  • allow to override configuration through scheduler tasks parameters
  • simplify JSON files for tests
swh-public-ci added a comment.Jan 13 2021, 3:42 PM

Build has FAILED

Patch application report for D4843 (id=17181)

Rebasing onto c782275296...

First, rewinding head to replay your work on top of it...
Applying: WIP new Bitbucket lister
Changes applied before test
commit 8a17cea06a1eac10e3720dc3995bcf130222d7b5
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Jan 13 12:20:17 2021 +0100

    WIP new Bitbucket lister

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

Harbormaster failed remote builds in B18325: Diff 17181!Jan 13 2021, 3:42 PM
ardumont added a subscriber: ardumont.Jan 13 2021, 4:46 PM

lgtm (minus the build failure that is ;)

(i did not read the test yet though)

one question about some more modules to remove inline.

swh/lister/bitbucket/lister.py
14

Do we entirely drop the old bitbucket lister code?

If yes, you can also remove that module.

tenma updated this revision to Diff 17193.Jan 13 2021, 6:28 PM

Improved tests

swh-public-ci added a comment.Jan 13 2021, 6:31 PM

Build is green

Patch application report for D4843 (id=17193)

Rebasing onto c782275296...

Current branch diff-target is up to date.
Changes applied before test
commit 489c9e25ab40b9ecc6b92e4cf9346ab7eef14978
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Jan 13 15:44:07 2021 +0100

    WIP new Bitbucket lister
    
    Reviewers: #reviewers
    
    Subscribers: anlambert
    
    Tags: #bitbucket_lister, #lister
    
    Differential Revision: https://forge.softwareheritage.org/D4843

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

Harbormaster completed remote builds in B18337: Diff 17193.Jan 13 2021, 6:31 PM
anlambert updated this revision to Diff 17197.Jan 13 2021, 7:05 PM

Update: Remove old range lister task and update tasks tests.

anlambert retitled this revision from WIP new Bitbucket lister to [WIP] Reimplement Bitbucket lister using new Lister API.Jan 13 2021, 7:06 PM
swh-public-ci added a comment.Jan 13 2021, 7:08 PM

Build is green

Patch application report for D4843 (id=17197)

Rebasing onto c782275296...

Current branch diff-target is up to date.
Changes applied before test
commit dffe53c2260188fb3ab8995113ac120359c7b9c3
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Jan 13 18:44:08 2021 +0100

    bitbucket: Reimplement lister using new API (WIP)

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

Harbormaster completed remote builds in B18341: Diff 17197.Jan 13 2021, 7:08 PM
tenma updated this revision to Diff 17198.Jan 13 2021, 7:24 PM

Fix copyrights and rebase

swh-public-ci added a comment.Jan 13 2021, 7:27 PM

Build is green

Patch application report for D4843 (id=17198)

Rebasing onto c782275296...

Current branch diff-target is up to date.
Changes applied before test
commit 4dd90ca2f489f406ef924daad33832a38fef96b1
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Jan 13 15:44:07 2021 +0100

    [WIP] Reimplement Bitbucket lister using new Lister API
    
    The new lister has incremental and full listing capability.
    It can request the Bitbucket API in anonymous and HTTP basic authentication
    modes. Rate-limiting is not aggressive and is handled.
    Listing mode, credentials and pagination parameters can be updated after
    creation.

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

Harbormaster completed remote builds in B18342: Diff 17198.Jan 13 2021, 7:27 PM
tenma retitled this revision from [WIP] Reimplement Bitbucket lister using new Lister API to Reimplement Bitbucket lister using new Lister API.Jan 14 2021, 10:26 AM
tenma edited the test plan for this revision. (Show Details)
tenma edited the summary of this revision. (Show Details)
tenma edited the summary of this revision. (Show Details)Jan 14 2021, 10:33 AM
tenma added a task: T2442: Provide a unified API for listers to interact with the scheduler.
tenma edited the test plan for this revision. (Show Details)Jan 14 2021, 10:45 AM
tenma edited the test plan for this revision. (Show Details)
vlorentz added a subscriber: vlorentz.Jan 14 2021, 12:33 PM

Not covered: authentication, max retries, small corner cases (see below)

What do you mean?

What is self.backoff? You're using it both as a time (time.sleep(self.backoff)) and as a dimensionless unit (self.backoff = self.BACKOFF_FACTOR)

And self.request_count should be a local variable of get_pages named retries_count, because it's actually used for. Plus, the way it's currently written, it looks like that whenever it reaches MAX_RETRIES, it will never be reset without creating a new BitbucketLister object.

swh/lister/bitbucket/lister.py
87

maybe a warning if there is more than one set of credentials?

90–101

Shouldn't these be (class)methods of BitbucketListerState?

103
108
113

And why does it need to be Optional[bool] instead of just bool?

174

IMO this yield should be moved before if next_page_url is not None for two reasons:

  1. we probably want to yield body["values"] even if we fail to parse next_page_url
  2. this means we can make last_repo_cdate non-optional and replace last_repo_cdate = None with a break (and make the loop a while True)
175
swh/lister/bitbucket/tests/test_lister.py
164

isn't this a method?

vlorentz requested changes to this revision.Jan 14 2021, 12:34 PM
This revision now requires changes to proceed.Jan 14 2021, 12:34 PM
vlorentz added a comment.Jan 14 2021, 12:36 PM

Sorry, I clicked "submit" too early.

I meant to say I don't have a good idea of how well it fits architecturaly, but I have some small comments nonetheless

In D4843#121373, @vlorentz wrote:

What is self.backoff? You're using it both as a time (time.sleep(self.backoff)) and as a dimensionless unit (self.backoff = self.BACKOFF_FACTOR)

Nevermind that, I guess self.backoff = self.BACKOFF_FACTOR is implicitly meant as multiplying the backoff by 1 second. The initial backoff could be configurable though, but that's not very important.

tenma edited the summary of this revision. (Show Details)Jan 14 2021, 12:40 PM
tenma added a task: T2955: Port Bitbucket lister to the new Lister API.
tenma added a comment.Jan 14 2021, 3:02 PM

Thanks for the remarks, will go through them.

swh/lister/bitbucket/lister.py
90–101

the new lister API has them as instance methods of the lister, but indeed the API could be amended to have it as ListerState class methods.
I would say, let discuss it and do that later.

swh/lister/bitbucket/tests/test_lister.py
164

oops, will fix this.

vlorentz added inline comments.Jan 14 2021, 3:19 PM
swh/lister/bitbucket/lister.py
90–101

yeah that makes sense, let's leave it like this then

tenma updated this revision to Diff 17294.Jan 18 2021, 9:59 AM
  • fix assert
  • fix typing
  • add a warning on credentials
swh-public-ci added a comment.Jan 18 2021, 10:02 AM

Build is green

Patch application report for D4843 (id=17294)

Rebasing onto c782275296...

Current branch diff-target is up to date.
Changes applied before test
commit 609ed8c893542a0f3206822bcf1fd8185cabf2a2
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Jan 13 15:44:07 2021 +0100

    [WIP] Reimplement Bitbucket lister using new Lister API
    
    The new lister has incremental and full listing capability.
    It can request the Bitbucket API in anonymous and HTTP basic authentication
    modes. Rate-limiting is not aggressive and is handled.
    Listing mode, credentials and pagination parameters can be updated after
    creation.

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

Harbormaster completed remote builds in B18433: Diff 17294.Jan 18 2021, 10:02 AM
tenma marked 6 inline comments as done.Jan 18 2021, 10:43 AM
tenma marked 2 inline comments as done.Jan 18 2021, 7:13 PM
tenma updated this revision to Diff 17317.Jan 18 2021, 7:43 PM
  • apply @vlorentz suggestions
  • use tenacity retrying decorator through lister.utils thanks to D4869
  • remove setters no more useful after D4872
  • adapt tests accordingly
swh-public-ci added a comment.Jan 18 2021, 7:46 PM

Build is green

Patch application report for D4843 (id=17317)

Rebasing onto 9fd91f007d...

Current branch diff-target is up to date.
Changes applied before test
commit 2cfc017435e8dee06f24717a947e8fe1e6f2d940
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Jan 13 15:44:07 2021 +0100

    [WIP] Reimplement Bitbucket lister using new Lister API
    
    The new lister has incremental and full listing capability.
    It can request the Bitbucket API in anonymous and HTTP basic authentication
    modes. Rate-limiting is not aggressive and is handled.

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

Harbormaster completed remote builds in B18456: Diff 17317.Jan 18 2021, 7:46 PM
tenma updated this revision to Diff 17318.Jan 18 2021, 8:32 PM

Have test coverage for credentials setting

swh-public-ci added a comment.Jan 18 2021, 8:35 PM

Build is green

Patch application report for D4843 (id=17318)

Rebasing onto 9fd91f007d...

Current branch diff-target is up to date.
Changes applied before test
commit b0b247e7a25e761c88ec0419cecc95a4198ffb7c
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Jan 13 15:44:07 2021 +0100

    [WIP] Reimplement Bitbucket lister using new Lister API
    
    The new lister has incremental and full listing capability.
    It can request the Bitbucket API in anonymous and HTTP basic authentication
    modes. Rate-limiting is not aggressive and is handled.

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

Harbormaster completed remote builds in B18457: Diff 17318.Jan 18 2021, 8:35 PM
tenma edited the test plan for this revision. (Show Details)Jan 19 2021, 10:18 AM
tenma edited the summary of this revision. (Show Details)
tenma updated this revision to Diff 17325.Jan 19 2021, 10:29 AM

Remove the persistent isort conftest modification from diff

anlambert added inline comments.Jan 19 2021, 10:31 AM
swh/lister/bitbucket/lister.py
105

Can you rename the function to set_credentials, shorter is better. Also parameters should be typed to Optional[str].

111

you can remove that parameter as it is already the default value of the decorator

swh-public-ci added a comment.Jan 19 2021, 10:32 AM

Build is green

Patch application report for D4843 (id=17325)

Rebasing onto 9fd91f007d...

Current branch diff-target is up to date.
Changes applied before test
commit dec3559f0ae277ad9271468444b6cad494bff6b9
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Jan 13 15:44:07 2021 +0100

    [WIP] Reimplement Bitbucket lister using new Lister API
    
    The new lister has incremental and full listing capability.
    It can request the Bitbucket API in anonymous and HTTP basic authentication
    modes. Rate-limiting is not aggressive and is handled.

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

Harbormaster completed remote builds in B18464: Diff 17325.Jan 19 2021, 10:32 AM
tenma updated this revision to Diff 17326.Jan 19 2021, 10:57 AM

Remove tenacity wait parameter

swh-public-ci added a comment.Jan 19 2021, 11:00 AM

Build is green

Patch application report for D4843 (id=17326)

Rebasing onto 9fd91f007d...

Current branch diff-target is up to date.
Changes applied before test
commit c8d7259ecbf9590e67160345d3cbf561f9206195
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Jan 13 15:44:07 2021 +0100

    [WIP] Reimplement Bitbucket lister using new Lister API
    
    The new lister has incremental and full listing capability.
    It can request the Bitbucket API in anonymous and HTTP basic authentication
    modes. Rate-limiting is not aggressive and is handled.

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

Harbormaster completed remote builds in B18465: Diff 17326.Jan 19 2021, 11:00 AM
tenma updated this revision to Diff 17329.Jan 19 2021, 11:31 AM

Revert changes to the credentials setting method, and improve corresponding test

swh-public-ci added a comment.Jan 19 2021, 11:34 AM

Build is green

Patch application report for D4843 (id=17329)

Rebasing onto 9fd91f007d...

Current branch diff-target is up to date.
Changes applied before test
commit 1b23378781dbfdccd4243556ade9f8d306a0d29c
Author: tenma <tenma+swh@mailbox.org>
Date:   Wed Jan 13 15:44:07 2021 +0100

    [WIP] Reimplement Bitbucket lister using new Lister API
    
    The new lister has incremental and full listing capability.
    It can request the Bitbucket API in anonymous and HTTP basic authentication
    modes. Rate-limiting is not aggressive and is handled.

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

Harbormaster completed remote builds in B18468: Diff 17329.Jan 19 2021, 11:34 AM
anlambert accepted this revision.Jan 19 2021, 1:59 PM

Looks good to me, time to land this !

vlorentz resigned from this revision.Jan 20 2021, 3:26 PM
This revision is now accepted and ready to land.Jan 20 2021, 3:26 PM
Closed by commit rDLS565e7423e39f: Reimplement Bitbucket lister using new Lister API (authored by tenma). · Explain WhyJan 20 2021, 3:29 PM
This revision was automatically updated to reflect the committed changes.
tenma marked 2 inline comments as done.
tenma added a commit: rDLS565e7423e39f: Reimplement Bitbucket lister using new Lister API.

Revision Contents
Changeset List

Diff 17396

View Options

requirements-test.txt

Loading...
View Options

swh/lister/bitbucket/__init__.py

Loading...
View Options

swh/lister/bitbucket/lister.py

Loading...
View Options

swh/lister/bitbucket/models.py

Loading...
View Options

swh/lister/bitbucket/tasks.py

Loading...
View Options

swh/lister/bitbucket/tests/conftest.py

Loading...
View Options

swh/lister/bitbucket/tests/data/bb_api_repositories_page1.json

Loading...
View Options

swh/lister/bitbucket/tests/data/bb_api_repositories_page2.json

Loading...
View Options

swh/lister/bitbucket/tests/data/https_api.bitbucket.org/2.0_repositories,after=1970-01-01T00:00:00+00:00,pagelen=100

Loading...
View Options

swh/lister/bitbucket/tests/data/https_api.bitbucket.org/empty_response.json

Loading...
View Options

swh/lister/bitbucket/tests/data/https_api.bitbucket.org/response.json

Loading...
View Options

swh/lister/bitbucket/tests/test_lister.py

Loading...
View Options

swh/lister/bitbucket/tests/test_tasks.py

Loading...
About · Developer information · Legal