From 706433fd3438c1176e5319208d60d711d96117dd Mon Sep 17 00:00:00 2001
From: Tyler Roscoe <troscoe@yelp.com>
Date: Thu, 5 Nov 2015 21:45:34 -0800
Subject: [PATCH 1/9] Start refactoring lookup_chronos_job

Got rid of regex, max_expected. Introduce service, instance.
All the unit tests pass, which means all callers are being honest and
mocking correctly :)
---
 paasta_tools/chronos_tools.py | 20 ++--------
 tests/test_chronos_tools.py   | 75 ++++++++++++-----------------------
 2 files changed, 30 insertions(+), 65 deletions(-)

diff --git a/paasta_tools/chronos_tools.py b/paasta_tools/chronos_tools.py
index e29d22f621..b562eb3314 100644
--- a/paasta_tools/chronos_tools.py
+++ b/paasta_tools/chronos_tools.py
@@ -555,34 +555,22 @@ def match_job_names_to_service_instance(service, instance, jobs):
     return matching
 
 
-def lookup_chronos_jobs(pattern, client, max_expected=None, include_disabled=False):
+def lookup_chronos_jobs(service, instance, client, include_disabled=False):
     """Retrieves Chronos jobs with names that match a specified pattern.
 
-    :param pattern: a Python style regular expression that the job name will be matched against
-                    (after being passed to re.compile)
+    :param service: service we're looking for
+    :param instance: instance we're looking for
     :param client: Chronos client object
-    :param max_expected: maximum number of results that is expected. If exceeded, raises a ValueError.
-                         If unspecified, defaults to no limit.
     :param include_disabled: boolean indicating if disabled jobs should be included in matches
     """
-    try:
-        regexp = re.compile(pattern)
-    except re.error:
-        raise ValueError("Invalid regex pattern '%s'" % pattern)
     jobs = client.list()
     matching_jobs = []
     for job in jobs:
-        if regexp.search(job['name']):
+        if job['name'].startswith(compose_job_id(service, instance)):
             if job['disabled'] and not include_disabled:
                 continue
             else:
                 matching_jobs.append(job)
-
-    if max_expected and len(matching_jobs) > max_expected:
-        matching_ids = [job['name'] for job in matching_jobs]
-        raise ValueError("Found %d jobs for pattern '%s', but max_expected is set to %d (ids: %s)" %
-                         (len(matching_jobs), pattern, max_expected, ', '.join(matching_ids)))
-
     return matching_jobs
 
 
diff --git a/tests/test_chronos_tools.py b/tests/test_chronos_tools.py
index b32c2e985e..7a4db63261 100644
--- a/tests/test_chronos_tools.py
+++ b/tests/test_chronos_tools.py
@@ -694,65 +694,42 @@ def test_get_chronos_jobs_for_cluster(self):
             list_jobs_patch.assert_any_call('dir2', cluster, soa_dir)
             assert list_jobs_patch.call_count == 2
 
-    def test_lookup_chronos_jobs_ok(self):
+    def test_lookup_chronos_jobs_with_service_and_instance(self):
         fake_service = 'fake_service'
+        fake_instance = 'fake_instance'
         fake_jobs = [
             {
-                'name': fake_service,
-                'disabled': False
+                'name': chronos_tools.compose_job_id(
+                    fake_service,
+                    fake_instance,
+                    'git1111',
+                    'config1111',
+                ),
+                'disabled': False,
             },
             {
-                'name': 'fake_other_service',
-                'disabled': False
-            }
-        ]
-        fake_client = mock.Mock(list=mock.Mock(return_value=fake_jobs))
-        actual = chronos_tools.lookup_chronos_jobs(r'^%s$' % fake_service, fake_client)
-        expected = [fake_jobs[0]]
-        assert actual == expected
-
-    def test_lookup_chronos_jobs_invalid(self):
-        fake_regex = 'fake)badregex'
-        fake_jobs = []
-        fake_client = mock.Mock(list=mock.Mock(return_value=fake_jobs))
-        with raises(ValueError) as excinfo:
-            chronos_tools.lookup_chronos_jobs(r'%s' % fake_regex, fake_client, max_expected=1)
-        assert "Invalid regex pattern '%s'" % fake_regex in excinfo.value
-
-    def test_lookup_chronos_jobs_max_expected(self):
-        fake_service = 'fake_service'
-        fake_jobs = [
-            {
-                'name': fake_service,
-                'disabled': False
+                'name': chronos_tools.compose_job_id(
+                    fake_service,
+                    fake_instance,
+                    'git2222',
+                    'config2222',
+                ),
+                'disabled': False,
             },
             {
-                'name': fake_service,
-                'disabled': False
-            }
-        ]
-        fake_client = mock.Mock(list=mock.Mock(return_value=fake_jobs))
-        with raises(ValueError) as excinfo:
-            chronos_tools.lookup_chronos_jobs(r'^%s$' % fake_service, fake_client, max_expected=1)
-        assert "Found 2 jobs for pattern '^%s$', but max_expected is set to 1 (ids: %s, %s)" % (
-            fake_service, fake_service, fake_service) in excinfo.value
-
-    def test_lookup_chronos_jobs_disabled(self):
-        fake_service = 'fake_service'
-        fake_jobs = [
-            {
-                'name': fake_service,
-                'disabled': True
+                'name': chronos_tools.compose_job_id(
+                    'some_other_service',
+                    'some_other_instance',
+                    'git3333',
+                    'config3333',
+                ),
+                'disabled': False,
             },
-            {
-                'name': fake_service,
-                'disabled': False
-            }
         ]
         fake_client = mock.Mock(list=mock.Mock(return_value=fake_jobs))
-        actual = chronos_tools.lookup_chronos_jobs(r'^%s$' % fake_service, fake_client, include_disabled=False)
-        expected = [fake_jobs[1]]
-        assert actual == expected
+        expected = [fake_jobs[0], fake_jobs[1]]
+        actual = chronos_tools.lookup_chronos_jobs(fake_service, fake_instance, fake_client)
+        assert sorted(actual) == sorted(expected)
 
     def test_create_complete_config(self):
         fake_owner = 'test_team'

From 2cef800e4ada21fa0fe94752111122617bde5323 Mon Sep 17 00:00:00 2001
From: Tyler Roscoe <troscoe@yelp.com>
Date: Thu, 5 Nov 2015 22:00:06 -0800
Subject: [PATCH 2/9] decompose the incoming job_id rather than composing our
 arguments.

I decided this is going to be better when I handle git_hash and config_hash, which is next.
---
 paasta_tools/chronos_tools.py |  7 ++++++-
 tests/test_chronos_tools.py   | 21 +++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/paasta_tools/chronos_tools.py b/paasta_tools/chronos_tools.py
index b562eb3314..73d173525a 100644
--- a/paasta_tools/chronos_tools.py
+++ b/paasta_tools/chronos_tools.py
@@ -34,6 +34,7 @@
 from paasta_tools.utils import get_default_branch
 from paasta_tools.utils import get_docker_url
 from paasta_tools.utils import InstanceConfig
+from paasta_tools.utils import InvalidJobNameError
 from paasta_tools.utils import load_deployments_json
 from paasta_tools.utils import load_system_paasta_config
 from paasta_tools.utils import PATH_TO_SYSTEM_PAASTA_CONFIG_DIR
@@ -566,7 +567,11 @@ def lookup_chronos_jobs(service, instance, client, include_disabled=False):
     jobs = client.list()
     matching_jobs = []
     for job in jobs:
-        if job['name'].startswith(compose_job_id(service, instance)):
+        try:
+            (job_service, job_instance, _, __) = decompose_job_id(job['name'])
+        except InvalidJobNameError:
+            continue
+        if job_service == service and job_instance == instance:
             if job['disabled'] and not include_disabled:
                 continue
             else:
diff --git a/tests/test_chronos_tools.py b/tests/test_chronos_tools.py
index 7a4db63261..682889b335 100644
--- a/tests/test_chronos_tools.py
+++ b/tests/test_chronos_tools.py
@@ -716,6 +716,15 @@ def test_lookup_chronos_jobs_with_service_and_instance(self):
                 ),
                 'disabled': False,
             },
+            {
+                'name': chronos_tools.compose_job_id(
+                    fake_service,
+                    fake_instance,
+                    'gitdisabled',
+                    'configdisabled',
+                ),
+                'disabled': True,
+            },
             {
                 'name': chronos_tools.compose_job_id(
                     'some_other_service',
@@ -731,6 +740,18 @@ def test_lookup_chronos_jobs_with_service_and_instance(self):
         actual = chronos_tools.lookup_chronos_jobs(fake_service, fake_instance, fake_client)
         assert sorted(actual) == sorted(expected)
 
+    def test_lookup_chronos_jobs_skips_non_paasta_job_id(self):
+        fake_jobs = [
+            {
+                'name': 'some non-paasta job',
+                'disabled': False,
+            },
+        ]
+        fake_client = mock.Mock(list=mock.Mock(return_value=fake_jobs))
+        actual = chronos_tools.lookup_chronos_jobs('whatever', 'whatever', fake_client)
+        # The main thing here is that InvalidJobNameError is not raised.
+        assert actual == []
+
     def test_create_complete_config(self):
         fake_owner = 'test_team'
         with contextlib.nested(

From 6cb26bc4bc38792c093cbdbc4588cc8c9afbafe0 Mon Sep 17 00:00:00 2001
From: Tyler Roscoe <troscoe@yelp.com>
Date: Thu, 5 Nov 2015 22:08:32 -0800
Subject: [PATCH 3/9] Test or disabled behavior

---
 tests/test_chronos_tools.py | 57 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/tests/test_chronos_tools.py b/tests/test_chronos_tools.py
index 682889b335..606ec3cd42 100644
--- a/tests/test_chronos_tools.py
+++ b/tests/test_chronos_tools.py
@@ -737,7 +737,62 @@ def test_lookup_chronos_jobs_with_service_and_instance(self):
         ]
         fake_client = mock.Mock(list=mock.Mock(return_value=fake_jobs))
         expected = [fake_jobs[0], fake_jobs[1]]
-        actual = chronos_tools.lookup_chronos_jobs(fake_service, fake_instance, fake_client)
+        actual = chronos_tools.lookup_chronos_jobs(
+            service=fake_service,
+            instance=fake_instance,
+            client=fake_client,
+        )
+        assert sorted(actual) == sorted(expected)
+
+    def test_lookup_chronos_jobs_include_disabled(self):
+        fake_service = 'fake_service'
+        fake_instance = 'fake_instance'
+        fake_jobs = [
+            {
+                'name': chronos_tools.compose_job_id(
+                    fake_service,
+                    fake_instance,
+                    'git1111',
+                    'config1111',
+                ),
+                'disabled': False,
+            },
+            {
+                'name': chronos_tools.compose_job_id(
+                    fake_service,
+                    fake_instance,
+                    'git2222',
+                    'config2222',
+                ),
+                'disabled': False,
+            },
+            {
+                'name': chronos_tools.compose_job_id(
+                    fake_service,
+                    fake_instance,
+                    'gitdisabled',
+                    'configdisabled',
+                ),
+                'disabled': True,
+            },
+            {
+                'name': chronos_tools.compose_job_id(
+                    'some_other_service',
+                    'some_other_instance',
+                    'git3333',
+                    'config3333',
+                ),
+                'disabled': False,
+            },
+        ]
+        fake_client = mock.Mock(list=mock.Mock(return_value=fake_jobs))
+        expected = [fake_jobs[0], fake_jobs[1], fake_jobs[2]]
+        actual = chronos_tools.lookup_chronos_jobs(
+            service=fake_service,
+            instance=fake_instance,
+            client=fake_client,
+            include_disabled=True,
+        )
         assert sorted(actual) == sorted(expected)
 
     def test_lookup_chronos_jobs_skips_non_paasta_job_id(self):

From 6aa193fd4e68e7d4afde21a1e5766f9bfa311481 Mon Sep 17 00:00:00 2001
From: Tyler Roscoe <troscoe@yelp.com>
Date: Thu, 5 Nov 2015 22:26:59 -0800
Subject: [PATCH 4/9] Support limiting by git_hash and config_hash

note1: I decided not to bother with the [de]compose_job_id requirement
that you either specify both git_hash and config_hash or you specify
neither. I don't thnk this function needs it and it didn't make
the implementation any more complicated.

note2: Technically my test is insufficient since it doesn't differentiate
matching just git_hash vs matching just config_hash. I don't think the
extra test granularity adds any value so I'm not going to bother. (I did
comment out both sides of the git_hash/config_hash and block to manually
verify that both sides work.)
---
 paasta_tools/chronos_tools.py | 21 +++++++++++---
 tests/test_chronos_tools.py   | 54 +++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/paasta_tools/chronos_tools.py b/paasta_tools/chronos_tools.py
index 73d173525a..2f7c34a857 100644
--- a/paasta_tools/chronos_tools.py
+++ b/paasta_tools/chronos_tools.py
@@ -556,22 +556,35 @@ def match_job_names_to_service_instance(service, instance, jobs):
     return matching
 
 
-def lookup_chronos_jobs(service, instance, client, include_disabled=False):
+def lookup_chronos_jobs(service, instance, client, git_hash=None, config_hash=None, include_disabled=False):
     """Retrieves Chronos jobs with names that match a specified pattern.
 
     :param service: service we're looking for
     :param instance: instance we're looking for
     :param client: Chronos client object
-    :param include_disabled: boolean indicating if disabled jobs should be included in matches
+    :param git_hash: git_hash we're looking for. Optional; return jobs with any
+    git_hash if omitted.
+    :param config_hash: config_hash we're looking for. Optional; return jobs
+    with any git_hash if omitted.
+    :param include_disabled: boolean indicating if disabled jobs should be
+    included in the returned list
+    :returns: list of job dicts whose name matches the ``service`` and
+    ``instance`` (and maybe ``git_hash`` and ``config_hash``) arguments
+    provided
     """
     jobs = client.list()
     matching_jobs = []
     for job in jobs:
         try:
-            (job_service, job_instance, _, __) = decompose_job_id(job['name'])
+            (job_service, job_instance, job_git_hash, job_config_hash) = decompose_job_id(job['name'])
         except InvalidJobNameError:
             continue
-        if job_service == service and job_instance == instance:
+        if (
+            job_service == service
+            and job_instance == instance
+            and (git_hash is None or job_git_hash == git_hash)
+            and (config_hash is None or job_config_hash == config_hash)
+        ):
             if job['disabled'] and not include_disabled:
                 continue
             else:
diff --git a/tests/test_chronos_tools.py b/tests/test_chronos_tools.py
index 606ec3cd42..aab8c0ae20 100644
--- a/tests/test_chronos_tools.py
+++ b/tests/test_chronos_tools.py
@@ -795,6 +795,60 @@ def test_lookup_chronos_jobs_include_disabled(self):
         )
         assert sorted(actual) == sorted(expected)
 
+    def test_lookup_chronos_jobs_with_everything_specified(self):
+        fake_service = 'fake_service'
+        fake_instance = 'fake_instance'
+        fake_git_hash = 'fake_git_hash'
+        fake_config_hash = 'fake_config_hash'
+        fake_jobs = [
+            {
+                'name': chronos_tools.compose_job_id(
+                    fake_service,
+                    fake_instance,
+                    fake_git_hash,
+                    fake_config_hash,
+                ),
+                'disabled': False,
+            },
+            {
+                'name': chronos_tools.compose_job_id(
+                    fake_service,
+                    fake_instance,
+                    'git2222',
+                    'config2222',
+                ),
+                'disabled': False,
+            },
+            {
+                'name': chronos_tools.compose_job_id(
+                    fake_service,
+                    fake_instance,
+                    'gitdisabled',
+                    'configdisabled',
+                ),
+                'disabled': True,
+            },
+            {
+                'name': chronos_tools.compose_job_id(
+                    'some_other_service',
+                    'some_other_instance',
+                    'git3333',
+                    'config3333',
+                ),
+                'disabled': False,
+            },
+        ]
+        fake_client = mock.Mock(list=mock.Mock(return_value=fake_jobs))
+        expected = [fake_jobs[0]]
+        actual = chronos_tools.lookup_chronos_jobs(
+            service=fake_service,
+            instance=fake_instance,
+            git_hash=fake_git_hash,
+            config_hash=fake_config_hash,
+            client=fake_client,
+        )
+        assert sorted(actual) == sorted(expected)
+
     def test_lookup_chronos_jobs_skips_non_paasta_job_id(self):
         fake_jobs = [
             {

From 9144148dd2d5cb00ba26da0fac1d2ffb09223988 Mon Sep 17 00:00:00 2001
From: Tyler Roscoe <troscoe@yelp.com>
Date: Thu, 5 Nov 2015 22:50:48 -0800
Subject: [PATCH 5/9] Delete get_matching_jobs. Replace with calls to newly
 improved lookup_chronos_jobs.

This is a long-delayed repsonse to kwa and robj comments in
review:117855. In my analysis, the underlying theme in their complaints
is that lookup_chronos_job is designed wrong. Correcting this malady is
what has motivated my last several refactoring branches: making
[de]compose_job_id more sensible, reducing the number of different ways
to accomplish various common job_id transformations, etc.
---
 paasta_tools/chronos_serviceinit.py | 60 +++++++++++++----------------
 tests/test_chronos_serviceinit.py   | 18 ---------
 2 files changed, 27 insertions(+), 51 deletions(-)

diff --git a/paasta_tools/chronos_serviceinit.py b/paasta_tools/chronos_serviceinit.py
index 0903a036f1..2eca5d4da8 100755
--- a/paasta_tools/chronos_serviceinit.py
+++ b/paasta_tools/chronos_serviceinit.py
@@ -73,32 +73,6 @@ def restart_chronos_job(service, instance, job_id, client, cluster, matching_job
     start_chronos_job(service, instance, job_id, client, cluster, job_config, emergency)
 
 
-def get_matching_jobs(client, job_id, all_tags):
-    """Use Chronos client `client` to get a list of configured Chronos jobs
-    related to `job_id`, the full name of the job as calculated by
-    create_complete_config().
-
-    If all_tags is False, fetch only the exact job specified by job_id.
-
-    If all_tags is True, fetch all jobs including those with different git and
-    config hashes (i.e. older versions of jobs associated with a given service
-    + instance).
-
-    Returns a list of dicts, each representing the configuration of a Chronos
-    job.
-    """
-    matching_jobs_pattern = r"^UNINITIALIZED PATTERN$"
-    if all_tags:
-        (service, instance, _, __) = chronos_tools.decompose_job_id(job_id)
-        # We add SPACER to the end as an anchor to prevent catching
-        # "my_service my_job_extra" when looking for "my_service my_job".
-        matching_jobs_pattern = r"^%s%s" % (chronos_tools.compose_job_id(service, instance), chronos_tools.SPACER)
-    else:
-        matching_jobs_pattern = r"^%s" % job_id
-    matching_jobs = chronos_tools.lookup_chronos_jobs(matching_jobs_pattern, client, include_disabled=True)
-    return matching_jobs
-
-
 def get_short_task_id(task_id):
     """Return just the Chronos-generated timestamp section of a Mesos task id."""
     return task_id.split(chronos_tools.MESOS_TASK_SPACER)[1]
@@ -252,10 +226,20 @@ def perform_command(command, service, instance, cluster, verbose, soa_dir):
     if command == "start":
         start_chronos_job(service, instance, job_id, client, cluster, complete_job_config, emergency=True)
     elif command == "stop":
-        matching_jobs = get_matching_jobs(client, job_id, all_tags=True)
+        matching_jobs = chronos_tools.lookup_chronos_jobs(
+            service=service,
+            instance=instance,
+            client=client,
+            include_disabled=True,
+        )
         stop_chronos_job(service, instance, client, cluster, matching_jobs, emergency=True)
     elif command == "restart":
-        matching_jobs = get_matching_jobs(client, job_id, all_tags=True)
+        matching_jobs = chronos_tools.lookup_chronos_jobs(
+            service=service,
+            instance=instance,
+            client=client,
+            include_disabled=True,
+        )
         restart_chronos_job(
             service,
             instance,
@@ -269,13 +253,23 @@ def perform_command(command, service, instance, cluster, verbose, soa_dir):
     elif command == "status":
         # Setting up transparent cache for http API calls
         requests_cache.install_cache("paasta_serviceinit", backend="memory")
-        # Verbose mode may want to display information about previous versions and configurations
-        all_tags = False
+        # Verbose mode shows previous versions.
         if verbose:
-            all_tags = True
-        matching_jobs = get_matching_jobs(client, job_id, all_tags)
+            git_hash = None
+            config_hash = None
+        # Non-verbose only shows the version specified via
+        # create_complete_config.
+        else:
+            (_, __, git_hash, config_hash) = chronos_tools.decompose_job_id(job_id)
+        matching_jobs = chronos_tools.lookup_chronos_jobs(
+            service=service,
+            instance=instance,
+            git_hash=git_hash,
+            config_hash=config_hash,
+            client=client,
+            include_disabled=True,
+        )
         sorted_matching_jobs = chronos_tools.sort_jobs(matching_jobs)
-        job_config = chronos_tools.load_chronos_job_config(service, instance, cluster, soa_dir=soa_dir)
         job_config = chronos_tools.load_chronos_job_config(
             service=service,
             instance=instance,
diff --git a/tests/test_chronos_serviceinit.py b/tests/test_chronos_serviceinit.py
index 5aa768b74f..154e07b658 100644
--- a/tests/test_chronos_serviceinit.py
+++ b/tests/test_chronos_serviceinit.py
@@ -88,24 +88,6 @@ def test_stop_chronos_job():
             mock_client.delete_tasks.assert_any_call(job['name'])
 
 
-def test_get_matching_jobs_all_tags_true():
-    job_id = 'my_service my_instance gityourmom configyourdad'
-    client = 'unused'
-    expected_pattern = r'^my_service my_instance '  # Trailing space is important!
-    with mock.patch('chronos_serviceinit.chronos_tools.lookup_chronos_jobs') as mock_lookup_chronos_jobs:
-        chronos_serviceinit.get_matching_jobs(client, job_id, all_tags=True)
-    mock_lookup_chronos_jobs.assert_called_once_with(expected_pattern, client, include_disabled=True)
-
-
-def test_get_matching_jobs_all_tags_false():
-    job_id = 'my_service my_instance gityourmom configyourdad'
-    client = 'unused'
-    expected_pattern = r'^my_service my_instance gityourmom configyourdad'
-    with mock.patch('chronos_serviceinit.chronos_tools.lookup_chronos_jobs') as mock_lookup_chronos_jobs:
-        chronos_serviceinit.get_matching_jobs(client, job_id, all_tags=False)
-    mock_lookup_chronos_jobs.assert_called_once_with(expected_pattern, client, include_disabled=True)
-
-
 def test_get_short_task_id():
     task_id = 'ct:1111111111111:0:my_service my_instance gityourmom configyourdad:'
     assert chronos_serviceinit.get_short_task_id(task_id) == '1111111111111'

From 888dc8b8aad4a417590514a7feb3884ce0f30caa Mon Sep 17 00:00:00 2001
From: Tyler Roscoe <troscoe@yelp.com>
Date: Thu, 5 Nov 2015 23:21:28 -0800
Subject: [PATCH 6/9] Update more callers of lookup_chronos_jobs

---
 paasta_itests/steps/chronos_steps.py           | 13 ++++++++-----
 paasta_itests/steps/setup_chronos_job_steps.py | 10 ++--------
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/paasta_itests/steps/chronos_steps.py b/paasta_itests/steps/chronos_steps.py
index 20c49a8312..007b4e6a72 100644
--- a/paasta_itests/steps/chronos_steps.py
+++ b/paasta_itests/steps/chronos_steps.py
@@ -107,12 +107,15 @@ def chronos_check_job_state(context, old_or_new_job, disabled):
         job_id = context.old_chronos_job_name
     else:
         job_id = context.chronos_job_name
+    (service, instance, git_hash, config_hash) = chronos_tools.decompose_job_id(job_id)
     jobs = chronos_tools.lookup_chronos_jobs(
-        job_id,
-        context.chronos_client,
-        max_expected=1,
-        include_disabled=desired_disabled
+        service=service,
+        instance=instance,
+        git_hash=git_hash,
+        config_hash=config_hash,
+        client=context.chronos_client,
+        include_disabled=desired_disabled,
     )
-    assert jobs != []
+    assert len(jobs) == 1
     for job in jobs:
         assert job['disabled'] == desired_disabled
diff --git a/paasta_itests/steps/setup_chronos_job_steps.py b/paasta_itests/steps/setup_chronos_job_steps.py
index ea3e0640a1..787afaea75 100644
--- a/paasta_itests/steps/setup_chronos_job_steps.py
+++ b/paasta_itests/steps/setup_chronos_job_steps.py
@@ -113,12 +113,9 @@ def old_jobs_leftover(context, job_count):
 
 @then(u'there should be {job_count} enabled jobs')
 def should_be_enabled_jobs(context, job_count):
-    search_pattern = chronos_tools.compose_job_id(
+    enabled_jobs = chronos_tools.lookup_chronos_jobs(
         service=fake_service_name,
         instance=fake_instance_name,
-    )
-    enabled_jobs = chronos_tools.lookup_chronos_jobs(
-        pattern=search_pattern,
         client=context.chronos_client,
         include_disabled=False,
     )
@@ -127,12 +124,9 @@ def should_be_enabled_jobs(context, job_count):
 
 @then(u'there should be {job_count} disabled jobs')
 def should_be_disabled_jobs(context, job_count):
-    search_pattern = chronos_tools.compose_job_id(
+    all_related_jobs = chronos_tools.lookup_chronos_jobs(
         service=fake_service_name,
         instance=fake_instance_name,
-    )
-    all_related_jobs = chronos_tools.lookup_chronos_jobs(
-        pattern=search_pattern,
         client=context.chronos_client,
         include_disabled=True,
     )

From c591ae1a557bb223948d5d82354c931c54fc5693 Mon Sep 17 00:00:00 2001
From: Tyler Roscoe <troscoe@yelp.com>
Date: Thu, 5 Nov 2015 23:29:39 -0800
Subject: [PATCH 7/9] Docstring

---
 paasta_tools/chronos_serviceinit.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/paasta_tools/chronos_serviceinit.py b/paasta_tools/chronos_serviceinit.py
index 2eca5d4da8..ecdb0e5528 100755
--- a/paasta_tools/chronos_serviceinit.py
+++ b/paasta_tools/chronos_serviceinit.py
@@ -257,7 +257,7 @@ def perform_command(command, service, instance, cluster, verbose, soa_dir):
         if verbose:
             git_hash = None
             config_hash = None
-        # Non-verbose only shows the version specified via
+        # Non-verbose shows only the version specified via
         # create_complete_config.
         else:
             (_, __, git_hash, config_hash) = chronos_tools.decompose_job_id(job_id)

From 690864933ab6bc79086cdaec7cc49520f0cba234 Mon Sep 17 00:00:00 2001
From: Tyler Roscoe <troscoe@yelp.com>
Date: Fri, 6 Nov 2015 17:02:42 -0800
Subject: [PATCH 8/9] Delete match_job_names_to_service_instance and replace
 with newly improved lookup_chronos_jobs

---
 paasta_tools/check_chronos_jobs.py | 24 ++++++++++++++----------
 paasta_tools/chronos_tools.py      | 12 +-----------
 paasta_tools/setup_chronos_job.py  |  7 +++++--
 tests/test_check_chronos_jobs.py   | 23 +++++++++++++----------
 tests/test_chronos_tools.py        |  9 ---------
 tests/test_setup_chronos_job.py    | 12 ++++++------
 6 files changed, 39 insertions(+), 48 deletions(-)

diff --git a/paasta_tools/check_chronos_jobs.py b/paasta_tools/check_chronos_jobs.py
index eabd4e4c22..264e2442ab 100755
--- a/paasta_tools/check_chronos_jobs.py
+++ b/paasta_tools/check_chronos_jobs.py
@@ -86,16 +86,23 @@ def sensu_event_for_last_run_state(state):
         return pysensu_yelp.Status.OK
 
 
-def build_service_job_mapping(configured_jobs, running_jobs):
+def build_service_job_mapping(client, configured_jobs):
     """
-    Create a dict of {(service, instance): [(chronos job, lastrunstate)]}
-    where the chronos job is any with a matching (service, instance)
-    in its name and disabled == false
+    :param client: A Chronos client used for getting the list of running jobs
+    :param configured_jobs: A list of jobs configured in Paasta, i.e. jobs we
+    expect to be able to find
+    :returns: A dict of {(service, instance): [(chronos job, lastrunstate)]}
+    where the chronos job is any with a matching (service, instance) in its
+    name and disabled == False
     """
     service_job_mapping = {}
     for job in configured_jobs:
-        # find all those jobs in the client.list belonging to each service
-        matching_jobs = chronos_tools.match_job_names_to_service_instance(job[0], job[1], running_jobs)
+        # find all the jobs belonging to each service
+        matching_jobs = chronos_tools.lookup_chronos_jobs(
+            service=job[0],
+            instance=job[1],
+            client=client,
+        )
         # filter the enabled jobs
         enabled = chronos_tools.filter_enabled_jobs(matching_jobs)
         # get the last run state for the job
@@ -143,10 +150,7 @@ def main(args):
     # get those jobs listed in configs
     configured_jobs = chronos_tools.get_chronos_jobs_for_cluster(soa_dir=args.soa_dir)
 
-    # get the running jobs
-    running_jobs = client.list()
-
-    service_job_mapping = build_service_job_mapping(configured_jobs, running_jobs)
+    service_job_mapping = build_service_job_mapping(client, configured_jobs)
     for service_instance, job_state_pairs in service_job_mapping.items():
         service, instance = service_instance[0], service_instance[1]
         sensu_output, sensu_status = sensu_message_status_for_jobs(service, instance, job_state_pairs)
diff --git a/paasta_tools/chronos_tools.py b/paasta_tools/chronos_tools.py
index 2f7c34a857..0711bc4a45 100644
--- a/paasta_tools/chronos_tools.py
+++ b/paasta_tools/chronos_tools.py
@@ -546,16 +546,6 @@ def get_key(job):
     )
 
 
-def match_job_names_to_service_instance(service, instance, jobs):
-    """Given a list of chronos jobs, return those which are associated with a given service and instance."""
-    matching = []
-    for job in jobs:
-        jobs_service, jobs_instance, _, _ = decompose_job_id(job['name'])
-        if jobs_service == service and jobs_instance == instance:
-            matching.append(job)
-    return matching
-
-
 def lookup_chronos_jobs(service, instance, client, git_hash=None, config_hash=None, include_disabled=False):
     """Retrieves Chronos jobs with names that match a specified pattern.
 
@@ -565,7 +555,7 @@ def lookup_chronos_jobs(service, instance, client, git_hash=None, config_hash=No
     :param git_hash: git_hash we're looking for. Optional; return jobs with any
     git_hash if omitted.
     :param config_hash: config_hash we're looking for. Optional; return jobs
-    with any git_hash if omitted.
+    with any config_hash if omitted.
     :param include_disabled: boolean indicating if disabled jobs should be
     included in the returned list
     :returns: list of job dicts whose name matches the ``service`` and
diff --git a/paasta_tools/setup_chronos_job.py b/paasta_tools/setup_chronos_job.py
index 181863dafc..2a545b0123 100755
--- a/paasta_tools/setup_chronos_job.py
+++ b/paasta_tools/setup_chronos_job.py
@@ -142,8 +142,11 @@ def bounce_chronos_job(
 
 def setup_job(service, instance, chronos_job_config, complete_job_config, client, cluster):
     job_id = complete_job_config['name']
-    all_existing_jobs = chronos_tools.match_job_names_to_service_instance(
-        service=service, instance=instance, jobs=client.list())
+    all_existing_jobs = chronos_tools.lookup_chronos_jobs(
+        service=service,
+        instance=instance,
+        client=client,
+    )
     # TODO: Sort the jobs in the right order so we delete the least relevant
     # This currently depends on implicit behavior that Chronos returns jobs
     # "oldest first"
diff --git a/tests/test_check_chronos_jobs.py b/tests/test_check_chronos_jobs.py
index a6b4676d5a..473a98aa8d 100644
--- a/tests/test_check_chronos_jobs.py
+++ b/tests/test_check_chronos_jobs.py
@@ -85,20 +85,23 @@ def test_sensu_event_for_last_run_state_invalid():
         check_chronos_jobs.sensu_event_for_last_run_state(100)
 
 
-@patch('check_chronos_jobs.chronos_tools.match_job_names_to_service_instance')
-@patch('check_chronos_jobs.chronos_tools.filter_enabled_jobs')
-@patch('check_chronos_jobs.chronos_tools.get_status_last_run')
-def test_build_service_job_mapping(mock_last_run_state, mock_filter_enabled_jobs, mock_match_job_names):
-    mock_match_job_names.side_effect = [[{}, {}, {}] for x in range(0, 3)]
-    mock_filter_enabled_jobs.side_effect = [[{}, {}, {}] for x in range(0, 3)]
-    mock_last_run_state.side_effect = [
+@patch('check_chronos_jobs.chronos_tools.lookup_chronos_jobs', autospec=True)
+@patch('check_chronos_jobs.chronos_tools.filter_enabled_jobs', autospec=True)
+@patch('check_chronos_jobs.chronos_tools.get_status_last_run', autospec=True)
+def test_build_service_job_mapping(mock_last_run_state, mock_filter_enabled_jobs, mock_lookup_chronos_jobs):
+    # iter() is a workaround
+    # (http://lists.idyll.org/pipermail/testing-in-python/2013-April/005527.html)
+    # for a bug in mock (http://bugs.python.org/issue17826)
+    mock_lookup_chronos_jobs.side_effect = iter([[{}, {}, {}] for x in range(0, 3)])
+    mock_filter_enabled_jobs.side_effect = iter([[{}, {}, {}] for x in range(0, 3)])
+    mock_last_run_state.side_effect = iter([
         ('faketimestamp', chronos_tools.LastRunState.Success),
         ('faketimestamp', chronos_tools.LastRunState.Fail),
         ('faketimestamp', chronos_tools.LastRunState.NotRun),
-    ] * 3
+    ] * 3)
 
     fake_configured_jobs = [('service1', 'main'), ('service2', 'main'), ('service3', 'main')]
-    fake_running_jobs = [('service1', 'main'), ('service2', 'main'), ('service3', 'main')]
+    fake_client = Mock(list=Mock(return_value=[('service1', 'main'), ('service2', 'main'), ('service3', 'main')]))
 
     expected_job_states = [
         ({}, chronos_tools.LastRunState.Success),
@@ -111,7 +114,7 @@ def test_build_service_job_mapping(mock_last_run_state, mock_filter_enabled_jobs
         ('service2', 'main'): expected_job_states,
         ('service3', 'main'): expected_job_states,
     }
-    assert check_chronos_jobs.build_service_job_mapping(fake_configured_jobs, fake_running_jobs) == expected
+    assert check_chronos_jobs.build_service_job_mapping(fake_client, fake_configured_jobs) == expected
 
 
 def test_message_for_status_fail():
diff --git a/tests/test_chronos_tools.py b/tests/test_chronos_tools.py
index aab8c0ae20..09722db788 100644
--- a/tests/test_chronos_tools.py
+++ b/tests/test_chronos_tools.py
@@ -1202,15 +1202,6 @@ def test_sort_jobs(self):
         jobs = [early_job, late_job, unrun_job]
         assert chronos_tools.sort_jobs(jobs) == [late_job, early_job, unrun_job]
 
-    def test_match_job_names_to_service_handles_mutiple_jobs(self):
-        mock_jobs = [{'name': 'fake-service fake-instance git1 config1'},
-                     {'name': 'fake-service fake-instance git2 config2'},
-                     {'name': 'other-service other-instance git config'}]
-        expected = mock_jobs[:2]
-        actual = chronos_tools.match_job_names_to_service_instance(
-            service='fake-service', instance='fake-instance', jobs=mock_jobs)
-        assert sorted(actual) == sorted(expected)
-
     def test_disable_job(self):
         fake_client_class = mock.Mock(spec='chronos.ChronosClient')
         fake_client = fake_client_class(servers=[])
diff --git a/tests/test_setup_chronos_job.py b/tests/test_setup_chronos_job.py
index cc25647ffb..30c5ab329f 100644
--- a/tests/test_setup_chronos_job.py
+++ b/tests/test_setup_chronos_job.py
@@ -252,14 +252,14 @@ def test_setup_job_with_previously_enabled_job(self):
         }
         with contextlib.nested(
             mock.patch('setup_chronos_job.bounce_chronos_job', autospec=True, return_value=(0, 'ok')),
-            mock.patch('paasta_tools.chronos_tools.match_job_names_to_service_instance',
+            mock.patch('paasta_tools.chronos_tools.lookup_chronos_jobs',
                        autospec=True, return_value=[fake_existing_job]),
             mock.patch('paasta_tools.chronos_tools.load_system_paasta_config', autospec=True),
             mock.patch('paasta_tools.chronos_tools.load_chronos_job_config',
                        autospec=True, return_value=self.fake_chronos_job_config),
         ) as (
             mock_bounce_chronos_job,
-            mock_match_job_names_to_service_instance,
+            mock_lookup_chronos_jobs,
             load_system_paasta_config_patch,
             load_chronos_job_config_patch,
         ):
@@ -286,21 +286,21 @@ def test_setup_job_with_previously_enabled_job(self):
                 job_to_create=complete_config,
                 client=self.fake_client,
             )
-            assert mock_match_job_names_to_service_instance.called
+            assert mock_lookup_chronos_jobs.called
             assert actual == mock_bounce_chronos_job.return_value
 
     def test_setup_job_does_nothing_with_only_existing_app(self):
         fake_existing_job = self.fake_config_dict
         with contextlib.nested(
             mock.patch('setup_chronos_job.bounce_chronos_job', autospec=True, return_value=(0, 'ok')),
-            mock.patch('paasta_tools.chronos_tools.match_job_names_to_service_instance',
+            mock.patch('paasta_tools.chronos_tools.lookup_chronos_jobs',
                        autospec=True, return_value=[fake_existing_job]),
             mock.patch('paasta_tools.chronos_tools.load_system_paasta_config', autospec=True),
             mock.patch('paasta_tools.chronos_tools.load_chronos_job_config',
                        autospec=True, return_value=self.fake_chronos_job_config),
         ) as (
             mock_bounce_chronos_job,
-            mock_match_job_names_to_service_instance,
+            mock_lookup_chronos_jobs,
             load_system_paasta_config_patch,
             load_chronos_job_config_patch,
         ):
@@ -330,7 +330,7 @@ def test_setup_job_does_nothing_with_only_existing_app(self):
                 job_to_create=None,
                 client=self.fake_client,
             )
-            assert mock_match_job_names_to_service_instance.called
+            assert mock_lookup_chronos_jobs.called
             assert actual == mock_bounce_chronos_job.return_value
 
     def test_send_event(self):

From 97603e04faf48a6c2a770913441ea0639c8d79a1 Mon Sep 17 00:00:00 2001
From: Tyler Roscoe <troscoe@yelp.com>
Date: Mon, 9 Nov 2015 15:54:47 -0800
Subject: [PATCH 9/9] Extract filter_chronos_jobs from lookup_chronos_jobs.

This was after discussion with robj in
https://github.com/Yelp/paasta/pull/24 and then in person.  Basically
this is a compromise between SRP and DI (which favor calculating the
list of jobs separately from filtering it) and YAGNI (currently every
caller of lookup_chronos_jobs would need to do its own client.list() and
pass that in). Hence, filter_chronos_jobs is fully SRP and DI while
lookup_chronos_jobs is less pure but more convenient.
---
 paasta_tools/chronos_tools.py |  47 +++++++++++----
 tests/test_chronos_tools.py   | 108 +++++++++++++++++++++++++++++-----
 2 files changed, 128 insertions(+), 27 deletions(-)

diff --git a/paasta_tools/chronos_tools.py b/paasta_tools/chronos_tools.py
index 0711bc4a45..3e01df0364 100644
--- a/paasta_tools/chronos_tools.py
+++ b/paasta_tools/chronos_tools.py
@@ -546,23 +546,44 @@ def get_key(job):
     )
 
 
-def lookup_chronos_jobs(service, instance, client, git_hash=None, config_hash=None, include_disabled=False):
-    """Retrieves Chronos jobs with names that match a specified pattern.
+def lookup_chronos_jobs(client, service=None, instance=None, git_hash=None, config_hash=None, include_disabled=False):
+    """Discovers Chronos jobs and filters them with ``filter_chronos_jobs()``.
 
-    :param service: service we're looking for
-    :param instance: instance we're looking for
     :param client: Chronos client object
-    :param git_hash: git_hash we're looking for. Optional; return jobs with any
-    git_hash if omitted.
-    :param config_hash: config_hash we're looking for. Optional; return jobs
-    with any config_hash if omitted.
+    :param service: passed on to ``filter_chronos_jobs()``
+    :param instance: passed on to ``filter_chronos_jobs()``
+    :param git_hash: passed on to ``filter_chronos_jobs()``
+    :param config_hash: passed on to ``filter_chronos_jobs()``
+    :param include_disabled: passed on to ``filter_chronos_jobs()``
+    :returns: list of job dicts discovered by ``client`` and filtered by
+    ``filter_chronos_jobs()`` using the other parameters
+    """
+    jobs = client.list()
+    return filter_chronos_jobs(
+        jobs=jobs,
+        service=service,
+        instance=instance,
+        git_hash=git_hash,
+        config_hash=config_hash,
+        include_disabled=include_disabled,
+    )
+
+
+def filter_chronos_jobs(jobs, service, instance, git_hash, config_hash, include_disabled):
+    """Filters a list of Chronos jobs based on several criteria.
+
+    :param jobs: a list of jobs, as calculated in ``lookup_chronos_jobs()``
+    :param service: service we're looking for. If None, don't filter based on this key.
+    :param instance: instance we're looking for. If None, don't filter based on this key.
+    :param git_hash: git_hash we're looking for. If None, don't filter based on
+    this key.
+    :param config_hash: config_hash we're looking for. If None, don't filter
+    based on this key.
     :param include_disabled: boolean indicating if disabled jobs should be
     included in the returned list
-    :returns: list of job dicts whose name matches the ``service`` and
-    ``instance`` (and maybe ``git_hash`` and ``config_hash``) arguments
+    :returns: list of job dicts whose name matches the arguments (if any)
     provided
     """
-    jobs = client.list()
     matching_jobs = []
     for job in jobs:
         try:
@@ -570,8 +591,8 @@ def lookup_chronos_jobs(service, instance, client, git_hash=None, config_hash=No
         except InvalidJobNameError:
             continue
         if (
-            job_service == service
-            and job_instance == instance
+            (service is None or job_service == service)
+            and (instance is None or job_instance == instance)
             and (git_hash is None or job_git_hash == git_hash)
             and (config_hash is None or job_config_hash == config_hash)
         ):
diff --git a/tests/test_chronos_tools.py b/tests/test_chronos_tools.py
index 09722db788..493c9f5083 100644
--- a/tests/test_chronos_tools.py
+++ b/tests/test_chronos_tools.py
@@ -695,6 +695,77 @@ def test_get_chronos_jobs_for_cluster(self):
             assert list_jobs_patch.call_count == 2
 
     def test_lookup_chronos_jobs_with_service_and_instance(self):
+        fake_client = mock.Mock()
+        fake_service = 'fake_service'
+        fake_instance = 'fake_instance'
+        with mock.patch('chronos_tools.filter_chronos_jobs', autospec=True) as mock_filter_chronos_jobs:
+            chronos_tools.lookup_chronos_jobs(
+                client=fake_client,
+                service=fake_service,
+                instance=fake_instance,
+            )
+            mock_filter_chronos_jobs.assert_called_once_with(
+                jobs=fake_client.list.return_value,
+                service=fake_service,
+                instance=fake_instance,
+                git_hash=None,
+                config_hash=None,
+                include_disabled=False,
+            )
+
+    def test_filter_chronos_jobs_with_no_filters(self):
+        fake_service = 'fake_service'
+        fake_instance = 'fake_instance'
+        fake_jobs = [
+            {
+                'name': chronos_tools.compose_job_id(
+                    fake_service,
+                    fake_instance,
+                    'git1111',
+                    'config1111',
+                ),
+                'disabled': False,
+            },
+            {
+                'name': chronos_tools.compose_job_id(
+                    fake_service,
+                    fake_instance,
+                    'git2222',
+                    'config2222',
+                ),
+                'disabled': False,
+            },
+            {
+                'name': chronos_tools.compose_job_id(
+                    fake_service,
+                    fake_instance,
+                    'gitdisabled',
+                    'configdisabled',
+                ),
+                'disabled': True,
+            },
+            {
+                'name': chronos_tools.compose_job_id(
+                    'some_other_service',
+                    'some_other_instance',
+                    'gitother',
+                    'configother',
+                ),
+                'disabled': False,
+            },
+        ]
+        expected = fake_jobs
+        actual = chronos_tools.filter_chronos_jobs(
+            jobs=fake_jobs,
+            service=None,
+            instance=None,
+            git_hash=None,
+            config_hash=None,
+            include_disabled=True,
+        )
+        assert sorted(actual) == sorted(expected)
+
+    def test_filter_chronos_jobs_with_service_and_instance(self):
         fake_service = 'fake_service'
         fake_instance = 'fake_instance'
         fake_jobs = [
@@ -735,16 +806,18 @@ def test_lookup_chronos_jobs_with_service_and_instance(self):
                 'disabled': False,
             },
         ]
-        fake_client = mock.Mock(list=mock.Mock(return_value=fake_jobs))
         expected = [fake_jobs[0], fake_jobs[1]]
-        actual = chronos_tools.lookup_chronos_jobs(
+        actual = chronos_tools.filter_chronos_jobs(
+            jobs=fake_jobs,
             service=fake_service,
             instance=fake_instance,
-            client=fake_client,
+            git_hash=None,
+            config_hash=None,
+            include_disabled=False,
         )
         assert sorted(actual) == sorted(expected)
 
-    def test_lookup_chronos_jobs_include_disabled(self):
+    def test_filter_chronos_jobs_include_disabled(self):
         fake_service = 'fake_service'
         fake_instance = 'fake_instance'
         fake_jobs = [
@@ -785,17 +858,18 @@ def test_lookup_chronos_jobs_include_disabled(self):
                 'disabled': False,
             },
         ]
-        fake_client = mock.Mock(list=mock.Mock(return_value=fake_jobs))
         expected = [fake_jobs[0], fake_jobs[1], fake_jobs[2]]
-        actual = chronos_tools.lookup_chronos_jobs(
+        actual = chronos_tools.filter_chronos_jobs(
+            jobs=fake_jobs,
             service=fake_service,
             instance=fake_instance,
-            client=fake_client,
+            git_hash=None,
+            config_hash=None,
             include_disabled=True,
         )
         assert sorted(actual) == sorted(expected)
 
-    def test_lookup_chronos_jobs_with_everything_specified(self):
+    def test_filter_chronos_jobs_with_everything_specified(self):
         fake_service = 'fake_service'
         fake_instance = 'fake_instance'
         fake_git_hash = 'fake_git_hash'
@@ -838,26 +912,32 @@ def test_lookup_chronos_jobs_with_everything_specified(self):
                 'disabled': False,
             },
         ]
-        fake_client = mock.Mock(list=mock.Mock(return_value=fake_jobs))
         expected = [fake_jobs[0]]
-        actual = chronos_tools.lookup_chronos_jobs(
+        actual = chronos_tools.filter_chronos_jobs(
+            jobs=fake_jobs,
             service=fake_service,
             instance=fake_instance,
             git_hash=fake_git_hash,
             config_hash=fake_config_hash,
-            client=fake_client,
+            include_disabled=False,
         )
         assert sorted(actual) == sorted(expected)
 
-    def test_lookup_chronos_jobs_skips_non_paasta_job_id(self):
+    def test_filter_chronos_jobs_skips_non_paasta_job_id(self):
         fake_jobs = [
             {
                 'name': 'some non-paasta job',
                 'disabled': False,
             },
         ]
-        fake_client = mock.Mock(list=mock.Mock(return_value=fake_jobs))
-        actual = chronos_tools.lookup_chronos_jobs('whatever', 'whatever', fake_client)
+        actual = chronos_tools.filter_chronos_jobs(
+            jobs=fake_jobs,
+            service='whatever',
+            instance='whatever',
+            git_hash='whatever',
+            config_hash='whatever',
+            include_disabled=False,
+        )
         # The main thing here is that InvalidJobNameError is not raised.
         assert actual == []