)]}'
{"tripleoclient/command.py":[{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"4482c8ea6dec352ec1738725c8dbb9eddc5579df","unresolved":false,"context_lines":[{"line_number":28,"context_line":"        super(Command, self).run(parsed_args)"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"    @staticmethod"},{"line_number":31,"context_line":"    def get_roles_data(roles_file, templates, roles_data):"},{"line_number":32,"context_line":"        \"\"\"Load tripleo roles data for heat templates to use in deployments"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"        Fetch the roles data from the normalized roles_file path into"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_8697333b","line":31,"range":{"start_line":31,"start_character":46,"end_line":31,"end_character":56},"updated":"2018-06-06 14:55:55.000000000","message":"This is only relevant to a couple of commands, would it be cleaner to make it a new subclass, or perhaps a mixin?","commit_id":"ec497d9feaadc06180c56a307822fe2e1e85e87e"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"5ec43c36a4a7c62dfbdd0c253bfa778ec746b317","unresolved":false,"context_lines":[{"line_number":28,"context_line":"        super(Command, self).run(parsed_args)"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"    @staticmethod"},{"line_number":31,"context_line":"    def get_roles_data(roles_file, templates, roles_data):"},{"line_number":32,"context_line":"        \"\"\"Load tripleo roles data for heat templates to use in deployments"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"        Fetch the roles data from the normalized roles_file path into"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_58bf8aa9","line":31,"range":{"start_line":31,"start_character":46,"end_line":31,"end_character":56},"in_reply_to":"5f7c97a3_8697333b","updated":"2018-06-08 07:32:36.000000000","message":"There is a six or so places actually. Please take a look for the rev #2.","commit_id":"ec497d9feaadc06180c56a307822fe2e1e85e87e"},{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"4482c8ea6dec352ec1738725c8dbb9eddc5579df","unresolved":false,"context_lines":[{"line_number":52,"context_line":"                                                          templates)"},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"        # NOTE(bogdando): fetch roles data only once per a deployment class"},{"line_number":55,"context_line":"        # type that derives from this common client command class."},{"line_number":56,"context_line":"        if not roles_data:"},{"line_number":57,"context_line":"            with open(roles_file_normalized) as f:"},{"line_number":58,"context_line":"                roles_data \u003d yaml.safe_load(f)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_26bf3fab","line":55,"range":{"start_line":55,"start_character":59,"end_line":55,"end_character":60},"updated":"2018-06-06 14:55:55.000000000","message":"Not sure what this comment means - because this is a staticmethod we\u0027ve got no way to cache the loaded roles_data, so it\u0027ll be fetched whenever this function is called?","commit_id":"ec497d9feaadc06180c56a307822fe2e1e85e87e"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"3b5cfe4fad9b7d8eb5d1dbb2fb13ae6cc501d1dc","unresolved":false,"context_lines":[{"line_number":52,"context_line":"                                                          templates)"},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"        # NOTE(bogdando): fetch roles data only once per a deployment class"},{"line_number":55,"context_line":"        # type that derives from this common client command class."},{"line_number":56,"context_line":"        if not roles_data:"},{"line_number":57,"context_line":"            with open(roles_file_normalized) as f:"},{"line_number":58,"context_line":"                roles_data \u003d yaml.safe_load(f)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_2497f748","line":55,"range":{"start_line":55,"start_character":59,"end_line":55,"end_character":60},"in_reply_to":"5f7c97a3_26bf3fab","updated":"2018-07-17 08:24:21.000000000","message":"Done","commit_id":"ec497d9feaadc06180c56a307822fe2e1e85e87e"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"724fb3f026d90297aef81db014d38bb712aeb19b","unresolved":false,"context_lines":[{"line_number":52,"context_line":"                                                          templates)"},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"        # NOTE(bogdando): fetch roles data only once per a deployment class"},{"line_number":55,"context_line":"        # type that derives from this common client command class."},{"line_number":56,"context_line":"        if not roles_data:"},{"line_number":57,"context_line":"            with open(roles_file_normalized) as f:"},{"line_number":58,"context_line":"                roles_data \u003d yaml.safe_load(f)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_431d2e60","line":55,"range":{"start_line":55,"start_character":59,"end_line":55,"end_character":60},"in_reply_to":"5f7c97a3_26bf3fab","updated":"2018-06-11 13:52:49.000000000","message":"The unit test https://review.openstack.org/#/c/572404/3..4/tripleoclient/tests/v1/tripleo/test_tripleo_deploy.py@558 answers that question hopefully! Sorry if the wording used is confusing. Shall I rephrase?","commit_id":"ec497d9feaadc06180c56a307822fe2e1e85e87e"},{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"6768e26ace0bfb6008d2f741718ea32bf4b47a8d","unresolved":false,"context_lines":[{"line_number":24,"context_line":"class Command(command.Command):"},{"line_number":25,"context_line":"    \"\"\"Is heat roles data aware, represents a deployment/operation.\"\"\""},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"    roles_file \u003d None"},{"line_number":28,"context_line":"    roles_data \u003d None"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"    def run(self, parsed_args):"},{"line_number":31,"context_line":"        utils.store_cli_param(self.cmd_name, parsed_args)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_f48864c6","line":28,"range":{"start_line":27,"start_character":0,"end_line":28,"end_character":21},"updated":"2018-06-14 10:03:35.000000000","message":"The scope of these is the instance of the Command class, they should be initialized in the constructor - this will work by luck in this case but IMO it\u0027s a bad pattern.","commit_id":"97dac37a6110788319391d6b0c54d85f47ad5fc1"},{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"6768e26ace0bfb6008d2f741718ea32bf4b47a8d","unresolved":false,"context_lines":[{"line_number":32,"context_line":"        super(Command, self).run(parsed_args)"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    @classmethod"},{"line_number":35,"context_line":"    def get_roles_data(self, roles_file, templates):"},{"line_number":36,"context_line":"        \"\"\"Load tripleo roles data for heat templates to use in deployments"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        Fetch the heat roles data from the normalized roles_file path."}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_f4038440","line":35,"range":{"start_line":35,"start_character":8,"end_line":35,"end_character":22},"updated":"2018-06-14 10:03:35.000000000","message":"The naming of this is inconsistent with what it does - it\u0027s named like a java-ish getter method, but it just loads the class variable (which as I said before, should really be an instance variable initialized in the constructor).\n\nIMO this would be nicer if it was either a property, or get_roles_data returned the value, as right now you have to do two calls everywhere:\n\n  self.get_roles_data(...)\n  blah(self.roles_data)\n\nvs this if it was a @property\n\n    blah(self.roles_data)\n\nor this if it was a non-classmethod that returned instance variables:\n\n    blah(self.get_roles_data)","commit_id":"97dac37a6110788319391d6b0c54d85f47ad5fc1"},{"author":{"_account_id":11082,"name":"Kamil Sambor","email":"ksambor@redhat.com","username":"ksambor"},"change_message_id":"a8b138798497966068876e42cc00103520f085a3","unresolved":false,"context_lines":[{"line_number":49,"context_line":"         used for a particular deployment type."},{"line_number":50,"context_line":"        \"\"\""},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"        if self.roles_data is not None:"},{"line_number":53,"context_line":"            return"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"        self.roles_file \u003d utils.normalize_file_path(roles_file, templates)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_88ec0c93","line":52,"updated":"2018-07-02 13:46:41.000000000","message":"What if self.roles_data will be empty string? Then this value will be used in normalize_file_path and will return empty string. On the end we will try to open(\u0027\u0027). This is correct behavior?","commit_id":"36b229fff3d006a292da56dee7f12132a527d28d"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"98975b6f1b09c3a4493fc925dc97ee0a2932dbe3","unresolved":false,"context_lines":[{"line_number":49,"context_line":"         used for a particular deployment type."},{"line_number":50,"context_line":"        \"\"\""},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"        if self.roles_data is not None:"},{"line_number":53,"context_line":"            return"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"        self.roles_file \u003d utils.normalize_file_path(roles_file, templates)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_4810345a","line":52,"in_reply_to":"5f7c97a3_88ec0c93","updated":"2018-07-02 13:52:29.000000000","message":"It is an internal only state for the class. It is controlled via roles_file, which in turn can be defined only from the --roles-file parser options. I hope that works.","commit_id":"36b229fff3d006a292da56dee7f12132a527d28d"},{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"12e11379153c6c26887c9cb909cf49f093dba69b","unresolved":false,"context_lines":[{"line_number":32,"context_line":"        super(Command, self).run(parsed_args)"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    @classmethod"},{"line_number":35,"context_line":"    def get_roles_data(self, roles_file, templates):"},{"line_number":36,"context_line":"        \"\"\"Load tripleo roles data for heat templates to use in deployments"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        Fetch the heat roles data from the normalized roles_file path."}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_989c2c5c","line":35,"range":{"start_line":35,"start_character":24,"end_line":35,"end_character":26},"updated":"2018-07-27 09:13:57.000000000","message":"The convention for classmethods is cls, not self, but as I mentioned before this should just be an ordinary method or perhaps a property, and roles_file and roles_data should be initialized in the class constructor.\n\nFWIW I suspect that\u0027s the root cause of you earlier KeyError issues in the unit tests - because the tests run in parallel you have a potential race setting the class variables.","commit_id":"754369d4df0b500f2e2a4e6cc36c4c0e2d2d4f40"},{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"12e11379153c6c26887c9cb909cf49f093dba69b","unresolved":false,"context_lines":[{"line_number":54,"context_line":""},{"line_number":55,"context_line":"        self.roles_file \u003d utils.normalize_file_path(roles_file, templates)"},{"line_number":56,"context_line":"        with open(self.roles_file) as f:"},{"line_number":57,"context_line":"            self.roles_data \u003d yaml.safe_load(f)"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"class Lister(Command, command.Lister):"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_58bbf4bc","line":57,"range":{"start_line":57,"start_character":17,"end_line":57,"end_character":27},"updated":"2018-07-27 09:13:57.000000000","message":"My previous comment about this function name/behavior haven\u0027t been addressed - it\u0027s named like a getter but it doesn\u0027t return anything, I\u0027d like to see it either return roles_data, or perhaps more cleanly it can just be a property.","commit_id":"754369d4df0b500f2e2a4e6cc36c4c0e2d2d4f40"},{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"12e11379153c6c26887c9cb909cf49f093dba69b","unresolved":false,"context_lines":[{"line_number":32,"context_line":"        super(Command, self).run(parsed_args)"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    @classmethod"},{"line_number":35,"context_line":"    def get_roles_data(self, roles_file, templates):"},{"line_number":36,"context_line":"        \"\"\"Load tripleo roles data for heat templates to use in deployments"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        Fetch the heat roles data from the normalized roles_file path."}],"source_content_type":"text/x-python","patch_set":19,"id":"5f7c97a3_18081c8f","line":35,"range":{"start_line":35,"start_character":23,"end_line":35,"end_character":27},"updated":"2018-07-27 09:13:57.000000000","message":"The convention for classmethods is cls, not self, but as I previously mentioned this shouldn\u0027t be a classmethod, and the scope of the variables is wrong, they should be initialized in the class constructor and this should be an instance method or a property.\n\nI suspect this is the root-cause of the KeyError issues you had in the unit tests btw, because the tests run in parallel and race to overwrite the class-scope variables added here.","commit_id":"5ffab94dd66aa2e61e612059e27f5defcc18d14e"},{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"12e11379153c6c26887c9cb909cf49f093dba69b","unresolved":false,"context_lines":[{"line_number":54,"context_line":""},{"line_number":55,"context_line":"        self.roles_file \u003d utils.normalize_file_path(roles_file, templates)"},{"line_number":56,"context_line":"        with open(self.roles_file) as f:"},{"line_number":57,"context_line":"            self.roles_data \u003d yaml.safe_load(f)"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"class Lister(Command, command.Lister):"}],"source_content_type":"text/x-python","patch_set":19,"id":"5f7c97a3_b806c873","line":57,"range":{"start_line":57,"start_character":17,"end_line":57,"end_character":27},"updated":"2018-07-27 09:13:57.000000000","message":"The name of this function is still confusing - it\u0027s named like a getter function, but it always returns None. Sigh.\n\nPersonally I\u0027d use a property here, but if we\u0027re having a get_foo function, it\u0027d be nice for it to return foo.","commit_id":"5ffab94dd66aa2e61e612059e27f5defcc18d14e"}],"tripleoclient/config/standalone.py":[{"author":{"_account_id":14985,"name":"Alex Schultz","email":"aschultz@next-development.com","username":"mwhahaha"},"change_message_id":"852022ede65b6e3d8dc68801e82ef6095efcfbe2","unresolved":false,"context_lines":[{"line_number":153,"context_line":"                       default\u003dNone,"},{"line_number":154,"context_line":"                       help\u003d_(\u0027Roles file to override for heat. May be an \u0027"},{"line_number":155,"context_line":"                              \u0027absolute path or the path relative to the \u0027"},{"line_number":156,"context_line":"                              \u0027t-h-t templates directory used for deployment\u0027)"},{"line_number":157,"context_line":"                       ),"},{"line_number":158,"context_line":"            cfg.BoolOpt(\u0027heat_native\u0027,"},{"line_number":159,"context_line":"                        default\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":22,"id":"5f7c97a3_0f8a2f12","line":156,"range":{"start_line":156,"start_character":31,"end_line":156,"end_character":36},"updated":"2018-07-26 14:59:27.000000000","message":"nit: we should probably not shorten this as I\u0027m not sure folks outside of us devs know that tht \u003d tripleo-heat-templates","commit_id":"8f1caf7abf74fa418b0572aaed6aa6e8a9c5931a"}],"tripleoclient/tests/test_utils.py":[{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"aceb99c4b27db673ef0d2a4043464eb696ef7530","unresolved":false,"context_lines":[{"line_number":768,"context_line":"                stack\u003d\u0027foo-overcloud\u0027"},{"line_number":769,"context_line":"            )"},{"line_number":770,"context_line":""},{"line_number":771,"context_line":"\"\"\""},{"line_number":772,"context_line":"class TestGetRolesData(TestCase):"},{"line_number":773,"context_line":""},{"line_number":774,"context_line":"    @mock.patch(\u0027os.path.exists\u0027, return_value\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_1816f2a8","line":771,"range":{"start_line":771,"start_character":0,"end_line":771,"end_character":3},"updated":"2018-06-08 07:36:36.000000000","message":"is still WIP...","commit_id":"404fb95d89b92b8823277e6081d121edba66ef42"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"e0faa563c8d4c92c531a2677ab924d4ac2fc12b1","unresolved":false,"context_lines":[{"line_number":773,"context_line":"class TestNormalizeFilePath(TestCase):"},{"line_number":774,"context_line":""},{"line_number":775,"context_line":"    @mock.patch(\u0027os.path.isfile\u0027, return_value\u003dTrue)"},{"line_number":776,"context_line":"    def test_norm_path_abs(self, mock_exists):"},{"line_number":777,"context_line":"        self.assertEqual("},{"line_number":778,"context_line":"            utils.rel_or_abs_path(\u0027/foobar.yaml\u0027, \u0027/tmp\u0027),"},{"line_number":779,"context_line":"            \u0027/foobar.yaml\u0027)"}],"source_content_type":"text/x-python","patch_set":22,"id":"5f7c97a3_790b5f21","line":776,"updated":"2018-07-27 09:17:55.000000000","message":"the former test_set_roles_file* moved here","commit_id":"8f1caf7abf74fa418b0572aaed6aa6e8a9c5931a"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"e0faa563c8d4c92c531a2677ab924d4ac2fc12b1","unresolved":false,"context_lines":[{"line_number":788,"context_line":"class TestFetchRolesFile(TestCase):"},{"line_number":789,"context_line":""},{"line_number":790,"context_line":"    @mock.patch(\u0027os.path.exists\u0027, return_value\u003dTrue)"},{"line_number":791,"context_line":"    def test_fetch_roles_file(self, mock_exists):"},{"line_number":792,"context_line":"        with tempfile.NamedTemporaryFile(mode\u003d\u0027w\u0027) as roles_file:"},{"line_number":793,"context_line":"            yaml.dump([{\u0027name\u0027: \u0027Foobar\u0027}], roles_file)"},{"line_number":794,"context_line":"            with mock.patch(\u0027tripleoclient.utils.rel_or_abs_path\u0027) as mock_rf:"}],"source_content_type":"text/x-python","patch_set":22,"id":"5f7c97a3_d9fab30f","line":791,"updated":"2018-07-27 09:17:55.000000000","message":"the former test_get_roles_data* moved here, @Steven","commit_id":"8f1caf7abf74fa418b0572aaed6aa6e8a9c5931a"}],"tripleoclient/tests/v1/overcloud_deploy/test_overcloud_deploy.py":[{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"12e11379153c6c26887c9cb909cf49f093dba69b","unresolved":false,"context_lines":[{"line_number":1355,"context_line":"    @mock.patch(\"six.moves.builtins.open\")"},{"line_number":1356,"context_line":"    def test_get_default_role_counts_custom_roles(self, mock_open,"},{"line_number":1357,"context_line":"                                                  mock_safe_load):"},{"line_number":1358,"context_line":"        self.cmd.roles_data \u003d ["},{"line_number":1359,"context_line":"            {\u0027name\u0027: \u0027ControllerApi\u0027, \u0027CountDefault\u0027: 3},"},{"line_number":1360,"context_line":"            {\u0027name\u0027: \u0027ControllerPcmk\u0027, \u0027CountDefault\u0027: 3},"},{"line_number":1361,"context_line":"            {\u0027name\u0027: \u0027Compute\u0027, \u0027CountDefault\u0027: 3},"}],"source_content_type":"text/x-python","patch_set":19,"id":"5f7c97a3_18649c48","line":1358,"range":{"start_line":1358,"start_character":9,"end_line":1358,"end_character":27},"updated":"2018-07-27 09:13:57.000000000","message":"By setting the class variable here, you potentially conflict with other tests, e.g this should either remain a variable scoped to this test and returned via the mock yaml load, or we should do what I originally asked and scope the class variables to the instance, not the class.","commit_id":"5ffab94dd66aa2e61e612059e27f5defcc18d14e"}],"tripleoclient/tests/v1/tripleo/test_tripleo_deploy.py":[{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"a43142cc2cedae45f6f7b54a84309e0bf37856a3","unresolved":false,"context_lines":[{"line_number":439,"context_line":"                  mode\u003d\u0027w\u0027) as env_file:"},{"line_number":440,"context_line":"            yaml.dump({}, env_file)"},{"line_number":441,"context_line":"        cmd_extra \u003d [\u0027-p\u0027, plan_env_path]"},{"line_number":442,"context_line":"        self.cmd.roles_data \u003d [{\u0027name\u0027: \u0027Controller\u0027}]"},{"line_number":443,"context_line":"        self._setup_heat_environments(tmpdir, tht_from, plan_env_path,"},{"line_number":444,"context_line":"                                      mock_update_pass_env, mock_run,"},{"line_number":445,"context_line":"                                      cmd_extra, \u0027notenv.yaml\u0027)"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_53229b55","line":442,"updated":"2018-07-10 05:51:11.000000000","message":"I hope that fixes intermittent KeyErrors randomly reported by tox jobs :/","commit_id":"754369d4df0b500f2e2a4e6cc36c4c0e2d2d4f40"},{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"12e11379153c6c26887c9cb909cf49f093dba69b","unresolved":false,"context_lines":[{"line_number":82,"context_line":"            yaml.dump([{\u0027name\u0027: \u0027Foobar\u0027}], roles_file)"},{"line_number":83,"context_line":"            self.cmd.roles_file \u003d roles_file.name"},{"line_number":84,"context_line":"            self.assertEqual(self.cmd._get_roles_data(),"},{"line_number":85,"context_line":"                             [{\u0027name\u0027: \u0027Foobar\u0027}])"},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"    @mock.patch(\u0027os.path.exists\u0027, return_value\u003dFalse)"},{"line_number":88,"context_line":"    def test_get_roles_data_missing(self, mock_exists):"}],"source_content_type":"text/x-python","patch_set":22,"id":"5f7c97a3_2e759bc9","side":"PARENT","line":85,"range":{"start_line":85,"start_character":46,"end_line":85,"end_character":50},"updated":"2018-07-27 09:13:57.000000000","message":"Can these tests be reinstated, as I think they may have been removed to work around the race issues I mentioned?","commit_id":"a33b1499d7c00e646a9b49715a8a7dbd4467ec91"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"49c8436abb699bb9af7ebd259940f4466ab919da","unresolved":false,"context_lines":[{"line_number":82,"context_line":"            yaml.dump([{\u0027name\u0027: \u0027Foobar\u0027}], roles_file)"},{"line_number":83,"context_line":"            self.cmd.roles_file \u003d roles_file.name"},{"line_number":84,"context_line":"            self.assertEqual(self.cmd._get_roles_data(),"},{"line_number":85,"context_line":"                             [{\u0027name\u0027: \u0027Foobar\u0027}])"},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"    @mock.patch(\u0027os.path.exists\u0027, return_value\u003dFalse)"},{"line_number":88,"context_line":"    def test_get_roles_data_missing(self, mock_exists):"}],"source_content_type":"text/x-python","patch_set":22,"id":"5f7c97a3_593783e5","side":"PARENT","line":85,"range":{"start_line":85,"start_character":46,"end_line":85,"end_character":50},"in_reply_to":"5f7c97a3_2e759bc9","updated":"2018-07-27 09:18:20.000000000","message":"as those methods turned into a shared utility, the place for tests has been changed as well, see https://review.openstack.org/#/c/572404/22/tripleoclient/tests/test_utils.py@773","commit_id":"a33b1499d7c00e646a9b49715a8a7dbd4467ec91"}],"tripleoclient/utils.py":[{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"d699f6557733ac77b13b6b58cd73bf994a3f9367","unresolved":false,"context_lines":[{"line_number":1171,"context_line":"        raise ValueError(msg)"},{"line_number":1172,"context_line":""},{"line_number":1173,"context_line":""},{"line_number":1174,"context_line":"def rel_or_abs_path(tht_root, file_path):"},{"line_number":1175,"context_line":"    \u0027\u0027\u0027Find a file, either absolute path or relative to the t-h-t dir\u0027\u0027\u0027"},{"line_number":1176,"context_line":"    path \u003d os.path.abspath(file_path)"},{"line_number":1177,"context_line":"    if not os.path.exists(path):"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_3d1edcba","line":1174,"range":{"start_line":1174,"start_character":4,"end_line":1174,"end_character":19},"updated":"2018-06-13 09:36:59.000000000","message":"TODO this prolly should be leveraged in or merged with normalize_file_path()","commit_id":"77ef201bf796544cee3dc5e3308e0644e668f6ec"},{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"6768e26ace0bfb6008d2f741718ea32bf4b47a8d","unresolved":false,"context_lines":[{"line_number":1155,"context_line":"    if (abs_path !\u003d full_path and os.path.isfile(abs_path)"},{"line_number":1156,"context_line":"       and os.path.isfile(full_path)):"},{"line_number":1157,"context_line":"        log \u003d logging.getLogger(__name__ + \".normalize_file_path\")"},{"line_number":1158,"context_line":"        log.warning("},{"line_number":1159,"context_line":"            _(\u0027An ambigous file {0} mathches both {1} and {2} paths\u0027).format("},{"line_number":1160,"context_line":"                f, abs_path, full_path))"},{"line_number":1161,"context_line":"        return f"},{"line_number":1162,"context_line":"    elif os.path.isfile(full_path):"},{"line_number":1163,"context_line":"        return full_path"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_34425c69","line":1160,"range":{"start_line":1158,"start_character":0,"end_line":1160,"end_character":40},"updated":"2018-06-14 10:03:35.000000000","message":"IMO this isn\u0027t a valid warning, consider the following:\n\n  cd\n  cp tripleo-heat-templates/roles_data.yaml ./roles_data.yaml\n   \u003cedit roles_data.yaml\u003e\n  openstack overcloud deploy --templates tripleo-heat-templates -r ./roles_data.yaml\n\nThat isn\u0027t wrong, and it\u0027s how roles_data.yaml was always used prior to the openstack overcloud roles generate file","commit_id":"97dac37a6110788319391d6b0c54d85f47ad5fc1"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"99abe40cd4c6aaddc79758225fc44689b40e7189","unresolved":false,"context_lines":[{"line_number":1155,"context_line":"    if (abs_path !\u003d full_path and os.path.isfile(abs_path)"},{"line_number":1156,"context_line":"       and os.path.isfile(full_path)):"},{"line_number":1157,"context_line":"        log \u003d logging.getLogger(__name__ + \".normalize_file_path\")"},{"line_number":1158,"context_line":"        log.warning("},{"line_number":1159,"context_line":"            _(\u0027An ambigous file {0} mathches both {1} and {2} paths\u0027).format("},{"line_number":1160,"context_line":"                f, abs_path, full_path))"},{"line_number":1161,"context_line":"        return f"},{"line_number":1162,"context_line":"    elif os.path.isfile(full_path):"},{"line_number":1163,"context_line":"        return full_path"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_cd3ef227","line":1160,"range":{"start_line":1158,"start_character":0,"end_line":1160,"end_character":40},"in_reply_to":"5f7c97a3_34425c69","updated":"2018-06-14 15:29:36.000000000","message":"Hm, this only warns about potentially different roles files located by a two paths, so the one originally passed in takes a precedence","commit_id":"97dac37a6110788319391d6b0c54d85f47ad5fc1"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"de53f17d7c38d792d8ba81d9fb7c68c746c58e91","unresolved":false,"context_lines":[{"line_number":1155,"context_line":"    if (abs_path !\u003d full_path and os.path.isfile(abs_path)"},{"line_number":1156,"context_line":"       and os.path.isfile(full_path)):"},{"line_number":1157,"context_line":"        log \u003d logging.getLogger(__name__ + \".normalize_file_path\")"},{"line_number":1158,"context_line":"        log.warning("},{"line_number":1159,"context_line":"            _(\u0027An ambigous file {0} mathches both {1} and {2} paths\u0027).format("},{"line_number":1160,"context_line":"                f, abs_path, full_path))"},{"line_number":1161,"context_line":"        return f"},{"line_number":1162,"context_line":"    elif os.path.isfile(full_path):"},{"line_number":1163,"context_line":"        return full_path"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_6d66be2b","line":1160,"range":{"start_line":1158,"start_character":0,"end_line":1160,"end_character":40},"in_reply_to":"5f7c97a3_cd3ef227","updated":"2018-06-14 15:38:47.000000000","message":"not sure tripleo-heat-templates/roles_data.yaml in the example would trigger a warning. We need a unit test...","commit_id":"97dac37a6110788319391d6b0c54d85f47ad5fc1"},{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"6768e26ace0bfb6008d2f741718ea32bf4b47a8d","unresolved":false,"context_lines":[{"line_number":1171,"context_line":"        raise ValueError(msg)"},{"line_number":1172,"context_line":""},{"line_number":1173,"context_line":""},{"line_number":1174,"context_line":"def rel_or_abs_path(tht_root, file_path):"},{"line_number":1175,"context_line":"    \u0027\u0027\u0027Find a file, either absolute path or relative to the t-h-t dir\u0027\u0027\u0027"},{"line_number":1176,"context_line":"    path \u003d os.path.abspath(file_path)"},{"line_number":1177,"context_line":"    if not os.path.exists(path):"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_3437bc00","line":1174,"range":{"start_line":1174,"start_character":4,"end_line":1174,"end_character":19},"updated":"2018-06-14 10:03:35.000000000","message":"Can\u0027t we just use this instead of the above as it basically does the same but without the warning?\n\nI know this was posted first, sorry for that, but it\u0027d be good to avoid duplication since it\u0027s now landed?","commit_id":"97dac37a6110788319391d6b0c54d85f47ad5fc1"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"e0f6f047995a4a76f2cadfaa0e16786a16a51a4c","unresolved":false,"context_lines":[{"line_number":1171,"context_line":"        raise ValueError(msg)"},{"line_number":1172,"context_line":""},{"line_number":1173,"context_line":""},{"line_number":1174,"context_line":"def rel_or_abs_path(tht_root, file_path):"},{"line_number":1175,"context_line":"    \u0027\u0027\u0027Find a file, either absolute path or relative to the t-h-t dir\u0027\u0027\u0027"},{"line_number":1176,"context_line":"    path \u003d os.path.abspath(file_path)"},{"line_number":1177,"context_line":"    if not os.path.exists(path):"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_ed83aea7","line":1174,"range":{"start_line":1174,"start_character":4,"end_line":1174,"end_character":19},"in_reply_to":"5f7c97a3_3437bc00","updated":"2018-06-14 15:36:42.000000000","message":"I\u0027d prefer to check against isfile instead for some reasons. Would be nice to remove this one and use the above method instead.","commit_id":"97dac37a6110788319391d6b0c54d85f47ad5fc1"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"9946c4e2f7458a011b3cc99ef2ab56b73aef4d79","unresolved":false,"context_lines":[{"line_number":1148,"context_line":"    path \u003d os.path.abspath(file_path)"},{"line_number":1149,"context_line":"    if not os.path.isfile(path):"},{"line_number":1150,"context_line":"        path \u003d os.path.abspath(os.path.join(tht_root, file_path))"},{"line_number":1151,"context_line":"    if not os.path.isfile(path):"},{"line_number":1152,"context_line":"        raise exceptions.DeploymentError("},{"line_number":1153,"context_line":"            \"Can\u0027t find path %s %s\" % (file_path, path))"},{"line_number":1154,"context_line":"    return path"}],"source_content_type":"text/x-python","patch_set":21,"id":"5f7c97a3_369d1209","line":1151,"updated":"2018-07-27 06:22:57.000000000","message":"this is switched for easier testing. There are too many os.path.exists mock levels otherwise.","commit_id":"068273095b4673b8245756adfc47bcd3020120c6"}],"tripleoclient/v1/container_image.py":[{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"4482c8ea6dec352ec1738725c8dbb9eddc5579df","unresolved":false,"context_lines":[{"line_number":378,"context_line":"        # populates roles_data from the roles_file, which is updated"},{"line_number":379,"context_line":"        # with a normalized path to the roles file passed from args"},{"line_number":380,"context_line":"        PrepareImageFiles.get_roles_data("},{"line_number":381,"context_line":"            parsed_args.roles_file, constants.TRIPLEO_HEAT_TEMPLATES,"},{"line_number":382,"context_line":"            roles_data)"},{"line_number":383,"context_line":""},{"line_number":384,"context_line":"        env \u003d utils.build_prepare_env("}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_463d9b2a","line":381,"range":{"start_line":381,"start_character":12,"end_line":381,"end_character":23},"updated":"2018-06-06 14:55:55.000000000","message":"Would this interface be cleaner if it wasn\u0027t a staticmethod?","commit_id":"ec497d9feaadc06180c56a307822fe2e1e85e87e"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"a923ebaf2bf88ed00c9155e81b69869f9a9bc455","unresolved":false,"context_lines":[{"line_number":378,"context_line":"        # populates roles_data from the roles_file, which is updated"},{"line_number":379,"context_line":"        # with a normalized path to the roles file passed from args"},{"line_number":380,"context_line":"        PrepareImageFiles.get_roles_data("},{"line_number":381,"context_line":"            parsed_args.roles_file, constants.TRIPLEO_HEAT_TEMPLATES,"},{"line_number":382,"context_line":"            roles_data)"},{"line_number":383,"context_line":""},{"line_number":384,"context_line":"        env \u003d utils.build_prepare_env("}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_446d5365","line":381,"range":{"start_line":381,"start_character":12,"end_line":381,"end_character":23},"in_reply_to":"5f7c97a3_463d9b2a","updated":"2018-07-17 08:26:44.000000000","message":"Done","commit_id":"ec497d9feaadc06180c56a307822fe2e1e85e87e"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"64f4e3f564620c3ea6c84c15d434cc9010abbb84","unresolved":false,"context_lines":[{"line_number":378,"context_line":"        # populates roles_data from the roles_file, which is updated"},{"line_number":379,"context_line":"        # with a normalized path to the roles file passed from args"},{"line_number":380,"context_line":"        PrepareImageFiles.get_roles_data("},{"line_number":381,"context_line":"            parsed_args.roles_file, constants.TRIPLEO_HEAT_TEMPLATES,"},{"line_number":382,"context_line":"            roles_data)"},{"line_number":383,"context_line":""},{"line_number":384,"context_line":"        env \u003d utils.build_prepare_env("}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_b85a5e80","line":381,"range":{"start_line":381,"start_character":12,"end_line":381,"end_character":23},"in_reply_to":"5f7c97a3_463d9b2a","updated":"2018-06-08 07:39:14.000000000","message":"I\u0027m really not sure which would be the best approach for that...","commit_id":"ec497d9feaadc06180c56a307822fe2e1e85e87e"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"a6a5e0fd3ae63724d959dc13e5fd8e66ecdf65a0","unresolved":false,"context_lines":[{"line_number":86,"context_line":"    auth_required \u003d False"},{"line_number":87,"context_line":"    log \u003d logging.getLogger(__name__ + \".BuildImage\")"},{"line_number":88,"context_line":"    roles_file \u003d None"},{"line_number":89,"context_line":"    roles_data \u003d None"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"    @staticmethod"},{"line_number":92,"context_line":"    def images_from_deps(images, dep):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_184ef295","line":89,"updated":"2018-06-08 07:50:58.000000000","message":"seem not needed for this class","commit_id":"404fb95d89b92b8823277e6081d121edba66ef42"},{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"6768e26ace0bfb6008d2f741718ea32bf4b47a8d","unresolved":false,"context_lines":[{"line_number":185,"context_line":"    def get_parser(self, prog_name):"},{"line_number":186,"context_line":"        parser \u003d super(PrepareImageFiles, self).get_parser(prog_name)"},{"line_number":187,"context_line":"        try:"},{"line_number":188,"context_line":"            self.roles_file \u003d utils.normalize_file_path("},{"line_number":189,"context_line":"                constants.OVERCLOUD_ROLES_FILE,"},{"line_number":190,"context_line":"                constants.TRIPLEO_HEAT_TEMPLATES)"},{"line_number":191,"context_line":"        except ValueError:"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_b42aec8d","line":188,"range":{"start_line":188,"start_character":36,"end_line":188,"end_character":55},"updated":"2018-06-14 10:03:35.000000000","message":"Why isn\u0027t this using the self.get_roles_data getter?","commit_id":"97dac37a6110788319391d6b0c54d85f47ad5fc1"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"c66e13263a73a9849663cfcbdeeb54679b6367d0","unresolved":false,"context_lines":[{"line_number":185,"context_line":"    def get_parser(self, prog_name):"},{"line_number":186,"context_line":"        parser \u003d super(PrepareImageFiles, self).get_parser(prog_name)"},{"line_number":187,"context_line":"        try:"},{"line_number":188,"context_line":"            self.roles_file \u003d utils.normalize_file_path("},{"line_number":189,"context_line":"                constants.OVERCLOUD_ROLES_FILE,"},{"line_number":190,"context_line":"                constants.TRIPLEO_HEAT_TEMPLATES)"},{"line_number":191,"context_line":"        except ValueError:"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_ad3ff61f","line":188,"range":{"start_line":188,"start_character":36,"end_line":188,"end_character":55},"in_reply_to":"5f7c97a3_b42aec8d","updated":"2018-06-14 15:30:43.000000000","message":"it takes this later. I was not sure if get_parser is a good place for doing that. I can fix that.","commit_id":"97dac37a6110788319391d6b0c54d85f47ad5fc1"},{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"6768e26ace0bfb6008d2f741718ea32bf4b47a8d","unresolved":false,"context_lines":[{"line_number":386,"context_line":"            # populates roles_data from the roles_file, which is updated"},{"line_number":387,"context_line":"            # with a normalized path to the roles file passed from args"},{"line_number":388,"context_line":"            PrepareImageFiles.get_roles_data("},{"line_number":389,"context_line":"                parsed_args.roles_file, constants.TRIPLEO_HEAT_TEMPLATES)"},{"line_number":390,"context_line":"        else:"},{"line_number":391,"context_line":"            self.roles_data \u003d set()"},{"line_number":392,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_d4fe8804","line":389,"range":{"start_line":389,"start_character":16,"end_line":389,"end_character":27},"updated":"2018-06-14 10:03:35.000000000","message":"This could just be:\n\n  roles_file \u003d utils.rel_or_abs_path(parsed_args.roles_file, constants.TRIPLEO_HEAT_TEMPLATES)\n  roles_data \u003d yaml.safe_load(open(roles_file).read())\n\nWhich is arguably simpler than the base-class getter stuff?\n\nOr, if want to keep the base-class stuff but make roles_data a property, you don\u0027t need this section at all, you can just referece the property below?","commit_id":"97dac37a6110788319391d6b0c54d85f47ad5fc1"},{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"6768e26ace0bfb6008d2f741718ea32bf4b47a8d","unresolved":false,"context_lines":[{"line_number":545,"context_line":"    def get_parser(self, prog_name):"},{"line_number":546,"context_line":"        parser \u003d super(TripleOImagePrepare, self).get_parser(prog_name)"},{"line_number":547,"context_line":"        try:"},{"line_number":548,"context_line":"            self.roles_file \u003d utils.normalize_file_path("},{"line_number":549,"context_line":"                constants.OVERCLOUD_ROLES_FILE,"},{"line_number":550,"context_line":"                constants.TRIPLEO_HEAT_TEMPLATES)"},{"line_number":551,"context_line":"        except ValueError:"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_281e7c5e","line":548,"range":{"start_line":548,"start_character":36,"end_line":548,"end_character":55},"updated":"2018-06-14 10:03:35.000000000","message":"Why call this directly vs calling the base-class getter and catching a known exception when the file is missing?","commit_id":"97dac37a6110788319391d6b0c54d85f47ad5fc1"}],"tripleoclient/v1/overcloud_deploy.py":[{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"4482c8ea6dec352ec1738725c8dbb9eddc5579df","unresolved":false,"context_lines":[{"line_number":50,"context_line":"    predeploy_warnings \u003d 0"},{"line_number":51,"context_line":"    _password_cache \u003d None"},{"line_number":52,"context_line":"    roles_file \u003d None"},{"line_number":53,"context_line":"    roles_data \u003d None"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    # This may be switched on by default in the future, but for now"},{"line_number":56,"context_line":"    # we\u0027ll want this behavior only in `overcloud update stack` and"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_2642dfac","line":53,"range":{"start_line":53,"start_character":17,"end_line":53,"end_character":21},"updated":"2018-06-06 14:55:55.000000000","message":"I don\u0027t think these should be class variables?","commit_id":"ec497d9feaadc06180c56a307822fe2e1e85e87e"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"a923ebaf2bf88ed00c9155e81b69869f9a9bc455","unresolved":false,"context_lines":[{"line_number":50,"context_line":"    predeploy_warnings \u003d 0"},{"line_number":51,"context_line":"    _password_cache \u003d None"},{"line_number":52,"context_line":"    roles_file \u003d None"},{"line_number":53,"context_line":"    roles_data \u003d None"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    # This may be switched on by default in the future, but for now"},{"line_number":56,"context_line":"    # we\u0027ll want this behavior only in `overcloud update stack` and"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_04bafbbe","line":53,"range":{"start_line":53,"start_character":17,"end_line":53,"end_character":21},"in_reply_to":"5f7c97a3_2642dfac","updated":"2018-07-17 08:26:44.000000000","message":"Done","commit_id":"ec497d9feaadc06180c56a307822fe2e1e85e87e"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"b24389a8acbc36e9a36a71a3d3d387cb5f7f87b6","unresolved":false,"context_lines":[{"line_number":50,"context_line":"    predeploy_warnings \u003d 0"},{"line_number":51,"context_line":"    _password_cache \u003d None"},{"line_number":52,"context_line":"    roles_file \u003d None"},{"line_number":53,"context_line":"    roles_data \u003d None"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    # This may be switched on by default in the future, but for now"},{"line_number":56,"context_line":"    # we\u0027ll want this behavior only in `overcloud update stack` and"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_78028640","line":53,"range":{"start_line":53,"start_character":17,"end_line":53,"end_character":21},"in_reply_to":"5f7c97a3_2642dfac","updated":"2018-06-08 07:41:26.000000000","message":"the thing is I thought of those as a singletons, exclusive for a class type (a deployment or operation type). So we fetch them only once and keep them immutable for each of the cases. At least, the unit tests seem confirming it works?..","commit_id":"ec497d9feaadc06180c56a307822fe2e1e85e87e"},{"author":{"_account_id":4328,"name":"Steven Hardy","email":"steven.hardy@suse.com","username":"shardy"},"change_message_id":"0c4d933ad62e51a7f4817e5011a70450435d62dd","unresolved":false,"context_lines":[{"line_number":50,"context_line":"    predeploy_warnings \u003d 0"},{"line_number":51,"context_line":"    _password_cache \u003d None"},{"line_number":52,"context_line":"    roles_file \u003d None"},{"line_number":53,"context_line":"    roles_data \u003d None"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    # This may be switched on by default in the future, but for now"},{"line_number":56,"context_line":"    # we\u0027ll want this behavior only in `overcloud update stack` and"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_34529768","line":53,"range":{"start_line":53,"start_character":17,"end_line":53,"end_character":21},"in_reply_to":"5f7c97a3_78028640","updated":"2018-06-12 09:14:52.000000000","message":"It works in the tests, and in the CLI because we happen to only ever instantiate one DeployOvercloud class - it\u0027s just a bad pattern IMO as anywhere else you\u0027d design so mutable class data like this is initialized in the constructor.\n\nIt\u0027s not a singleton, it\u0027s a variable associated with this specific class instance.\n\nNot blocking though as we know, luckily, it\u0027ll work OK in the context of thish single-object usage in openstackclient.","commit_id":"ec497d9feaadc06180c56a307822fe2e1e85e87e"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"a6a5e0fd3ae63724d959dc13e5fd8e66ecdf65a0","unresolved":false,"context_lines":[{"line_number":906,"context_line":"        # populates roles_data from the roles_file, which is updated"},{"line_number":907,"context_line":"        # with a normalized path to the roles file passed from args"},{"line_number":908,"context_line":"        if parsed_args.roles_file:"},{"line_number":909,"context_line":"            self.roles_file \u003d DeployOvercloud.get_roles_data("},{"line_number":910,"context_line":"                parsed_args.roles_file, parsed_args.templates,"},{"line_number":911,"context_line":"                self.roles_data)"},{"line_number":912,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_58530a3a","line":909,"range":{"start_line":909,"start_character":30,"end_line":909,"end_character":60},"updated":"2018-06-08 07:50:58.000000000","message":"@Steven, I found this interface quite fancy. So each of the roles aware classes know how to normalize paths and fetch roles data from a file. Just think of it as a class property.","commit_id":"404fb95d89b92b8823277e6081d121edba66ef42"}],"tripleoclient/v1/tripleo_deploy.py":[{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"f2b4e7765290ce5b38c87a56341b56b2cebdaa51","unresolved":false,"context_lines":[{"line_number":33,"context_line":"from osc_lib.i18n import _"},{"line_number":34,"context_line":"from six.moves import configparser"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"from tripleoclient import command"},{"line_number":37,"context_line":"from tripleoclient import constants"},{"line_number":38,"context_line":"from tripleoclient import exceptions"},{"line_number":39,"context_line":"from tripleoclient import heat_launcher"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_b8125e61","line":36,"range":{"start_line":36,"start_character":0,"end_line":36,"end_character":33},"updated":"2018-06-08 07:52:47.000000000","message":"dunno why it was cliff here, but I think it needs to be the tripleo command instead. And it needs to know how to get roles data here as well.","commit_id":"404fb95d89b92b8823277e6081d121edba66ef42"}],"tripleoclient/workflows/plan_management.py":[{"author":{"_account_id":11082,"name":"Kamil Sambor","email":"ksambor@redhat.com","username":"ksambor"},"change_message_id":"a8b138798497966068876e42cc00103520f085a3","unresolved":false,"context_lines":[{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    print(\"Creating plan from template files in: {}\".format(tht_root))"},{"line_number":162,"context_line":"    if roles_file:"},{"line_number":163,"context_line":"        roles_file_normalized \u003d utils.normalize_file_path(roles_file, tht_root)"},{"line_number":164,"context_line":"    else:"},{"line_number":165,"context_line":"        roles_file_normalized \u003d None"},{"line_number":166,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_28193822","line":163,"updated":"2018-07-02 13:46:41.000000000","message":"You can return None inside normalize_file_path. You already check the same statement inside normalize_file_path (https://review.openstack.org/#/c/572404/9/tripleoclient/utils.py). If you will return None from normalize_file_path, then this lines can be changed into: \u0027\u0027\u0027roles_file_normalized \u003d utils.normalize_file_path(roles_file, tht_root)\u0027\u0027\u0027\nThis will simplified a little bit","commit_id":"36b229fff3d006a292da56dee7f12132a527d28d"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"d3172289798250d658310696c6465b0521f02ef5","unresolved":false,"context_lines":[{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    print(\"Creating plan from template files in: {}\".format(tht_root))"},{"line_number":162,"context_line":"    if roles_file:"},{"line_number":163,"context_line":"        roles_file_normalized \u003d utils.normalize_file_path(roles_file, tht_root)"},{"line_number":164,"context_line":"    else:"},{"line_number":165,"context_line":"        roles_file_normalized \u003d None"},{"line_number":166,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_23339bf9","line":163,"in_reply_to":"5f7c97a3_28193822","updated":"2018-07-02 13:55:31.000000000","message":"good point! Let\u0027s try that in the follow up patch, if you don\u0027t mind.","commit_id":"36b229fff3d006a292da56dee7f12132a527d28d"}]}