1324570 - Autophone - fix issues with trigger runs by revision

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

I found an issue with trigger_runs where it was no longer possible to trigger debug builds by revision.

One issue was that the builds are performed by taskcluster and the task definition no longer has task_definition['extra']['build_type'] which resulted in the failure to detect debug builds.

Another issue was related to my mixing up whether json-pushes by default omits the first or last revision in a range. I had it backwards and json-pushes does not normally include the first revision.

One final issue was the fact that taskcluster routes using revisions use the full 40 byte revision id. This necessitated looking up the first revision if it was a 12 byte abbreviated revision id in order to obtain the full 40 byte revision id.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: bug-1324570-v1-trigger-runs.patch

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8820045 [details] [diff] [review]
bug-1324570-v1-trigger-runs.patch

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

I had to give an r-, a few small nits/questions- but overall I like what this is doing.

::: builds.py
@@ +517,5 @@
> +            if len(start_revision) == 12:
> +                parameters = {'changeset': start_revision}
> +                revisions = get_push_revisions(repo, parameters)
> +                if len(revisions) == 1:
> +                    start_revision = revisions[0]

it would be nice to give a warning/error if we had len(revisions) != 1.

@@ +518,5 @@
> +                parameters = {'changeset': start_revision}
> +                revisions = get_push_revisions(repo, parameters)
> +                if len(revisions) == 1:
> +                    start_revision = revisions[0]
> +            if not end_revision.startswith(start_revision):

wouldn't we want:
if not end_revision == start_revision:

this would assume both are the same 40 character revision id.

@@ +612,5 @@
> +                            LOGGER.debug('_find_task_ids_by_revisions: '
> +                                         'using task_definition["workerType"] and '
> +                                         'task_definition["extra"]["build_type"]')
> +                            platform = task_definition['workerType']
> +                            build_type = task_definition['extra']['build_type']

for this block of code, could you include an example of what metadata:name is that we are working with.  I know it from memory, but I might not 8 months from now when I could be hacking on this code.

Comment 3

[Bob Clary [:bc] (inactive)]

(In reply to Joel Maher ( :jmaher) from comment #2)
> Comment on attachment 8820045 [details] [diff] [review]
> bug-1324570-v1-trigger-runs.patch
> 
> Review of attachment 8820045 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I had to give an r-, a few small nits/questions- but overall I like what
> this is doing.
> 
> ::: builds.py
> @@ +517,5 @@
> > +            if len(start_revision) == 12:
> > +                parameters = {'changeset': start_revision}
> > +                revisions = get_push_revisions(repo, parameters)
> > +                if len(revisions) == 1:
> > +                    start_revision = revisions[0]
> 
> it would be nice to give a warning/error if we had len(revisions) != 1.
> 

sure. We could also just fail and return an empty list of revisions for the repo. That might make more sense if the start_revision isn't found. See below as well.

> @@ +518,5 @@
> > +                parameters = {'changeset': start_revision}
> > +                revisions = get_push_revisions(repo, parameters)
> > +                if len(revisions) == 1:
> > +                    start_revision = revisions[0]
> > +            if not end_revision.startswith(start_revision):
> 
> wouldn't we want:
> if not end_revision == start_revision:
> 
> this would assume both are the same 40 character revision id.
> 

If we don't treat the failure to get the start_revision converted to 40 bytes as a failure, start_revision could still be the 12 character version the way things are now. That would mean we would have to use startswith.

Perhaps it would be better overall to just treat the failure to convert the 12 byte to 40 byte revision id as a real failure and set the list of revisions for that repo to the empty list? This would allow us to check if start_revision and end_revision are equal and save the call to get_push_revisions() when we have a bad revision id.

I think I'll do that.

> @@ +612,5 @@
> > +                            LOGGER.debug('_find_task_ids_by_revisions: '
> > +                                         'using task_definition["workerType"] and '
> > +                                         'task_definition["extra"]["build_type"]')
> > +                            platform = task_definition['workerType']
> > +                            build_type = task_definition['extra']['build_type']
> 
> for this block of code, could you include an example of what metadata:name
> is that we are working with.  I know it from memory, but I might not 8
> months from now when I could be hacking on this code.

sure.

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: bug-1324570-v2-trigger-runs.patch

Thanks for the review. I opted for always checking the start revision and simplifying things a bit I think. Found latent identation bug during testing.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8820337 [details] [diff] [review]
bug-1324570-v2-trigger-runs.patch

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

thanks :bc

Comment 6

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/43dca9321089f977f9025e504ed84e2cff6c0075

deployed 2016-12-21 08:00