staticanalysis/bot: Load diff_phid from try task as early as possible (…

…#1960)

src/staticanalysis/bot/static_analysis_bot/cli.py

@@ -17,6 +17,8 @@
 from static_analysis_bot import AnalysisException
 from static_analysis_bot import config
 from static_analysis_bot import stats
+from static_analysis_bot.config import SOURCE_PHABRICATOR
+from static_analysis_bot.config import SOURCE_TRY
 from static_analysis_bot.config import settings
 from static_analysis_bot.report import get_reporters
 from static_analysis_bot.revisions import PhabricatorRevision
@@ -31,10 +33,6 @@
     '--id',
     envvar='ANALYSIS_ID',
 )
-@click.option(
-    '--task-id',
-    envvar='TASK_ID',
-)
 @click.option(
     '--work-dir',
     default=os.path.join(
@@ -45,7 +43,6 @@
 )
 @stats.api.timer('runtime.analysis')
 def main(id,
-         task_id,
          work_dir,
          taskcluster_secret,
          taskcluster_client_id,
@@ -123,7 +120,12 @@ def main(id,
         reporters['phabricator'].setup_api(phabricator_api)
 
     # Load unique revision
-    revision = PhabricatorRevision(id, phabricator_api)
+    if settings.source == SOURCE_PHABRICATOR:
+        revision = PhabricatorRevision(phabricator_api, diff_phid=id)
+    elif settings.source == SOURCE_TRY:
+        revision = PhabricatorRevision(phabricator_api, try_task=queue_service.task(settings.try_task_id))
+    else:
+        raise Exception('Unsupported source {}'.format(settings.source))
 
     # Run workflow according to source
     w = Workflow(reporters, secrets['ANALYZERS'], index_service, queue_service, phabricator_api)

src/staticanalysis/bot/static_analysis_bot/revisions.py

@@ -5,7 +5,6 @@
 
 import io
 import os
-import re
 from datetime import timedelta
 
 import hglib
@@ -188,20 +187,30 @@ class PhabricatorRevision(Revision):
     '''
     A phabricator revision to process
     '''
-    regex = re.compile(r'^(PHID-DIFF-(?:\w+))$')
+    diff_phid = None
 
-    def __init__(self, description, api):
+    def __init__(self, api, diff_phid=None, try_task=None):
         super().__init__()
         assert isinstance(api, PhabricatorAPI)
+        assert (diff_phid is not None) ^ (try_task is not None)
         self.api = api
         self.mercurial_revision = None
 
-        # Parse Diff description
-        match = self.regex.match(description)
-        if match is None:
-            raise Exception('Invalid Phabricator description')
-        groups = match.groups()
-        self.diff_phid = groups[0]
+        if diff_phid is not None:
+            # Load directly from the diff phid
+            self.load_phabricator(diff_phid)
+        elif try_task is not None:
+            # Load diff phid from the task env
+            self.load_phabricator(try_task['extra']['code-review']['phabricator-diff'])
+        else:
+            raise Exception('Invalid revision configuration')
+
+    def load_phabricator(self, diff_phid):
+        '''
+        Load identifiers from Phabricator
+        '''
+        assert diff_phid.startswith('PHID-DIFF-')
+        self.diff_phid = diff_phid
 
         # Load diff details to get the diff revision
         diffs = self.api.search_diffs(diff_phid=self.diff_phid)

src/staticanalysis/bot/tests/conftest.py

@@ -266,7 +266,7 @@ def mock_revision(mock_phabricator):
     '''
     from static_analysis_bot.revisions import PhabricatorRevision
     with mock_phabricator as api:
-        return PhabricatorRevision('PHID-DIFF-XXX', api)
+        return PhabricatorRevision(api, diff_phid='PHID-DIFF-XXX')
 
 
 @pytest.fixture

src/staticanalysis/bot/tests/test_reporter_debug.py

@@ -21,7 +21,7 @@ def test_publication(tmpdir, mock_issues, mock_phabricator):
     assert not os.path.exists(report_path)
 
     with mock_phabricator as api:
-        prev = PhabricatorRevision('PHID-DIFF-abcdef', api)
+        prev = PhabricatorRevision(api, 'PHID-DIFF-abcdef')
 
     r = DebugReporter(report_dir)
     r.publish(mock_issues, prev)

src/staticanalysis/bot/tests/test_reporter_mail.py

@@ -105,7 +105,7 @@ def _check_email(request):
     r = MailReporter(conf, 'test_tc', 'token_tc')
 
     with mock_phabricator as api:
-        prev = PhabricatorRevision('PHID-DIFF-test', api)
+        prev = PhabricatorRevision(api, 'PHID-DIFF-test')
         prev.improvement_patches = [
             ImprovementPatch('clang-tidy', repr(prev), 'Some code fixes'),
             ImprovementPatch('clang-format', repr(prev), 'Some lint fixes'),

src/staticanalysis/bot/tests/test_reporter_phabricator.py

@@ -82,7 +82,7 @@ def _check_comment(request):
     )
 
     with mock_phabricator as api:
-        revision = PhabricatorRevision('PHID-DIFF-abcdef', api)
+        revision = PhabricatorRevision(api, 'PHID-DIFF-abcdef')
         revision.lines = {
             # Add dummy lines diff
             'test.cpp': [41, 42, 43],
@@ -135,7 +135,7 @@ def _check_comment(request):
     )
 
     with mock_phabricator as api:
-        revision = PhabricatorRevision('PHID-DIFF-abcdef', api)
+        revision = PhabricatorRevision(api, 'PHID-DIFF-abcdef')
         revision.lines = {
             # Add dummy lines diff
             'test.cpp': [41, 42, 43],
@@ -193,7 +193,7 @@ def _check_comment(request):
     )
 
     with mock_phabricator as api:
-        revision = PhabricatorRevision('PHID-DIFF-abcdef', api)
+        revision = PhabricatorRevision(api, 'PHID-DIFF-abcdef')
         revision.lines = {
             # Add dummy lines diff
             'test.txt': [0],
@@ -274,7 +274,7 @@ def _check_comment_ccov(request):
     )
 
     with mock_phabricator as api:
-        revision = PhabricatorRevision('PHID-DIFF-abcdef', api)
+        revision = PhabricatorRevision(api, 'PHID-DIFF-abcdef')
         revision.lines = {
             # Add dummy lines diff
             'test.txt': [0],
@@ -326,7 +326,7 @@ def test_phabricator_analyzers(mock_config, mock_repository, mock_phabricator):
 
     def _test_reporter(api, analyzers):
         # Always use the same setup, only varies the analyzers
-        revision = PhabricatorRevision('PHID-DIFF-abcdef', api)
+        revision = PhabricatorRevision(api, 'PHID-DIFF-abcdef')
         revision.lines = {
             'test.cpp': [0, 41, 42, 43],
             'dom/test.cpp': [42, ],

src/staticanalysis/bot/tests/test_revisions.py

@@ -17,7 +17,7 @@ def test_phabricator(mock_phabricator, mock_repository, mock_config):
     from static_analysis_bot.revisions import PhabricatorRevision
 
     with mock_phabricator as api:
-        r = PhabricatorRevision('PHID-DIFF-testABcd12', api)
+        r = PhabricatorRevision(api, 'PHID-DIFF-testABcd12')
     assert not hasattr(r, 'mercurial')
     assert r.diff_id == 42
     assert r.diff_phid == 'PHID-DIFF-testABcd12'