)]}'
{"mistral/services/actions.py":[{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"3cceac75853727f7a22750a97f2610e3a6258a77","unresolved":false,"context_lines":[{"line_number":42,"context_line":"                action_list_spec,"},{"line_number":43,"context_line":"                db_actions,"},{"line_number":44,"context_line":"                definition,"},{"line_number":45,"context_line":"                scope, update_action"},{"line_number":46,"context_line":"            )"},{"line_number":47,"context_line":"    else:"},{"line_number":48,"context_line":"        db_actions \u003d _append_all_actions("}],"source_content_type":"text/x-python","patch_set":2,"id":"5a9d85d2_0f5d93e9","line":45,"range":{"start_line":45,"start_character":23,"end_line":45,"end_character":36},"updated":"2016-06-20 09:05:13.000000000","message":"Style: please have \"update_action\" param on a new line.","commit_id":"fa8bb82d50e0d73794b7e5a44cc0992af27464f3"},{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"3cceac75853727f7a22750a97f2610e3a6258a77","unresolved":false,"context_lines":[{"line_number":49,"context_line":"            action_list_spec,"},{"line_number":50,"context_line":"            db_actions,"},{"line_number":51,"context_line":"            definition,"},{"line_number":52,"context_line":"            scope, update_action"},{"line_number":53,"context_line":"        )"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    return db_actions"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a9d85d2_4f6f9b92","line":52,"range":{"start_line":52,"start_character":19,"end_line":52,"end_character":32},"updated":"2016-06-20 09:05:13.000000000","message":"Same","commit_id":"fa8bb82d50e0d73794b7e5a44cc0992af27464f3"},{"author":{"_account_id":9712,"name":"Dougal Matthews","email":"dougal.matthews@canonical.com","username":"dougal"},"change_message_id":"a9e171c780704dfaa86f8e800246410e90e7e3c3","unresolved":false,"context_lines":[{"line_number":60,"context_line":""},{"line_number":61,"context_line":"    db_actions \u003d []"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    if run_in_tx:"},{"line_number":64,"context_line":"        with db_api.transaction():"},{"line_number":65,"context_line":"            db_actions \u003d _append_all_actions("},{"line_number":66,"context_line":"                action_list_spec,"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a9d85d2_6f96d795","line":63,"updated":"2016-06-20 08:44:17.000000000","message":"I think we can probably remove this if statement here and above. From what I can tell, only mistral.services.action_manager called it with transactions disabled. So we can make this simpler (or leave it like this.)","commit_id":"fa8bb82d50e0d73794b7e5a44cc0992af27464f3"},{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"3cceac75853727f7a22750a97f2610e3a6258a77","unresolved":false,"context_lines":[{"line_number":60,"context_line":""},{"line_number":61,"context_line":"    db_actions \u003d []"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    if run_in_tx:"},{"line_number":64,"context_line":"        with db_api.transaction():"},{"line_number":65,"context_line":"            db_actions \u003d _append_all_actions("},{"line_number":66,"context_line":"                action_list_spec,"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a9d85d2_af76ff3b","line":63,"in_reply_to":"5a9d85d2_6f96d795","updated":"2016-06-20 09:05:13.000000000","message":"Well, it looks kinda weird. I\u0027d like to get rid of that param. IMO, this helper method shouldn\u0027t be aware of any transactional context. It\u0027s a caller who know about it and it\u0027s caller\u0027s responsibility to start a TX if needed.\n\nIn other words, this method is not a good place to make this decision.","commit_id":"fa8bb82d50e0d73794b7e5a44cc0992af27464f3"},{"author":{"_account_id":9712,"name":"Dougal Matthews","email":"dougal.matthews@canonical.com","username":"dougal"},"change_message_id":"17e8dbcbf1a90aa876154f26ecf098a45e571bdf","unresolved":false,"context_lines":[{"line_number":60,"context_line":""},{"line_number":61,"context_line":"    db_actions \u003d []"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    if run_in_tx:"},{"line_number":64,"context_line":"        with db_api.transaction():"},{"line_number":65,"context_line":"            db_actions \u003d _append_all_actions("},{"line_number":66,"context_line":"                action_list_spec,"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a9d85d2_30a55004","line":63,"in_reply_to":"5a9d85d2_af76ff3b","updated":"2016-06-20 10:09:10.000000000","message":"Agreed. I\u0027ll try and remove it at the same time.","commit_id":"fa8bb82d50e0d73794b7e5a44cc0992af27464f3"},{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"3cceac75853727f7a22750a97f2610e3a6258a77","unresolved":false,"context_lines":[{"line_number":82,"context_line":"def _append_all_actions(action_list_spec, db_actions, definition, scope, fn):"},{"line_number":83,"context_line":"    for action_spec in action_list_spec.get_actions():"},{"line_number":84,"context_line":"        db_actions.append("},{"line_number":85,"context_line":"            fn("},{"line_number":86,"context_line":"                action_spec,"},{"line_number":87,"context_line":"                definition,"},{"line_number":88,"context_line":"                scope"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a9d85d2_6f26d717","line":85,"range":{"start_line":85,"start_character":14,"end_line":85,"end_character":15},"updated":"2016-06-20 09:05:13.000000000","message":"It seems like these params will fit into 79 chars if placed in one line. Please check.","commit_id":"fa8bb82d50e0d73794b7e5a44cc0992af27464f3"},{"author":{"_account_id":6732,"name":"Lingxian Kong","email":"anlin.kong@gmail.com","username":"kong"},"change_message_id":"c978dd1a69f3a31d6f38f88d4b6a9fd3b350f93f","unresolved":false,"context_lines":[{"line_number":24,"context_line":""},{"line_number":25,"context_line":"    db_actions \u003d []"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"    with db_api.transaction():"},{"line_number":28,"context_line":"        for action_spec in action_list_spec.get_actions():"},{"line_number":29,"context_line":"            db_actions.append(create_action(action_spec, definition, scope))"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"1aa78d24_0ad10ece","side":"PARENT","line":27,"updated":"2016-07-02 11:54:15.000000000","message":"I see no reason to remove this line. and Ｉ rather think the transaction should live in the service layer instead of api layer.","commit_id":"9d00052f617c22b87a98320df3ee4f0f457fe828"},{"author":{"_account_id":9712,"name":"Dougal Matthews","email":"dougal.matthews@canonical.com","username":"dougal"},"change_message_id":"052e452ba7489525a825e249654c97bd6c1773cf","unresolved":false,"context_lines":[{"line_number":24,"context_line":""},{"line_number":25,"context_line":"    db_actions \u003d []"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"    with db_api.transaction():"},{"line_number":28,"context_line":"        for action_spec in action_list_spec.get_actions():"},{"line_number":29,"context_line":"            db_actions.append(create_action(action_spec, definition, scope))"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"1aa78d24_79550a1c","side":"PARENT","line":27,"in_reply_to":"1aa78d24_0ad10ece","updated":"2016-07-02 21:55:08.000000000","message":"This was removed after a discussion with Renat. I\u0027ll ask him to input his thoughts here. You\u0027ll see it was also removed in the function below.","commit_id":"9d00052f617c22b87a98320df3ee4f0f457fe828"},{"author":{"_account_id":9712,"name":"Dougal Matthews","email":"dougal.matthews@canonical.com","username":"dougal"},"change_message_id":"da78bc75c61d569235fa2ebcf2c50129c90f5925","unresolved":false,"context_lines":[{"line_number":24,"context_line":""},{"line_number":25,"context_line":"    db_actions \u003d []"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"    with db_api.transaction():"},{"line_number":28,"context_line":"        for action_spec in action_list_spec.get_actions():"},{"line_number":29,"context_line":"            db_actions.append(create_action(action_spec, definition, scope))"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"1aa78d24_f87e01ad","side":"PARENT","line":27,"in_reply_to":"1aa78d24_235cf024","updated":"2016-07-04 08:31:44.000000000","message":"It seems like a mistake to me to have functions here be so similar but have different usage requirements. They should absolutely be consistent. How do I know that I need to start a transaction for one, but not the other?","commit_id":"9d00052f617c22b87a98320df3ee4f0f457fe828"},{"author":{"_account_id":6732,"name":"Lingxian Kong","email":"anlin.kong@gmail.com","username":"kong"},"change_message_id":"3bc61c0ed196f13aae4ebbf23ef3f8dfea3e0452","unresolved":false,"context_lines":[{"line_number":24,"context_line":""},{"line_number":25,"context_line":"    db_actions \u003d []"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"    with db_api.transaction():"},{"line_number":28,"context_line":"        for action_spec in action_list_spec.get_actions():"},{"line_number":29,"context_line":"            db_actions.append(create_action(action_spec, definition, scope))"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"1aa78d24_5b462a24","side":"PARENT","line":27,"in_reply_to":"1aa78d24_49c149e4","updated":"2016-07-04 10:09:08.000000000","message":"fair enough :-)","commit_id":"9d00052f617c22b87a98320df3ee4f0f457fe828"},{"author":{"_account_id":6732,"name":"Lingxian Kong","email":"anlin.kong@gmail.com","username":"kong"},"change_message_id":"e8ace19929e8947f37a14df81a23d1b57c1f72e1","unresolved":false,"context_lines":[{"line_number":24,"context_line":""},{"line_number":25,"context_line":"    db_actions \u003d []"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"    with db_api.transaction():"},{"line_number":28,"context_line":"        for action_spec in action_list_spec.get_actions():"},{"line_number":29,"context_line":"            db_actions.append(create_action(action_spec, definition, scope))"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"1aa78d24_235cf024","side":"PARENT","line":27,"in_reply_to":"1aa78d24_79550a1c","updated":"2016-07-03 04:17:17.000000000","message":"Yeah, the \u0027update_actions\u0027 will be used both by API and action generation script. For API, we hope the update happened within one transaction, and for that script, it need not. Things are different with \u0027create_actions\u0027.\n\nThe only reason of the remove I can think of is for consistency, but it\u0027s weird.","commit_id":"9d00052f617c22b87a98320df3ee4f0f457fe828"},{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"df876a384dac2a1165cab2a41aaee3ad6b49eae6","unresolved":false,"context_lines":[{"line_number":24,"context_line":""},{"line_number":25,"context_line":"    db_actions \u003d []"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"    with db_api.transaction():"},{"line_number":28,"context_line":"        for action_spec in action_list_spec.get_actions():"},{"line_number":29,"context_line":"            db_actions.append(create_action(action_spec, definition, scope))"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"1aa78d24_49c149e4","side":"PARENT","line":27,"in_reply_to":"1aa78d24_f87e01ad","updated":"2016-07-04 09:29:35.000000000","message":"The exact reason I recommended to remove TX management from these methods is that \"run_in_tx\" param. Don\u0027t you agree that it is weird?\nMore broadly, the point I\u0027m making here is that these methods are used more like utilities, not services (feel the difference?). If they were really services then they should be in charge of managing everything including transactions, but in fact, they are not services, they are just called so (package name). And the problem is that if you look at the controller you\u0027ll see that it does much else and calling this method is only a part of that logic. It means that the main logic is concentrated in the controller, not in the service and only the controller knows how to demarcate transactions.\n\nI think the root issue here is that our controllers are often not thin they don\u0027t offload all heavy stuff to underlying layers as it should be. I\u0027d say it\u0027s a \"bad smell\" here from architecture perspective and demands a refactoring.\n\nBut again, having parameters like \"run_in_tx\" seems to be even worse smell :)\n\nTell me what you think please. May be I\u0027m wrong.","commit_id":"9d00052f617c22b87a98320df3ee4f0f457fe828"}],"mistral/tests/unit/api/v2/test_actions.py":[{"author":{"_account_id":9712,"name":"Dougal Matthews","email":"dougal.matthews@canonical.com","username":"dougal"},"change_message_id":"6c0c838fe1c85ea8c1077467483af4e95bd450eb","unresolved":false,"context_lines":[{"line_number":173,"context_line":"    @mock.patch.object("},{"line_number":174,"context_line":"        db_api, \"create_or_update_action_definition\", MOCK_NOT_FOUND"},{"line_number":175,"context_line":"    )"},{"line_number":176,"context_line":"    def test_put_not_found(self):"},{"line_number":177,"context_line":"        resp \u003d self.app.put("},{"line_number":178,"context_line":"            \u0027/v2/actions\u0027,"},{"line_number":179,"context_line":"            UPDATED_ACTION_DEFINITION,"}],"source_content_type":"text/x-python","patch_set":8,"id":"3aaa91ec_e0b67187","side":"PARENT","line":176,"updated":"2016-07-01 12:23:56.000000000","message":"Renat, this tests here verifies the 404 response.","commit_id":"9d00052f617c22b87a98320df3ee4f0f457fe828"},{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"df876a384dac2a1165cab2a41aaee3ad6b49eae6","unresolved":false,"context_lines":[{"line_number":173,"context_line":"    @mock.patch.object("},{"line_number":174,"context_line":"        db_api, \"create_or_update_action_definition\", MOCK_NOT_FOUND"},{"line_number":175,"context_line":"    )"},{"line_number":176,"context_line":"    def test_put_not_found(self):"},{"line_number":177,"context_line":"        resp \u003d self.app.put("},{"line_number":178,"context_line":"            \u0027/v2/actions\u0027,"},{"line_number":179,"context_line":"            UPDATED_ACTION_DEFINITION,"}],"source_content_type":"text/x-python","patch_set":8,"id":"1aa78d24_a9b42df2","side":"PARENT","line":176,"in_reply_to":"3aaa91ec_e0b67187","updated":"2016-07-04 09:29:35.000000000","message":"Ooh, ok. I guess I overlooked it.","commit_id":"9d00052f617c22b87a98320df3ee4f0f457fe828"}]}