Merge pull request #30 from djmitche/issue29

Support merging resources

README.md

@@ -247,6 +247,10 @@ The class has the following methods:
 * `resources.filter(pattern)` - return a new Resources object containing only resources matching the given regular expression string
 * `resources.map(functor)` - return a new Resources object, with fuctor applied to each resource.  This is typically used in modifiers.
 
+Resources must be unique -- tc-admin cannot manage multiple hooks with the same name, for example.
+However, some resource kinds support merging, where adding a resource with the same identity as one that already exists "merges" it into the existing resource.
+See the description of roles, below.
+
 The remaining classes represent individual resources.
 Each has an `id` formed from its kind and the unique identifier for the resource in the Taskcluster.
 For example, `Hook=release-hooks/beta-release`.
@@ -282,14 +286,27 @@ The items in `bindings` are instances of `Binding(exchange=.., routingKeyPattern
 ```python
 from tcadmin.resources import Role
 
-hook = Role(
+role = Role(
     roleId=..,
     description=..,
     scopes=(.., ..))
 ```
 
 As with hooks, `scopes` must be a tuple (not a list) of strings.
 
+Roles can be merged if their descriptions match.
+The resulting role contains the union of the scopes of the input roles.
+This functionality makes management of roles easier in cases where different parts of the generation process may add scopes to the same role.
+
+For example:
+
+```python
+resources.add(Role(roleId="my-role", description="My Role", scopes=["scope1"]))
+resources.add(Role(roleId="my-role", description="My Role", scopes=["scope2"]))
+```
+
+This will result in a single Role with scopes `["scope1", "scope2"]`.
+
 ### WorkerPool
 
 ```python

tcadmin/resources/resources.py

@@ -9,7 +9,6 @@
 import blessings
 import functools
 import textwrap
-import itertools
 from memoized import memoized
 from sortedcontainers import SortedKeyList
 
@@ -51,6 +50,15 @@ def to_api(self):
         "Construct a payload for Taskcluster API methods"
         raise NotImplementedError
 
+    def merge(self, other):
+        """
+        Construct a 'merged' resource, given another resource with the same kind and id.
+
+        Generally, resources can be merged if all fields either match or can be combined
+        in some way, such as taking the union of scopesets
+        """
+        raise RuntimeError("Cannot merge resources of kind {}".format(self.kind))
+
     @property
     def kind(self):
         "The kind of this instance"
@@ -101,21 +109,37 @@ class Resources:
     )
 
     def __attrs_post_init__(self):
+        self._by_id = {r.id: r for r in self.resources}
+        if len(self._by_id) != len(self.resources):
+            raise RuntimeError("duplicate resources passed to Resources constructor")
         self._verify()
 
-    def add(self, resource):
+    def add(self, resource, _skip_verify=False):
         "Add the given resource to the collection"
         if not self.is_managed(resource.id):
             raise RuntimeError("unmanaged resource: " + resource.id)
+
+        # if the resource already exists, try to merge it and remove the
+        # previous version from the list of resources (SortedKeyList does not
+        # support in-place replacement of items)
+        if resource.id in self._by_id:
+            existing = self._by_id[resource.id]
+            resource = self._by_id[resource.id].merge(resource)
+            idx = self.resources.index(existing)
+            del self.resources[idx]
+
+        self._by_id[resource.id] = resource
         self.resources.add(resource)
-        self._verify()
+
+        if not _skip_verify:
+            self._verify()
 
     def update(self, resources):
         "Add the given resources to the collection"
         for resource in resources:
             if not self.is_managed(resource.id):
                 raise RuntimeError("unmanaged resource: " + resource.id)
-        self.resources.update(resources)
+            self.add(resource, _skip_verify=True)
         self._verify()
 
     def manage(self, pattern):
@@ -141,15 +165,6 @@ def map(self, functor):
     def _verify(self):
         "Verify that this set of resources is legal (all managed, no duplicates)"
 
-        # search for duplicates, taking advantage of sorting
-        pairs = zip(
-            itertools.chain([None], (r1.id for r1 in self)), (r2.id for r2 in self)
-        )
-        dupes = [a for (a, b) in pairs if a == b]
-        if dupes:
-            unique_dupes = sorted(set(dupes))
-            raise RuntimeError("duplicate resources: " + ", ".join(unique_dupes))
-
         unmanaged = sorted([r.id for r in self if not self.is_managed(r.id)])
         if unmanaged:
             raise RuntimeError("unmanaged resources: " + ", ".join(unmanaged))

tcadmin/resources/role.py

@@ -8,6 +8,7 @@
 
 from .resources import Resource
 from .util import description_converter, scopes_converter, list_formatter
+from ..util.scopes import normalizeScopes
 
 
 @attr.s
@@ -30,3 +31,12 @@ def from_api(cls, api_result):
     def to_api(self):
         "Construct a payload for use with auth.createRole or auth.updateRole"
         return {"description": self.description, "scopes": self.scopes}
+
+    def merge(self, other):
+        assert self.roleId == other.roleId
+        if self.description != other.description:
+            raise RuntimeError(
+                "Descriptions for {} to be merged differ".format(self.id)
+            )
+        scopes = normalizeScopes(self.scopes + other.scopes)
+        return Role(roleId=self.roleId, description=self.description, scopes=scopes)

tcadmin/tests/test_resources_resources.py

@@ -18,6 +18,17 @@ class Thing(Resource):
     value = attr.ib(type=str)
 
 
+@attr.s
+class MergeableThing(Resource):
+    thingId = attr.ib(type=str)
+    value = attr.ib(type=str)
+
+    def merge(self, other):
+        return MergeableThing(
+            thingId=self.thingId, value=self.value + "|" + other.value
+        )
+
+
 @attr.s
 class ListThing(Resource):
     listThingId = attr.ib(type=str)
@@ -136,6 +147,14 @@ def test_resources_filter():
     assert [r.thingId for r in coll] == ["abc", "abd"]
 
 
+def test_resources_merge():
+    "Resources merge when added"
+    coll = Resources([], [".*"])
+    coll.add(MergeableThing(thingId="a", value="artichoke"))
+    coll.add(MergeableThing(thingId="a", value="aardvark"))
+    assert coll.resources[0] == MergeableThing(thingId="a", value="artichoke|aardvark")
+
+
 def test_resources_to_json():
     "Resources.to_json produces the expected data structure"
     rsrcs = Resources(
@@ -184,11 +203,19 @@ def test_resources_manages():
     assert not rsrcs.managed.matches("Thing=y")
 
 
-def test_resources_verify_duplicates_prohibited():
+def test_resources_verify_duplicates_prohibited_constructor():
     "Duplicate resources are not allowed"
     with pytest.raises(RuntimeError) as exc:
         Resources([Thing("x", "1"), Thing("x", "1")], [".*"])
-    assert "duplicate resources: Thing=x" in str(exc.value)
+    assert "duplicate resources" in str(exc.value)
+
+
+def test_resources_verify_duplicates_prohibited():
+    "Duplicate resources are not allowed"
+    rsrcs = Resources([Thing("x", "1")], [".*"])
+    with pytest.raises(RuntimeError) as exc:
+        rsrcs.add(Thing("x", "1"))
+    assert "Cannot merge resources of kind Thing" in str(exc.value)
 
 
 def test_resources_verify_unmanaged_prohibited():

tcadmin/tests/test_resources_role.py

@@ -4,6 +4,7 @@
 # v. 2.0. If a copy of the MPL was not distributed with this file, You can
 # obtain one at http://mozilla.org/MPL/2.0/.
 
+import pytest
 import textwrap
 
 from tcadmin.resources.resources import Resource
@@ -51,3 +52,32 @@ def test_role_from_api():
     assert role.roleId == "my:role-id"
     assert role.description == api_result["description"]
     assert role.scopes == ("scope-a", "scope-b")
+
+
+def test_role_merge_simple():
+    "Roles with matching descriptions can be merged"
+    r1 = Role(roleId="role", description="test", scopes=["a"])
+    r2 = Role(roleId="role", description="test", scopes=["b"])
+    merged = r1.merge(r2)
+    assert merged.roleId == "role"
+    assert merged.description.endswith("test")
+    assert merged.scopes == ("a", "b")
+
+
+def test_role_merge_normalized():
+    "Scopes are normalized when merging"
+    r1 = Role(roleId="role", description="test", scopes=["a", "b*"])
+    r2 = Role(roleId="role", description="test", scopes=["a", "bcdef", "c*"])
+    merged = r1.merge(r2)
+    assert merged.roleId == "role"
+    assert merged.description.endswith("test")
+    assert merged.scopes == ("a", "b*", "c*")
+
+
+def test_role_merge_different_descr():
+    "Descriptions must match to merge"
+    r1 = Role(roleId="role", description="test1", scopes=["a"])
+    r2 = Role(roleId="role", description="test2", scopes=["b"])
+    with pytest.raises(RuntimeError) as exc:
+        r1.merge(r2)
+    assert "Descriptions for Role=role to be merged differ" in str(exc.value)

tcadmin/util/scopes.py

@@ -6,8 +6,6 @@
 
 import re
 
-from ..resources import Role
-
 
 class Resolver:
     """
@@ -23,6 +21,8 @@ def __init__(self, roles):
     def from_resources(cls, resources):
         """Construct an instance from a Resources instance, ignoring any non-Role
         resources"""
+        from ..resources import Role
+
         roles = {}
         for resource in resources:
             if isinstance(resource, Role):