From d35c13685893ec33ec0c04d886b8aeff8bec0656 Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Fri, 20 Mar 2020 13:32:37 -0700
Subject: [PATCH] Warn about unused pools, ones not referenced by a rule

---
 octodns/record/__init__.py   | 11 +++++++++--
 tests/test_octodns_record.py | 35 +++++++++++++++++++++++++----------
 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/octodns/record/__init__.py b/octodns/record/__init__.py
index 70066fe..0eb9431 100644
--- a/octodns/record/__init__.py
+++ b/octodns/record/__init__.py
@@ -507,6 +507,8 @@ class _DynamicMixin(object):
         except KeyError:
             pools = {}
 
+        pools_exist = set()
+        pools_seen = set()
         if not isinstance(pools, dict):
             reasons.append('pools must be a dict')
         elif not pools:
@@ -522,6 +524,8 @@ class _DynamicMixin(object):
                     reasons.append('pool "{}" is missing values'.format(_id))
                     continue
 
+                pools_exist.add(_id)
+
                 for i, value in enumerate(values):
                     value_num = i + 1
                     try:
@@ -578,8 +582,6 @@ class _DynamicMixin(object):
             seen_default = False
 
             # TODO: don't allow 'default' as a pool name, reserved
-            # TODO: warn or error on unused pools?
-            pools_seen = set()
             for i, rule in enumerate(rules):
                 rule_num = i + 1
                 try:
@@ -618,6 +620,11 @@ class _DynamicMixin(object):
                         reasons.extend(GeoCodes.validate(geo, 'rule {} '
                                                          .format(rule_num)))
 
+        unused = pools_exist - pools_seen
+        if unused:
+            unused = '", "'.join(sorted(unused))
+            reasons.append('unused pools: "{}"'.format(unused))
+
         return reasons
 
     def __init__(self, zone, name, data, *args, **kwargs):
diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py
index 749e686..4216c52 100644
--- a/tests/test_octodns_record.py
+++ b/tests/test_octodns_record.py
@@ -3075,7 +3075,7 @@ class TestDynamicRecords(TestCase):
             'invalid IPv4 address "blip"',
         ], ctx.exception.reasons)
 
-        # missing rules
+        # missing rules, and unused pools
         a_data = {
             'dynamic': {
                 'pools': {
@@ -3102,7 +3102,10 @@ class TestDynamicRecords(TestCase):
         }
         with self.assertRaises(ValidationError) as ctx:
             Record.new(self.zone, 'bad', a_data)
-        self.assertEquals(['missing rules'], ctx.exception.reasons)
+        self.assertEquals([
+            'missing rules',
+            'unused pools: "one", "two"',
+        ], ctx.exception.reasons)
 
         # empty rules
         a_data = {
@@ -3132,7 +3135,10 @@ class TestDynamicRecords(TestCase):
         }
         with self.assertRaises(ValidationError) as ctx:
             Record.new(self.zone, 'bad', a_data)
-        self.assertEquals(['missing rules'], ctx.exception.reasons)
+        self.assertEquals([
+            'missing rules',
+            'unused pools: "one", "two"',
+        ], ctx.exception.reasons)
 
         # rules not a list/tuple
         a_data = {
@@ -3162,7 +3168,10 @@ class TestDynamicRecords(TestCase):
         }
         with self.assertRaises(ValidationError) as ctx:
             Record.new(self.zone, 'bad', a_data)
-        self.assertEquals(['rules must be a list'], ctx.exception.reasons)
+        self.assertEquals([
+            'rules must be a list',
+            'unused pools: "one", "two"',
+        ], ctx.exception.reasons)
 
         # rule without pool
         a_data = {
@@ -3196,7 +3205,10 @@ class TestDynamicRecords(TestCase):
         }
         with self.assertRaises(ValidationError) as ctx:
             Record.new(self.zone, 'bad', a_data)
-        self.assertEquals(['rule 1 missing pool'], ctx.exception.reasons)
+        self.assertEquals([
+            'rule 1 missing pool',
+            'unused pools: "two"',
+        ], ctx.exception.reasons)
 
         # rule with non-string pools
         a_data = {
@@ -3231,8 +3243,10 @@ class TestDynamicRecords(TestCase):
         }
         with self.assertRaises(ValidationError) as ctx:
             Record.new(self.zone, 'bad', a_data)
-        self.assertEquals(['rule 1 invalid pool "[]"'],
-                          ctx.exception.reasons)
+        self.assertEquals([
+            'rule 1 invalid pool "[]"',
+            'unused pools: "two"',
+        ], ctx.exception.reasons)
 
         # rule references non-existent pool
         a_data = {
@@ -3267,8 +3281,10 @@ class TestDynamicRecords(TestCase):
         }
         with self.assertRaises(ValidationError) as ctx:
             Record.new(self.zone, 'bad', a_data)
-        self.assertEquals(["rule 1 undefined pool \"non-existent\""],
-                          ctx.exception.reasons)
+        self.assertEquals([
+            "rule 1 undefined pool \"non-existent\"",
+            'unused pools: "two"',
+        ], ctx.exception.reasons)
 
         # rule with invalid geos
         a_data = {
@@ -3411,7 +3427,6 @@ class TestDynamicRecords(TestCase):
                 '2.2.2.2',
             ],
         }
-        print('\n*** start ***')
         with self.assertRaises(ValidationError) as ctx:
             Record.new(self.zone, 'bad', a_data)
         self.assertEquals(['rule 3 invalid, target pool "one" reused'],