)]}'
{"roles/write-devstack-local-conf/library/devstack_local_conf.py":[{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"ec94d3c994379d41aff494177f4f203aef69a835","unresolved":false,"context_lines":[{"line_number":45,"context_line":"    def getDependenciesRecursively(self, parent):"},{"line_number":46,"context_line":"        dependencies \u003d []"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"        current_dependencies \u003d self._dependencies[parent]"},{"line_number":49,"context_line":"        for current in current_dependencies:"},{"line_number":50,"context_line":"            if current not in dependencies:"},{"line_number":51,"context_line":"                dependencies.append(current)"}],"source_content_type":"text/x-python","patch_set":5,"id":"df7087c5_6f9427e9","line":48,"updated":"2018-03-20 23:57:49.000000000","message":"Slight nit. The comment above says\n\n  # dependent_name -\u003e set(parent_names)\n\nSo the lookup here is actually on dependent (or child) not parent? I guess if we look at it as a tree the parent is the dependent and all the children are its dependencies? In that case maybe it is the comment that needs updating.","commit_id":"b449e40635b41e235b3cdffecdc5211f52e1c8f5"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"ec94d3c994379d41aff494177f4f203aef69a835","unresolved":false,"context_lines":[{"line_number":193,"context_line":""},{"line_number":194,"context_line":"    def getPlugins(self):"},{"line_number":195,"context_line":"        ret \u003d []"},{"line_number":196,"context_line":"        keys \u003d sorted(self.plugins.keys())"},{"line_number":197,"context_line":"        seen \u003d set()"},{"line_number":198,"context_line":"        for key in keys:"},{"line_number":199,"context_line":"            dependencies \u003d self.getDependenciesRecursively(key)"}],"source_content_type":"text/x-python","patch_set":5,"id":"df7087c5_922a047c","line":196,"updated":"2018-03-20 23:57:49.000000000","message":"See comment in test, we seem to be testing only this sorting behavior rather than the explicit settings defined plugin_requires.","commit_id":"b449e40635b41e235b3cdffecdc5211f52e1c8f5"}],"roles/write-devstack-local-conf/library/test.py":[{"author":{"_account_id":1921,"name":"Andrea Frittoli","email":"andrea.frittoli@gmail.com","username":"andrea-frittoli"},"change_message_id":"a749435f96e085442bd2c76ddd0c46ef3bd34991","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"df87a7cf_ec955288","updated":"2017-12-08 10:50:27.000000000","message":"I\u0027m happy to add tests here, but perhaps unit tests would work better? There is no expected result here, and it\u0027s not executed in the gate, so it\u0027s only useful for manual visual inspection.","commit_id":"fe975edddad2cf450a76afc90011fb454d4ddefe"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"ec94d3c994379d41aff494177f4f203aef69a835","unresolved":false,"context_lines":[{"line_number":42,"context_line":"            (\u0027bar\u0027, \u0027git://git.openstack.org/openstack/bar-plugin\u0027),"},{"line_number":43,"context_line":"            (\u0027foo\u0027, \u0027git://git.openstack.org/openstack/foo-plugin\u0027),"},{"line_number":44,"context_line":"            (\u0027baz\u0027, \u0027git://git.openstack.org/openstack/baz-plugin\u0027),"},{"line_number":45,"context_line":"            ])"},{"line_number":46,"context_line":"        p \u003d dict(localrc\u003dlocalrc,"},{"line_number":47,"context_line":"                 local_conf\u003dlocal_conf,"},{"line_number":48,"context_line":"                 base_services\u003d[],"}],"source_content_type":"text/x-python","patch_set":5,"id":"df7087c5_120d9434","line":45,"updated":"2018-03-20 23:57:49.000000000","message":"If I understand the ansible library addition the plugin dependencies are set in plugin specific settings files. I can\u0027t see where we are setting those settings for these plugins to affect their order.\n\nInstead it almost seems like we are testing that they get ordered by an alphabetical ordering?\n\nI\u0027m probably missing something important.","commit_id":"b449e40635b41e235b3cdffecdc5211f52e1c8f5"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"2d9c50e98efa046e11de90d46673459d314cb449","unresolved":false,"context_lines":[{"line_number":42,"context_line":"            (\u0027bar\u0027, \u0027git://git.openstack.org/openstack/bar-plugin\u0027),"},{"line_number":43,"context_line":"            (\u0027foo\u0027, \u0027git://git.openstack.org/openstack/foo-plugin\u0027),"},{"line_number":44,"context_line":"            (\u0027baz\u0027, \u0027git://git.openstack.org/openstack/baz-plugin\u0027),"},{"line_number":45,"context_line":"            ])"},{"line_number":46,"context_line":"        p \u003d dict(localrc\u003dlocalrc,"},{"line_number":47,"context_line":"                 local_conf\u003dlocal_conf,"},{"line_number":48,"context_line":"                 base_services\u003d[],"}],"source_content_type":"text/x-python","patch_set":5,"id":"df7087c5_72bbb0e4","line":45,"updated":"2018-03-21 00:24:10.000000000","message":"That\u0027s correct, this tests that the plugins are ordered alphabetically (which they should be if there\u0027s no explicit dependency).\n\nThe amount of test infrastructure needed to test explicit dependencies is significant.  Do you think it\u0027s worth it?","commit_id":"b449e40635b41e235b3cdffecdc5211f52e1c8f5"},{"author":{"_account_id":1921,"name":"Andrea Frittoli","email":"andrea.frittoli@gmail.com","username":"andrea-frittoli"},"change_message_id":"292369d34d0553fa5ca5a8c791cf88de6d8e3690","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                 plugins\u003dplugins,"},{"line_number":102,"context_line":"                 base_dir\u003dself.tmpdir,"},{"line_number":103,"context_line":"                 path\u003dos.path.join(self.tmpdir, \u0027test.local.conf\u0027))"},{"line_number":104,"context_line":"        lc \u003d LocalConf(p.get(\u0027localrc\u0027),"},{"line_number":105,"context_line":"                       p.get(\u0027local_conf\u0027),"},{"line_number":106,"context_line":"                       p.get(\u0027base_services\u0027),"},{"line_number":107,"context_line":"                       p.get(\u0027services\u0027),"},{"line_number":108,"context_line":"                       p.get(\u0027plugins\u0027),"},{"line_number":109,"context_line":"                       p.get(\u0027base_dir\u0027))"},{"line_number":110,"context_line":"        lc.write(p[\u0027path\u0027])"},{"line_number":111,"context_line":""},{"line_number":112,"context_line":"        plugins \u003d []"}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_12357e46","line":109,"range":{"start_line":104,"start_character":0,"end_line":109,"end_character":41},"updated":"2018-03-21 16:33:23.000000000","message":"If path was stored in a separate variable, could this be simply LocalConf(**p) ?","commit_id":"206fb267e6f3cc21738653679f8c45bb4bde9594"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"947aa127390acc95e76ec74cb6d761cd2f4fb257","unresolved":false,"context_lines":[{"line_number":106,"context_line":"                       p.get(\u0027base_services\u0027),"},{"line_number":107,"context_line":"                       p.get(\u0027services\u0027),"},{"line_number":108,"context_line":"                       p.get(\u0027plugins\u0027),"},{"line_number":109,"context_line":"                       p.get(\u0027base_dir\u0027))"},{"line_number":110,"context_line":"        lc.write(p[\u0027path\u0027])"},{"line_number":111,"context_line":""},{"line_number":112,"context_line":"        plugins \u003d []"}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_8674cb69","line":109,"updated":"2018-03-22 18:52:38.000000000","message":"We could, or we could just drop the p dict altogether.  However, this is structured this way so that the LocalConf invocation can be copied from the module itself.","commit_id":"206fb267e6f3cc21738653679f8c45bb4bde9594"}]}