)]}'
{"tests/unit/test_zk.py":[{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"2552b32865c5d7af97368773b99eefee21ffdc00","unresolved":true,"context_lines":[{"line_number":278,"context_line":"            if len(components) \u003d\u003d 0:"},{"line_number":279,"context_line":"                break"},{"line_number":280,"context_line":""},{"line_number":281,"context_line":"    def test_component_registry(self):"},{"line_number":282,"context_line":"        self.component_info \u003d ExecutorComponent(self.zk_client, \u0027test\u0027)"},{"line_number":283,"context_line":"        self.component_info.register()"},{"line_number":284,"context_line":"        self.assertComponentState(\"executor\", BaseComponent.STOPPED)"}],"source_content_type":"text/x-python","patch_set":4,"id":"edaa07a6_69b2fd67","line":281,"updated":"2021-06-18 06:37:29.000000000","message":"Could we move this test to tests/unit/test_component_registry.py? This would avoid that we have to duplicate all the assertion helper methods.","commit_id":"120f9b473d9eb63dcc4d58fd965c210ee2778336"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"7c24c8b2f363d49f887a33c86ca1ad24de0bca04","unresolved":false,"context_lines":[{"line_number":278,"context_line":"            if len(components) \u003d\u003d 0:"},{"line_number":279,"context_line":"                break"},{"line_number":280,"context_line":""},{"line_number":281,"context_line":"    def test_component_registry(self):"},{"line_number":282,"context_line":"        self.component_info \u003d ExecutorComponent(self.zk_client, \u0027test\u0027)"},{"line_number":283,"context_line":"        self.component_info.register()"},{"line_number":284,"context_line":"        self.assertComponentState(\"executor\", BaseComponent.STOPPED)"}],"source_content_type":"text/x-python","patch_set":4,"id":"8201f3be_acfbe419","line":281,"updated":"2021-06-21 18:02:53.000000000","message":"No, that\u0027s a full ZuulTestCase; this test runs without a Zuul so that we can test the API without actually starting up components.\n\nIt would make sense to move the assertions to a shared location, but we don\u0027t have an established pattern for that, so we\u0027re pretty lenient on duplicating helper methods in tests when they need to be in different classes.","commit_id":"120f9b473d9eb63dcc4d58fd965c210ee2778336"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"2ccb773285283b18c243348f422f733a4758d80f","unresolved":false,"context_lines":[{"line_number":278,"context_line":"            if len(components) \u003d\u003d 0:"},{"line_number":279,"context_line":"                break"},{"line_number":280,"context_line":""},{"line_number":281,"context_line":"    def test_component_registry(self):"},{"line_number":282,"context_line":"        self.component_info \u003d ExecutorComponent(self.zk_client, \u0027test\u0027)"},{"line_number":283,"context_line":"        self.component_info.register()"},{"line_number":284,"context_line":"        self.assertComponentState(\"executor\", BaseComponent.STOPPED)"}],"source_content_type":"text/x-python","patch_set":4,"id":"7ed642fa_936c3835","line":281,"in_reply_to":"8201f3be_acfbe419","updated":"2021-06-23 12:18:32.000000000","message":"I see. This makes sense.","commit_id":"120f9b473d9eb63dcc4d58fd965c210ee2778336"}],"zuul/zk/__init__.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"2a85b00ef9e1538ee22ad26b33d097e853136ba3","unresolved":false,"context_lines":[{"line_number":191,"context_line":"    \"\"\"Base class for stateless Zookeeper interaction.\"\"\""},{"line_number":192,"context_line":""},{"line_number":193,"context_line":"    def __init__(self, client: ZooKeeperClient):"},{"line_number":194,"context_line":"        self.client \u003d client"},{"line_number":195,"context_line":""},{"line_number":196,"context_line":"    @property"},{"line_number":197,"context_line":"    def kazoo_client(self) -\u003e KazooClient:"}],"source_content_type":"text/x-python","patch_set":7,"id":"b559cec1_638b298c","line":194,"updated":"2021-06-28 17:54:05.000000000","message":"Seems like it would raise an attributeerror exception; that seems okay for someone trying to modify a read-only component?  I guess we could use a dedicated exception for that?  But as long as it fails, I\u0027m happy.  :)","commit_id":"0ec943e7a1fb42e49f1e996578510dce8869228b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"f0b65ad17db857d9bc05d0c4ad56ab35e505897e","unresolved":true,"context_lines":[{"line_number":191,"context_line":"    \"\"\"Base class for stateless Zookeeper interaction.\"\"\""},{"line_number":192,"context_line":""},{"line_number":193,"context_line":"    def __init__(self, client: ZooKeeperClient):"},{"line_number":194,"context_line":"        self.client \u003d client"},{"line_number":195,"context_line":""},{"line_number":196,"context_line":"    @property"},{"line_number":197,"context_line":"    def kazoo_client(self) -\u003e KazooClient:"}],"source_content_type":"text/x-python","patch_set":7,"id":"a94bbc98_4238e938","line":194,"updated":"2021-06-28 17:47:00.000000000","message":"The class below calls super() on __init__() and passes in the client but we don\u0027t guard against client being falsey here. Below the kazoo_client() property method assumes self.client is not None. Do we need to guard here?\n\nI think we only expect a None client when using a read only component. Not sure how that plays in here.","commit_id":"0ec943e7a1fb42e49f1e996578510dce8869228b"}],"zuul/zk/components.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"7c24c8b2f363d49f887a33c86ca1ad24de0bca04","unresolved":false,"context_lines":[{"line_number":53,"context_line":"            \"kind\": self.kind,"},{"line_number":54,"context_line":"        }"},{"line_number":55,"context_line":"        # NOTE (felix): If we want to have a \"read-only\" component, we could"},{"line_number":56,"context_line":"        # provide client\u003dNone to the constructor."},{"line_number":57,"context_line":"        super().__init__(client)"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"        self.path \u003d None"}],"source_content_type":"text/x-python","patch_set":4,"id":"fbe73f5b_2cb97102","line":56,"updated":"2021-06-21 18:02:53.000000000","message":"This is the comment that prompted this change.","commit_id":"120f9b473d9eb63dcc4d58fd965c210ee2778336"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"7c24c8b2f363d49f887a33c86ca1ad24de0bca04","unresolved":false,"context_lines":[{"line_number":258,"context_line":"                # Get the correct kind of component"},{"line_number":259,"context_line":"                # TODO (felix): KeyError on unknown component type?"},{"line_number":260,"context_line":"                component_cls \u003d self.COMPONENT_CLASSES[kind]"},{"line_number":261,"context_line":"                component \u003d component_cls.fromDict(None, hostname, d)"},{"line_number":262,"context_line":"                component.path \u003d self._getComponentPath(kind, hostname)"},{"line_number":263,"context_line":"                component._zstat \u003d stat"},{"line_number":264,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"60ca1b3c_0841f22d","line":261,"updated":"2021-06-21 18:02:53.000000000","message":"Correct.  The whole point of this change is to implement your suggestion of passing in \"None\" as the client in order to create a read-only component.  Users of the registry should be read-only; there\u0027s no case where the scheduler should write out data for one of the executors.","commit_id":"120f9b473d9eb63dcc4d58fd965c210ee2778336"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"2552b32865c5d7af97368773b99eefee21ffdc00","unresolved":true,"context_lines":[{"line_number":258,"context_line":"                # Get the correct kind of component"},{"line_number":259,"context_line":"                # TODO (felix): KeyError on unknown component type?"},{"line_number":260,"context_line":"                component_cls \u003d self.COMPONENT_CLASSES[kind]"},{"line_number":261,"context_line":"                component \u003d component_cls.fromDict(None, hostname, d)"},{"line_number":262,"context_line":"                component.path \u003d self._getComponentPath(kind, hostname)"},{"line_number":263,"context_line":"                component._zstat \u003d stat"},{"line_number":264,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"5f5559db_096fa6bd","line":261,"updated":"2021-06-18 06:37:29.000000000","message":"Is there a reason why we don\u0027t provide the client here? That would mean that we can\u0027t use a component from the registry to write/update data, or? But we could still set the client after we retrieved the component from the registry which would still re-enable this behaviour.","commit_id":"120f9b473d9eb63dcc4d58fd965c210ee2778336"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"2ccb773285283b18c243348f422f733a4758d80f","unresolved":false,"context_lines":[{"line_number":258,"context_line":"                # Get the correct kind of component"},{"line_number":259,"context_line":"                # TODO (felix): KeyError on unknown component type?"},{"line_number":260,"context_line":"                component_cls \u003d self.COMPONENT_CLASSES[kind]"},{"line_number":261,"context_line":"                component \u003d component_cls.fromDict(None, hostname, d)"},{"line_number":262,"context_line":"                component.path \u003d self._getComponentPath(kind, hostname)"},{"line_number":263,"context_line":"                component._zstat \u003d stat"},{"line_number":264,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"15b675ac_ccf922ed","line":261,"in_reply_to":"60ca1b3c_0841f22d","updated":"2021-06-23 12:18:32.000000000","message":"Ok. I think I left this comment, but didn\u0027t expect somebody to make use of it 😄\nIf we go this way it might make sense to add a comment (here and/or in the docstring of the ComponentRegistry class) that the registry by design only contains read-only components. I think that was also the case in the initial implementation but I didn\u0027t like the boilerplate of having additional classes and type checks for that and wanted to keep the new version simple.","commit_id":"120f9b473d9eb63dcc4d58fd965c210ee2778336"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"2c675230630887e55e1c26a83fa71eb800649f6f","unresolved":true,"context_lines":[{"line_number":263,"context_line":"                component._zstat \u003d stat"},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"            self._cached_components[kind][hostname] \u003d component"},{"line_number":266,"context_line":"        elif etype in (EventType.DELETED):"},{"line_number":267,"context_line":"            self.log.info("},{"line_number":268,"context_line":"                \"Noticed %s component %s disappeared\","},{"line_number":269,"context_line":"                kind, hostname)"}],"source_content_type":"text/x-python","patch_set":4,"id":"43c57a76_29f4be75","line":266,"updated":"2021-06-23 10:19:18.000000000","message":"This should be (missing comma to create a tuple):\n\n    elif etype in (EventType.DELETED,):\n\nOtherwise this is comparing `\"string\" in \"string\"`.","commit_id":"120f9b473d9eb63dcc4d58fd965c210ee2778336"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"2552b32865c5d7af97368773b99eefee21ffdc00","unresolved":true,"context_lines":[{"line_number":277,"context_line":""},{"line_number":278,"context_line":"    def all(self, kind\u003dNone):"},{"line_number":279,"context_line":"        if kind is None:"},{"line_number":280,"context_line":"            return [(kind, list(components.values()))"},{"line_number":281,"context_line":"                    for (kind, components) in self._cached_components.items()]"},{"line_number":282,"context_line":""},{"line_number":283,"context_line":"        # Filter the cached components for the given kind"}],"source_content_type":"text/x-python","patch_set":4,"id":"89e525e4_e6887c00","line":280,"updated":"2021-06-18 06:37:29.000000000","message":"That would mean that the return value of the method is different depending on whether we provide a kind or not, right? In case we provide a kind, we get a list of components. If no kind is provided, we would get a list of tuples (kind, \u003clist_of_components\u003e).","commit_id":"120f9b473d9eb63dcc4d58fd965c210ee2778336"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"7c24c8b2f363d49f887a33c86ca1ad24de0bca04","unresolved":false,"context_lines":[{"line_number":277,"context_line":""},{"line_number":278,"context_line":"    def all(self, kind\u003dNone):"},{"line_number":279,"context_line":"        if kind is None:"},{"line_number":280,"context_line":"            return [(kind, list(components.values()))"},{"line_number":281,"context_line":"                    for (kind, components) in self._cached_components.items()]"},{"line_number":282,"context_line":""},{"line_number":283,"context_line":"        # Filter the cached components for the given kind"}],"source_content_type":"text/x-python","patch_set":4,"id":"de08fa65_c95afa16","line":280,"updated":"2021-06-21 18:02:53.000000000","message":"Yes, that\u0027s correct.  That is simila to the case with the current code, which produces a list of dict value iterators if kind is None (compared to a single dict value iterator if kind is specified).\n\nActually, the current code produces a traceback, because it\u0027s untested and it has a bug.  But that appears to have been the intention.\n\nSo the new version is similar in spirit to what was intended orginially, except by *also* including the kind information, the caller has access to the grouped components.  Think about how this would be used in the web api or on the status page -- you\u0027re going to want a list of every type of component, and every component under it.","commit_id":"120f9b473d9eb63dcc4d58fd965c210ee2778336"},{"author":{"_account_id":27952,"name":"Felix Edel","email":"felix.edel@bmw.de","username":"felix.schmidt"},"change_message_id":"2ccb773285283b18c243348f422f733a4758d80f","unresolved":false,"context_lines":[{"line_number":277,"context_line":""},{"line_number":278,"context_line":"    def all(self, kind\u003dNone):"},{"line_number":279,"context_line":"        if kind is None:"},{"line_number":280,"context_line":"            return [(kind, list(components.values()))"},{"line_number":281,"context_line":"                    for (kind, components) in self._cached_components.items()]"},{"line_number":282,"context_line":""},{"line_number":283,"context_line":"        # Filter the cached components for the given kind"}],"source_content_type":"text/x-python","patch_set":4,"id":"9894dcaa_c5e9961e","line":280,"in_reply_to":"de08fa65_c95afa16","updated":"2021-06-23 12:18:32.000000000","message":"Ok, that sounds reasonable. Maybe we could add a comment here describing the different return types (or at least say that they are intentional). Although one can see the return value from the code I often like to have comments describing what\u0027s the intention behind (but maybe that\u0027s only me).\n\nTaking a look at the old version now, it looks like this is something I never followed up on.\nThanks for spotting the bug. That might indeed have been a path that wasn\u0027t used anywhere and thus I didn\u0027t spot the issue. Currently the only thing that comes to my mind requesting a list of all components (for all kinds) is the system overview page.","commit_id":"120f9b473d9eb63dcc4d58fd965c210ee2778336"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"f0b65ad17db857d9bc05d0c4ad56ab35e505897e","unresolved":true,"context_lines":[{"line_number":281,"context_line":"                # If it\u0027s already gone, don\u0027t care"},{"line_number":282,"context_line":"                pass"},{"line_number":283,"context_line":"            # Return False to stop the datawatch"},{"line_number":284,"context_line":"            return False"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"    def all(self, kind\u003dNone):"},{"line_number":287,"context_line":"        \"\"\"Returns a list of components."}],"source_content_type":"text/x-python","patch_set":7,"id":"a2e5b3dc_7fd32cd2","line":284,"updated":"2021-06-28 17:47:00.000000000","message":"Is False the specific type to stop the watch? I think we\u0027ll return None if we don\u0027t hit this elif. If any falsey return stops watching we may stop here?","commit_id":"0ec943e7a1fb42e49f1e996578510dce8869228b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"2a85b00ef9e1538ee22ad26b33d097e853136ba3","unresolved":false,"context_lines":[{"line_number":281,"context_line":"                # If it\u0027s already gone, don\u0027t care"},{"line_number":282,"context_line":"                pass"},{"line_number":283,"context_line":"            # Return False to stop the datawatch"},{"line_number":284,"context_line":"            return False"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"    def all(self, kind\u003dNone):"},{"line_number":287,"context_line":"        \"\"\"Returns a list of components."}],"source_content_type":"text/x-python","patch_set":7,"id":"5d80b0f7_f19a013a","line":284,"updated":"2021-06-28 17:54:05.000000000","message":"It\u0027s actual False; kazoo code is:\n  if result is False:","commit_id":"0ec943e7a1fb42e49f1e996578510dce8869228b"}]}