Merge lp:~roadmr/isitdeployable/revision-disappeared-ohnoes into lp:isitdeployable

[Daniel Manrique]

If self.branch.revparse_single(revspec) Keyerrors, it means the revspec we wanted doesn't exist in the source tree anymore - return None

This happened because the review-tools branch had its history rewritten, so a hash referenced in sca r5536 no longer exists; and we can't change sca history itself just to erase this fact; we noticed this when we moved sca's main trunk to git, so it's trying to re-scan all revisions and refetch dependency branches to update the db.

[2019-07-22 20:57:25,119: WARNING/Worker-13] Getting branch git+ssh://git.launchpad.net/review-tools
[2019-07-22 20:57:25,525: WARNING/Worker-13] Got branch
[2019-07-22 20:57:25,530: ERROR/MainProcess] Task revtracker.tasks.update_revisions_task[5cf03caa-51eb-40f2-827f-549dab189367] raised unexpected: KeyError('e8d7f8e7',)
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/celery/app/trace.py", line 240, in trace_task
    R = retval = fun(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/celery/app/trace.py", line 438, in __protected_call__
    return self.run(*args, **kwargs)
  File "/usr/src/app/revtracker/tasks.py", line 735, in update_revisions_task
    update_revisions(project_id)
  File "/usr/src/app/revtracker/tasks.py", line 717, in update_revisions
    fetched_branches=fetched_branches)
  File "/usr/src/app/revtracker/tasks.py", line 660, in get_revisions
    parent_branch_url, fetched_branches=fetched_branches)
  File "/usr/src/app/revtracker/tasks.py", line 606, in sourcedeps_diff_revisions
    current_revision, fetched_branches=fetched_branches)
  File "/usr/src/app/revtracker/tasks.py", line 578, in sourcedeps_diff
    new_revision = branch.revspec_to_revision(new_revspec)
  File "/usr/src/app/revtracker/tasks.py", line 246, in revspec_to_revision
    revid = self.branch.revparse_single(revspec).id.hex
KeyError: 'e8d7f8e7'

It looks like the code in sourcedeps_diff already handles the case of revspec_to_revision returning None for a non-found revspec.

[Maximiliano Bertacchini (maxiberta)]

+1

review: Approve

[Otto Co-Pilot (otto-copilot)]

Running landing tests failed
https://jenkins.ols.canonical.com/online-services/job/isitdeployable-docker/61/

[Otto Co-Pilot (otto-copilot)]

Running landing tests failed
https://jenkins.ols.canonical.com/online-services/job/isitdeployable-docker/62/

[Jonathan Hartley (tartley)]

The modified functon is also called here:

https://bazaar.launchpad.net/~roadmr/isitdeployable/revision-disappeared-ohnoes/view/head:/revtracker/tasks.py#L264

and the return value from that is used in iter_revisions(), here:

https://bazaar.launchpad.net/~roadmr/isitdeployable/revision-disappeared-ohnoes/view/head:/revtracker/tasks.py#L352

If our modification had returned None, we could guard the access to tip_revision.revno with:

        tip_revision = self.tip_revision()
+ if not tip_revision:
+ return
        if (start_revision is not None and
                tip_revision.revno <= start_revision.revno):
            return

It seems like if this were to occur, then iter_revisions has been asked to iterate over a range starting with a tip that doesn't exist, so returning without yielding anything, an empty generator, seems like the right thing to do maybe?

[Otto Co-Pilot (otto-copilot)]

Running landing tests failed
https://jenkins.ols.canonical.com/online-services/job/isitdeployable-docker/64/

[Jonathan Hartley (tartley)]

I guess I meant:

  tip_revision = self.tip_revision()
  if (start_revision is not None and
+ tip_revision is not None and
      tip_revision.revno <= start_revision.revno):
          return

[Jonathan Hartley (tartley)]

Do I need to put code in markup to prevent whitespace mangling? Apologies for experimenting on your MP.

```
  tip_revision = self.tip_revision()
  if (start_revision is not None and
+ tip_revision is not None and
      tip_revision.revno <= start_revision.revno):
          return
```

[Daniel Manrique (roadmr)]

I don't think markup is supported here :/

I think I got it, please check latest set of changes :)

[lp:~roadmr/isitdeployable/revision-disappeared-ohnoes]

297. By Daniel Manrique on 2019-07-23

revert nonsensical

298. By Daniel Manrique on 2019-07-23

This makes more sense

=== modified file 'revtracker/tasks.py'
--- revtracker/tasks.py	2018-10-11 16:10:43 +0000
+++ revtracker/tasks.py	2019-07-23 13:24:12 +0000
@@ -243,7 +243,10 @@
             revid = RevisionSpec.from_string(revspec).as_revision_id(
                     self.branch)
         else:
-            revid = self.branch.revparse_single(revspec).id.hex
+            try:
+                revid = self.branch.revparse_single(revspec).id.hex
+            except KeyError:
+                return None
         return self.get_revision(revid)
 
     def null_revision(self):
@@ -348,6 +351,7 @@
                        tip_only=False):
         tip_revision = self.tip_revision()
         if (start_revision is not None and
+                tip_revision is not None and
                 tip_revision.revno <= start_revision.revno):
             return
         if start_revision is None: