Details

Reviewers

Group Reviewers

Commits

rDLDHG7238bd53bd5d: tasks: Enforce kwargs use in task message
rDLDHG5b6b33dd61a3: tests: Migrate to pytest-mock's fixture
rDLDHG37f4eb3035a1: tests: Clarify message
rDLDHGdfc0fae4370f: loader.mercurial: Register mercurial worker
rDLDHGf07d401e75a7: mercurial.tasks: Rename tasks according to production
rDLDHG1c4618e297c4: mercurial: Unify loader constructor

Summary

With this commit, we stop passing parameters to the load method. We pass those
parameters to the loader's constructor.

loader.mercurial: Register mercurial worker

Related to T2134

Test Plan

tox

Diff Detail

Repository

rDLDHG Mercurial loader

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 9580
Build 14105: tox-on-jenkins	Jenkins
Build 14104: arc lint + arc unit

Event Timeline

ardumont created this revision.Dec 5 2019, 3:00 PM

Build is green
See https://jenkins.softwareheritage.org/job/DLDHG/job/tox/39/ for more details.

Harbormaster completed remote builds in B9532: Diff 8453.Dec 5 2019, 3:01 PM

mercurial.tasks: Rename tasks according to production

Build is green
See https://jenkins.softwareheritage.org/job/DLDHG/job/tox/40/ for more details.

Harbormaster completed remote builds in B9533: Diff 8454.Dec 5 2019, 3:03 PM

ardumont retitled this revision from mercurial: Unify loader constructor + register worker (task, cli) to mercurial: Unify loader constructor + register worker (tasks, cli).Dec 5 2019, 3:15 PM

ardumont mentioned this in T2134: loader: Implement uniform loading CLI.Dec 6 2019, 10:04 AM

Looks good !

swh/loader/mercurial/__init__.py
10	s/Mapping/Dict/
swh/loader/mercurial/tasks.py
11	Why the `(*,` notation here ?
11–12	Just curious, why using `@shared_task` here as `@app.task` is used in the other loaders ?
swh/loader/mercurial/tests/test_tasks.py
5–6	Same remark as in D2398 and D2400, you should use `pytest-mock`

ardumont added inline comments.Dec 6 2019, 1:48 PM

swh/loader/mercurial/tasks.py
11	To force the use of kwargs in the tasks.
11–12	To stop importing the scheduler's app state. We had issues with that up to the point we stop doing that import. That's what we do in lister and loader-core's tasks. I'm just aligning as i go along (if i see it ;)

anlambert added inline comments.Dec 6 2019, 1:52 PM

swh/loader/mercurial/tasks.py
11	Oh I see, should the same behavior be ensured in other loader tasks ?
11–12	Ack, you should do the same for the git and svn loader then.

ardumont added inline comments.Dec 6 2019, 1:55 PM

swh/loader/mercurial/tasks.py
11	Yes, i think so. Though there comes the issue of migration (at least for recurring tasks ;)
11–12	Indeed. I possibly missed them.

Adapt according to review

Build is green
See https://jenkins.softwareheritage.org/job/DLDHG/job/tox/41/ for more details.

Harbormaster completed remote builds in B9577: Diff 8494.Dec 6 2019, 3:01 PM

anlambert accepted this revision.Dec 6 2019, 3:11 PM

anlambert added inline comments.

swh/loader/mercurial/tests/test_tasks.py
12–20	How about using this instead ? res = swh_app.send_task( 'swh.loader.mercurial.tasks.LoadMercurial', kwargs={'url': 'origin_url', 'directory': '/some/repo', 'visit_date': 'now'})
33–40	same here