1177257 - Remove revision-lookup API

Status: RESOLVED FIXED

(Tree Management :: Treeherder: API, defect)

Description

[William Lachance (:wlach)]

When going through the API methods listed at https://treeherder.mozilla.org/docs, I noticed that we have a revision-lookup method. I'm not really sure what value it provides.

Here's an example call:

https://treeherder.mozilla.org/api/project/mozilla-inbound/revision-lookup/?revision=662d5b37ad5e

{
    "662d5b37ad5e": {
        "active_status": "active",
        "id": 16383,
        "push_timestamp": 1435181827,
        "revision": "662d5b37ad5e",
        "revision_hash": "6315828ab6051893bcd470530f40e20accd40324",
        "revision_id": 60683
    }
}

I don't see anything here which you can't get via the /resultset/ api (e.g. in the case https://treeherder.mozilla.org/api/project/mozilla-central/resultset/?revision=2694ff2ace6a), aside from active_status which doesn't sound particularly useful. Moreover, I see no references to this method in the treeherder UI. Should we just take it out, so it doesn't confuse people?

Comment 1

[Cameron Dawson [:camd]]

That works for me.  As long as the UI isn't using it, then I'd consider it superfluous.  If it's ever needed, we can add it back in at that time.

Comment 2

[William Lachance (:wlach)]

Suspecting moijes12 might find this interesting, it's more django/rest-framework stuff. 

moijes12: you can probably just delete your way to success with this one. It looks like the revision-lookup reference is in treeherder/webapp/api/urls.py and treeherder/webapp/api/revision.py (it looks like we can remove the latter altogether). There may also be some unit tests that test this endpoint: see if anything fails by running them after you've removed the code -- http://treeherder.readthedocs.org/en/latest/common_tasks.html#running-the-tests

Comment 3

[moijes]

wlach: It looks like there is more to this. 

We can remove the ViewSet "ResultSetViewSet" from the urls.py. But, doing causes the tests in tests/etl/test_common.py to fail. That's because they depend on this ViewSet. 
Now, the lookup_revision method in etl/common.py, which has this endpoint, is used in a couple of places. A grep led me to the below

(treeherder)moses@moses:~/treeherder/treeherder$ grep -i "lookup_revision" -nr ../tests/conftest.py:268:    monkeypatch.setattr(common, 'lookup_revisions', _get_resultset)
Binary file ./tests/etl/__pycache__/test_common.cpython-27-PYTEST.pyc matches
./tests/etl/test_common.py:15:    resultset = common.lookup_revisions({project: [revision]})
./tests/etl/test_common.py:25:    resultset = common.lookup_revisions({project: [revision]})
Binary file ./tests/conftest.pyc matches
./htmlcov/treeherder_etl_buildapi.html:660:<p id='t127' class='stm run hide_run'>&nbsp; &nbsp; &nbsp; &nbsp; <span class='nam'>revisions_lookup</span> <span class='op'>=</span> <span class='nam'>common</span><span class='op'>.</span><span class='nam'>lookup_revisions</span><span class='op'>(</span><span class='nam'>revisions</span><span class='op'>)</span><span class='strut'>&nbsp;</span></p>
./htmlcov/treeherder_etl_buildapi.html:819:<p id='t286' class='stm run hide_run'>&nbsp; &nbsp; &nbsp; &nbsp; <span class='nam'>revisions_lookup</span> <span class='op'>=</span> <span class='nam'>common</span><span class='op'>.</span><span class='nam'>lookup_revisions</span><span class='op'>(</span><span class='nam'>revision_dict</span><span class='op'>)</span><span class='strut'>&nbsp;</span></p>
./htmlcov/treeherder_etl_common.html:383:<p id='t92' class='stm run hide_run'><span class='key'>def</span> <span class='nam'>lookup_revisions</span><span class='op'>(</span><span class='nam'>revision_dict</span><span class='op'>)</span><span class='op'>:</span><span class='strut'>&nbsp;</span></p>
./treeherder/etl/buildapi.py:127:        revisions_lookup = common.lookup_revisions(revisions)
./treeherder/etl/buildapi.py:286:        revisions_lookup = common.lookup_revisions(revision_dict)
./treeherder/etl/common.py:92:def lookup_revisions(revision_dict):

What do you suggest ?

Comment 4

[William Lachance (:wlach)]

(In reply to moijes from comment #3)
> wlach: It looks like there is more to this. 
> 
> We can remove the ViewSet "ResultSetViewSet" from the urls.py. But, doing
> causes the tests in tests/etl/test_common.py to fail. That's because they
> depend on this ViewSet. 

I think you're trying to delete the wrong thing. We definitely don't want to remove ResultSetViewSet, just revision-lookup. That's this one:

https://github.com/mozilla/treeherder/blob/master/treeherder/webapp/api/urls.py#L53
https://github.com/mozilla/treeherder/blob/master/treeherder/webapp/api/revision.py#L12

Comment 5

[moijes]

Oops. Sorry. I meant RevisionLookupSetViewSet and not ResultSetViewSet

Comment 6

[William Lachance (:wlach)]

(In reply to moijes from comment #5)
> Oops. Sorry. I meant RevisionLookupSetViewSet and not ResultSetViewSet

Ah ok, I see what you mean. So I think in this case we should probably just call the internal treeherder method returned by the endpoint directly, and process its results. That is, modify `lookup_revisions` in webapp/api/common.py to call jm.get_revision_resultset_lookup(revision_list) directly, and process its return value.

Something like:

    with JobsModel(project) as jm:
         lookup_content = jm.get_revision_resultset_lookup(revision_set)

We could also modify the method to call the /resultset/ API, but I don't see the point. We're running locally so we might as well just do the simplest possible thing and access the jobsmodel directly.

Comment 7

[moijes]

Attachment: treeherder-PR790

Comment 8

[William Lachance (:wlach)]

Comment on attachment 8636122 [details] [review]
treeherder-PR790

Looks good!

Comment 9

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/14af4b5b83890fcadaf5337626d42fd464ccbd8d
Bug 1177257 - Remove the revision-lookup API

Comment 10

[William Lachance (:wlach)]

Merged, thanks!

Comment 11

[Bob Clary [:bc] (inactive)]

Please announce changes such as this so that external users can comment and prepare for the changes. Just because something doesn't appear to be used internally does not mean that it is not being used by someone else.

Comment 12

[William Lachance (:wlach)]

(In reply to Bob Clary [:bc:] from comment #11)
> Please announce changes such as this so that external users can comment and
> prepare for the changes. Just because something doesn't appear to be used
> internally does not mean that it is not being used by someone else.

Sorry about that, I'll post to mozilla.tools for future change proposals like this.

Comment 13

[Rail Aliiev [:rail]]

Hmmm, I have been using this method to report Funsize jobs to Treeherder... Is there any other way to get revision_hash except trying to reimplement the same algorithm?

Comment 14

[William Lachance (:wlach)]

(In reply to Rail Aliiev [:rail] from comment #13)
> Hmmm, I have been using this method to report Funsize jobs to Treeherder...
> Is there any other way to get revision_hash except trying to reimplement the
> same algorithm?

The resultset endpoint provides the same information:

https://treeherder.mozilla.org/api/project/mozilla-inbound/resultset/?revision=1a4960f6c56e

Let me know if you need me to temporarily revert this change, as I said earlier this should have been more broadly announced first (my fault).

Comment 15

[Rail Aliiev [:rail]]

https://treeherder.mozilla.org/api/project/mozilla-inbound/resultset/?revision=1a4960f6c56e should work fine for me, thanks for the tip.