1079796 - Store the full revision SHA in addition to the shortened version & allow API requests using either

Status: RESOLVED FIXED

(Tree Management :: Treeherder: API, defect, P5)

Description

[Frederik Braun [:freddy]]

Compare results for these 2 links:
* https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=0fdbe5761ea3 
* https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=0fdbe5761ea3bff86826c94c833ddc83c92e083a

Both links should show results, but only the first does.
This seems to be a backend issue: I observed outbound requests using the DevTools network tab and saw that the revision id is sent correctly in both cases. But in the latter case, no response is coming back.

Comment 1

[Mauro Doglio [:mdoglio]]

Treeherder is storing short revisions only for now. A very easy fix would be to preprocess that parameter somewhere here https://github.com/mozilla/treeherder-service/blob/master/treeherder/webapp/api/resultset.py#L72-L98 and limit the number of characters to 12.

Comment 2

[Ed Morley [:emorley]]

Where was the URL in comment 0 generated, out of curiosity? :-)

Comment 3

[Ryan VanderMeulen [:RyanVM]]

I'm seeing Github PRs using them for Gaia-Try runs now. Which produces the above error. For that reason, I'm upping the severity on this.

Comment 4

[Frederik Braun [:freddy]]

I asked jhford if he could change his bot that submits gaia pull request to try: It usually provides tbpl links, but I asked if it could also point to Treeherder.
Considering that tbpl is going away at some point, I figured it could help getting people used to treeherder :)

I *think* he will fix this on his side, but I think this is odd behavior from treeherder, so I wanted this capture here too. :-)

Comment 5

[Ed Morley [:emorley]]

So the reason this occurs for Treeherder and not TBPL, is that:
* TBPL handles the pushlog entirely in the UI, by making a call to json-pushes with whatever SHA/branch name you've used for the rev param. The actual revision that's used for the TBPL backend call then comes from the short SHA returned by the json-pushes. The backend itself still only supports the short SHA. eg compare:
https://tbpl.mozilla.org/php/getRevisionBuilds.php?branch=gaia-try&rev=0fdbe5761ea3
https://tbpl.mozilla.org/php/getRevisionBuilds.php?branch=gaia-try&rev=0fdbe5761ea3bff86826c94c833ddc83c92e083a
* Treeherder ingests the pushlog in the backend only, and as Mauro said, only stores the first 12 characters of the revision hash:
https://github.com/mozilla/treeherder-service/blob/master/treeherder/etl/pushlog.py#L33

I was going to say that we should perhaps just say that people should use the short revision form when creating treeherder links, or that we should go with Mauro's suggestion in comment 1, however then noticed that things like buildapi cope with this:
https://secure.pub.build.mozilla.org/buildapi/self-serve/gaia-try/rev/0fdbe5761ea3bff86826c94c833ddc83c92e083a

Also, there's no guarantee that we won't need to use something longer than the 12 character form, if we start using a repo with enough commits that a collision could occur.

As such, what about:
1) Short term: change the gaia-try bot to output short SHA URLs (they are tidier anyway, and are what is used everywhere else)
2) Longer term: switch treeherder to storing the full 40 character revision for both git and mercurial repos, and then allowing partial matches when a short revision is passed in. (Or else, storing both the 12 char and full shas separately, and using the former if a 12 char revision passed, if that makes a big difference to DB perf)

Comment 6

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley] from comment #5)
> I was going to say that we should perhaps just say that 

Bah I need my morning caffeine

Comment 7

[Mauro Doglio [:mdoglio]]

+1 on storing both and do exact match.

Comment 8

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley] from comment #5)
> 1) Short term: change the gaia-try bot to output short SHA URLs (they are
> tidier anyway, and are what is used everywhere else)

This was done in:
https://github.com/jhford/try-server-hook/commit/940d1d1b17f38e290570690a9e65059717c8bcce

> 2) Longer term: switch treeherder to storing the full 40 character revision
> for both git and mercurial repos, and then allowing partial matches when a
> short revision is passed in. (Or else, storing both the 12 char and full
> shas separately, and using the former if a 12 char revision passed, if that
> makes a big difference to DB perf)

Morphing summary to be about this, though priority is low given #1 and that this will require schema changes etc.

Comment 9

[Ed Morley [:emorley]]

Looks like this is going to be done in bug 1199364 now.

Comment 11

[Ed Morley [:emorley]]

Hmm, actually let's make this depend on bug 1199364, since whilst that will get us storing the full SHA, we may then need to make API changes to use either short or long SHA, which we can do here if not already done.

Comment 13

[Cameron Dawson [:camd]]

Added a quick fix for the original description: Bug 1214039

The fix for that bug will be useful when we start storing both the long and short revisions.

Comment 14

[Cameron Dawson [:camd]]

Attachment: PR

Comment 15

[Cameron Dawson [:camd]]

Comment on attachment 8673288 [details] [review]
PR

Here's the long one, Ed.  Please let me know if you want to Vidyo while reviewing it to make it easier.  :)

Comment 16

[Ed Morley [:emorley]]

Comment on attachment 8673288 [details] [review]
PR

Have left some comments - I haven't tested this locally, and this was a quicker than I wanted review - but didn't want to not get to this before my day off tomorrow :-)

Comment 17

[Ed Morley [:emorley]]

Would it be possible to set some bug dependencies between this, bug 1214015, bug 1214039, bug 1199364 etc, so it's clearer which pieces depend on which? :-)

Comment 18

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/53bc948f940d9453a874031b892f9cc680657731
Bug 1079796 - store long and short revisions

We store both long and short, but only utilize the short (as before).  We
need to populate all the short and long revision records before we can
start using them.  So after this commit, we will begin backfilling the
old records that don't yet have those values populated.  Once they all
are, we can move to using the long_revision primarily in Bug 1199364.

Comment 19

[Cameron Dawson [:camd]]

(In reply to Ed Morley [:emorley] from comment #17)
> Would it be possible to set some bug dependencies between this, bug 1214015,
> bug 1214039, bug 1199364 etc, so it's clearer which pieces depend on which?
> :-)

Done.  However, bug 1214039, is actually orthogonal.  It does deal with revision lengths, but only uses what was stored before these changes.