1270643 - thclient's TreeherderJob.add_revision_hash() has been deprecated

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Geoff Brown [:gbrown] (pto Apr 11-18)]

In newer versions of thclient, TreeherderJob.add_revision_hash() is deprecated (possibly obsoleted?). We should update Autophone accordingly.

As it is, with thclient 2.1.0, I see:

root|INFO|creating Treeherder job a4ae56dc-ae16-4c31-82b3-5e2f6c2d8622 for autophone-s1s2 mozilla-central, revision_hash: 0a470caec614568a68bcda3b7c833b84723f45f0
Thread-2|thclient.client|WARNING|DEPRECATED: Use ``add_revision`` to add the 40 character top revision instead.

and subsequently:

root|ERROR|Error submitting request to Treeherder, attempt=1, last=2016-05-05T14:42:31.732180
Traceback (most recent call last):
  File "/home/gbrown/autophone2/autophonetreeherder.py", line 91, in post_request
    client.post_collection(project, job_collection)
  File "/home/gbrown/autophone2/_virtualenv/local/lib/python2.7/site-packages/thclient/client.py", line 926, in post_collection
    collection_inst.validate()
  File "/home/gbrown/autophone2/_virtualenv/local/lib/python2.7/site-packages/thclient/client.py", line 531, in validate
    d.validate()
  File "/home/gbrown/autophone2/_virtualenv/local/lib/python2.7/site-packages/thclient/client.py", line 62, in validate
    cb(prop.split('.'), required_properties[prop], prop)
  File "/home/gbrown/autophone2/_virtualenv/local/lib/python2.7/site-packages/thclient/client.py", line 117, in validate_existence
    raise TreeherderClientError(msg, [])
TreeherderClientError: TreeherderJob structure validation errors detected for property:revision
        Value not defined for revision

Comment 1

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Attachment: rename add_revision_hash to add_revision

This works for me.

Comment 2

[Geoff Brown [:gbrown] (pto Apr 11-18)]

I'm not sure if the treeherder-client required-version bump is necessary, but it seems like a good idea. 

2.1.0 is the current version and 2.0.1 is the immediately prior version: https://github.com/mozilla/treeherder/commit/fdb9a482b5cf243dd13b849b57de49a26b6e5ed3

Comment 3

[Bob Clary [:bc] (inactive)]

camd: is the argument to add_revision_hash and add_revision the same? We currently get the revision hash via: https://github.com/mozilla/autophone/blob/master/utils.py#L131

But it seems from the comment and the result of calls such as: https://treeherder.mozilla.org/api/project/mozilla-inbound/resultset/?revision=ac4a5896be44 that the actual 40 character revision is all you need and if we have it already we don't need to look it up?

Comment 4

[Cameron Dawson [:camd]]

Bob, that's correct.  Though you should, ideally, be using the 40 char revision.  But yeah, it's just the top revision for the push.  So no need to query treeherder to get the revision_hash any longer.

Comment 5

[Bob Clary [:bc] (inactive)]

Comment on attachment 8749419 [details] [diff] [review]
rename add_revision_hash to add_revision

camd: thanks!

gbrown: I believe the revision we get from the utils.get_build_data or from parsing the application.ini from the build is guaranteed to be 40 characters now, so I don't think we need to worry about looking it up.

I think we can just use the revision directly instead of looking it up via utils.get_treeherder_revision_hash. There are a bunch of places where revision_hash is used that we should change to use revision.

Comment 6

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Attachment: use revision instead of revision_hash

This eliminates revision_hash from everything except the database, where revision_hash will be stored as '' from now on.

I successfully submitted pending/running/complete to treeherder: https://treeherder.allizom.org/#/jobs?repo=mozilla-central&revision=043082cb7bd8490c60815f67fbd1f33323ad7663

Comment 7

[Bob Clary [:bc] (inactive)]

Comment on attachment 8750465 [details] [diff] [review]
use revision instead of revision_hash

Review of attachment 8750465 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with these changes. This appears to test well. I would like to see an updated patch though.

::: autophone.py
@@ +494,4 @@
>                  self.treeherder.submit_pending(phoneid,
>                                                 build_url,
>                                                 build_data['repo'],
> +                                               build_data['revision'][-40:],

Not thrilled with doing this in the caller. It probably wouldn't be clear to a newcomer that build_data['revision'] is actually the url to the changeset and that by taking the last 40 characters you are getting just the revision id from it. I think I would prefer to push this kind of thing down into the treeherder methods submit_* and to comment about what is going on. I'm less concerned that we may end up with a revision url that contains a revision id less than 40 characters but still have some misgivings there.

::: autophonetreeherder.py
@@ +127,5 @@
>  
>          :param machine: machine id
>          :param build_url: url to build being tested.
>          :param project: repository of build.
> +        :param revision: Treeherder revision of build.

The way this is written currently this is the changeset revision id but requires the callers to munge the changeset url to extract the revision id before calling. As I mentioned before, I'd rather not impose this tax on the callers. The AutophoneTreeherder methods should take a changeset url as argument and then extract the revision id from it.

We could use a helper function/method that took the revision as the changeset url and then extracted the revision id like

        # Extract the revision id from the changeset url.
        re_revision = re.compile(r'http.*/rev/(.*)')
        match = re_revision.match(revision)
        if match:
            revision = match.group(1)

This would handle both 40 character revision ids as well as shortened ones and also allow the caller to submit the revision id instead of the changeset url. Each of the submit_* methods could call this helper function to get the revision id that needs to be sent to Treeherder.

If we make this change, then a description like

:param revision: Either a URL to the changeset or the revision id.

would be more appropriate.

@@ +194,5 @@
>  
>          :param machine: machine id
>          :param build_url: url to build being tested.
>          :param project: repository of build.
> +        :param revision: Treeherder revision of build.

ditto.

@@ +252,5 @@
>  
>          :param machine: machine id
>          :param build_url: url to build being tested.
>          :param project: repository of build.
> +        :param revision: Treeherder revision of build.

ditto.

::: jobs.py
@@ +172,4 @@
>                  conn,
>                  'insert into jobs values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)',
>                  values=(None, now, None, build_url, build_id, changeset, tree,
> +                        revision, '', enable_unittests, attempts, device))

What is the benefit of leaving revision_hash even as an empty string?

::: phonetest.py
@@ +872,4 @@
>              self.phone.id,
>              self.build.url,
>              self.build.tree,
> +            self.build.revision[-40:],

ditto.

@@ +968,4 @@
>                  self.phone.id,
>                  self.build.url,
>                  self.build.tree,
> +                self.build.revision[-40:],

ditto.

::: worker.py
@@ +788,4 @@
>                  self.treeherder.submit_pending(self.phone.id,
>                                                 job['build_url'],
>                                                 job['tree'],
> +                                               job['revision'][-40:],

ditto.

@@ +985,4 @@
>                                      t.phone.id,
>                                      job['build_url'],
>                                      job['tree'],
> +                                    job['revision'][-40:],

ditto.

Comment 8

[Geoff Brown [:gbrown] (pto Apr 11-18)]

(In reply to Bob Clary [:bc:] from comment #7)
> Comment on attachment 8750465 [details] [diff] [review]
> use revision instead of revision_hash

> ::: autophone.py
> @@ +494,4 @@
> >                  self.treeherder.submit_pending(phoneid,
> >                                                 build_url,
> >                                                 build_data['repo'],
> > +                                               build_data['revision'][-40:],
> 
> Not thrilled with doing this in the caller. It probably wouldn't be clear to
> a newcomer that build_data['revision'] is actually the url to the changeset
> and that by taking the last 40 characters you are getting just the revision
> id from it. I think I would prefer to push this kind of thing down into the
> treeherder methods submit_* and to comment about what is going on.

Agreed. Updated in the next patch.

> ::: jobs.py
> @@ +172,4 @@
> >                  conn,
> >                  'insert into jobs values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)',
> >                  values=(None, now, None, build_url, build_id, changeset, tree,
> > +                        revision, '', enable_unittests, attempts, device))
> 
> What is the benefit of leaving revision_hash even as an empty string?

As discussed, no real benefit; just a necessary placeholder for the insert statement.

Comment 9

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Attachment: use revision instead of revision_hash

Stole your suggested code (thanks!), added some cautionary logging, and re-tested.

Comment 10

[Bob Clary [:bc] (inactive)]

Comment on attachment 8750941 [details] [diff] [review]
use revision instead of revision_hash

Review of attachment 8750941 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. I'll update the production servers when we push this.

Comment 11

[Geoff Brown [:gbrown] (pto Apr 11-18)]

https://github.com/mozilla/autophone/commit/a24ce1eae26a800a7bb4c37db1be9d8894fdfc83

Comment 12

[Bob Clary [:bc] (inactive)]

deployed 2016-05-12 14:00:00

upgraded treeherder client as well.