)]}'
{"keystone/common/sql/contract_repo/versions/023_contract_drop_federated_user_fks.py":[{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"dd2ac572f81edcc169c7417627d3e6a30e51b46a","unresolved":false,"context_lines":[{"line_number":37,"context_line":"        migrate.ForeignKeyConstraint("},{"line_number":38,"context_line":"            columns\u003d[federated_table.c.protocol_id, federated_table.c.idp_id],"},{"line_number":39,"context_line":"            refcolumns\u003d[protocol_table.c.id, protocol_table.c.idp_id],"},{"line_number":40,"context_line":"            name\u003dfk_name).drop()"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":""},{"line_number":43,"context_line":"def _get_fk_name(inspector, table, fk_columns):"}],"source_content_type":"text/x-python","patch_set":5,"id":"1a1ced50_42d91783","line":40,"updated":"2017-03-23 18:59:30.000000000","message":"At first glance, I didn\u0027t think this would work with our upgrade process because I thought for a second that we would have a race condition between nodes don\u0027t have the constraint in the manager layers once the foreign keys are dropped.\n\nTurns out that isn\u0027t the case because according to our rolling upgrade documentation, all nodes in the deployment should be running the latest release *before* the contract phase is executed. Which means the constraint should be honored throughout the entire upgrade either in the API or in the DB. But, for a brief period of time we should have the constraint duplicated because we\u0027ll be checking it at the API layer on nodes that have new code, and at the DB layer for the nodes that have the older code.\n\nThat might make a good comment for other reviews or maybe other contributors looking to do something similar?","commit_id":"d3a1d981fb970977f82eb11d4c68506ffcd455d5"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"4afd285cd1ff2d4411d06c7ce9eecfdf80f3f645","unresolved":false,"context_lines":[{"line_number":37,"context_line":"        migrate.ForeignKeyConstraint("},{"line_number":38,"context_line":"            columns\u003d[federated_table.c.protocol_id, federated_table.c.idp_id],"},{"line_number":39,"context_line":"            refcolumns\u003d[protocol_table.c.id, protocol_table.c.idp_id],"},{"line_number":40,"context_line":"            name\u003dfk_name).drop()"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":""},{"line_number":43,"context_line":"def _get_fk_name(inspector, table, fk_columns):"}],"source_content_type":"text/x-python","patch_set":5,"id":"ffe62b97_d24ad07e","line":40,"in_reply_to":"1a1ced50_42d91783","updated":"2017-03-24 14:24:54.000000000","message":"yep, excellent observation. Will try to translate in a comment here.","commit_id":"d3a1d981fb970977f82eb11d4c68506ffcd455d5"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"26f6a58f215109278d6940a96404f7b59aee81ce","unresolved":false,"context_lines":[{"line_number":25,"context_line":"    protocol_table \u003d sql.Table(\u0027federation_protocol\u0027, meta, autoload\u003dTrue)"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"    # In a replicated environment, all nodes in the deployment should be"},{"line_number":28,"context_line":"    # running the latest release before the contract phase is executed. Meaning"},{"line_number":29,"context_line":"    # the constraint should be honored throughout the entire upgrade either in"},{"line_number":30,"context_line":"    # the API or in the DB. But, for a brief period of time we should have the"},{"line_number":31,"context_line":"    # constraint duplicated because we will be checking it at the API layer on"}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_e36b0182","line":28,"updated":"2017-03-29 18:46:44.000000000","message":"This first sentence could be amended to say that this is per our rolling upgrade documentation.","commit_id":"db3a1657e630d15b7d49c325e0ac05527016765a"}],"keystone/federation/core.py":[{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"dd2ac572f81edcc169c7417627d3e6a30e51b46a","unresolved":false,"context_lines":[{"line_number":83,"context_line":"    def _assert_valid_domain_id(self, domain_id):"},{"line_number":84,"context_line":"        self.resource_api.get_domain(domain_id)"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":"    def delete_idp(self, idp_id):"},{"line_number":87,"context_line":"        \"\"\"Delete an identity provider."},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"        Before we delete the identity provider, we need to remove all"}],"source_content_type":"text/x-python","patch_set":5,"id":"1a1ced50_e25683cd","line":86,"updated":"2017-03-23 18:59:30.000000000","message":"To be consistent with other methods in this module (and in other keystone APIs), we should add a documentation string fro `idp_id`.","commit_id":"d3a1d981fb970977f82eb11d4c68506ffcd455d5"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"5f175d14c241c399206b672b034381dcd903adfb","unresolved":false,"context_lines":[{"line_number":83,"context_line":"    def _assert_valid_domain_id(self, domain_id):"},{"line_number":84,"context_line":"        self.resource_api.get_domain(domain_id)"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":"    def delete_idp(self, idp_id):"},{"line_number":87,"context_line":"        \"\"\"Delete an identity provider."},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"        Before we delete the identity provider, we need to remove all"}],"source_content_type":"text/x-python","patch_set":5,"id":"ffe62b97_346f2cfd","line":86,"in_reply_to":"1a1ced50_e25683cd","updated":"2017-03-28 12:55:56.000000000","message":"Done","commit_id":"d3a1d981fb970977f82eb11d4c68506ffcd455d5"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"dd2ac572f81edcc169c7417627d3e6a30e51b46a","unresolved":false,"context_lines":[{"line_number":97,"context_line":"        self._delete_federated_users(fed_users_list)"},{"line_number":98,"context_line":"        self.driver.delete_idp(idp_id)"},{"line_number":99,"context_line":""},{"line_number":100,"context_line":"    def _list_federated_users(self, idp_id\u003dNone, protocol_id\u003dNone):"},{"line_number":101,"context_line":"        hints \u003d driver_hints.Hints()"},{"line_number":102,"context_line":"        if idp_id:"},{"line_number":103,"context_line":"            hints.add_filter(\u0027idp_id\u0027, idp_id)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1a1ced50_a28d6b2c","line":100,"updated":"2017-03-23 18:59:30.000000000","message":"nit: I\u0027d be in favor of foregoing this method and just keeping the idp or protocol specific hints logic in the respective `delete_idp` and `delete_protocol` method, since it keeps the business logic in the method.\n\n\n  def delete_idp(self, idp_id):\n      \u003csnip docstrings\u003e\n      hints \u003d driver_hints.Hints()\n      hints.add_filter(\u0027idp_id\u0027, idp_id)\n      idp_users \u003d self.identity_api.list_users(hints\u003dhints)\n      for user in idp_users:\n          self.identity_api.delete_user(user[\u0027id\u0027])","commit_id":"d3a1d981fb970977f82eb11d4c68506ffcd455d5"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"4afd285cd1ff2d4411d06c7ce9eecfdf80f3f645","unresolved":false,"context_lines":[{"line_number":97,"context_line":"        self._delete_federated_users(fed_users_list)"},{"line_number":98,"context_line":"        self.driver.delete_idp(idp_id)"},{"line_number":99,"context_line":""},{"line_number":100,"context_line":"    def _list_federated_users(self, idp_id\u003dNone, protocol_id\u003dNone):"},{"line_number":101,"context_line":"        hints \u003d driver_hints.Hints()"},{"line_number":102,"context_line":"        if idp_id:"},{"line_number":103,"context_line":"            hints.add_filter(\u0027idp_id\u0027, idp_id)"}],"source_content_type":"text/x-python","patch_set":5,"id":"ffe62b97_4d32a1f3","line":100,"in_reply_to":"1a1ced50_a28d6b2c","updated":"2017-03-24 14:24:54.000000000","message":"I tend to disagree here. We are creating a method that has only one, well defined goal and it is being shared between two other methods.","commit_id":"d3a1d981fb970977f82eb11d4c68506ffcd455d5"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"dd2ac572f81edcc169c7417627d3e6a30e51b46a","unresolved":false,"context_lines":[{"line_number":173,"context_line":"            raise exception.ValidationError(message\u003d(msg % mapping_id))"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    def delete_protocol(self, idp_id, protocol_id):"},{"line_number":176,"context_line":"        \"\"\"Delete a protocol."},{"line_number":177,"context_line":""},{"line_number":178,"context_line":"        Before we delete the protocol, we need to remove all"},{"line_number":179,"context_line":"        the federated_users that reference it. To do this we"}],"source_content_type":"text/x-python","patch_set":5,"id":"1a1ced50_42c6774a","line":176,"updated":"2017-03-23 18:59:30.000000000","message":"Similar comments here as above - specifically for the documentation strings.","commit_id":"d3a1d981fb970977f82eb11d4c68506ffcd455d5"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"5f175d14c241c399206b672b034381dcd903adfb","unresolved":false,"context_lines":[{"line_number":173,"context_line":"            raise exception.ValidationError(message\u003d(msg % mapping_id))"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    def delete_protocol(self, idp_id, protocol_id):"},{"line_number":176,"context_line":"        \"\"\"Delete a protocol."},{"line_number":177,"context_line":""},{"line_number":178,"context_line":"        Before we delete the protocol, we need to remove all"},{"line_number":179,"context_line":"        the federated_users that reference it. To do this we"}],"source_content_type":"text/x-python","patch_set":5,"id":"ffe62b97_9432780f","line":176,"in_reply_to":"1a1ced50_42c6774a","updated":"2017-03-28 12:55:56.000000000","message":"Done","commit_id":"d3a1d981fb970977f82eb11d4c68506ffcd455d5"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"26f6a58f215109278d6940a96404f7b59aee81ce","unresolved":false,"context_lines":[{"line_number":98,"context_line":"        self._delete_federated_users(fed_users_list)"},{"line_number":99,"context_line":"        self.driver.delete_idp(idp_id)"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"    def _list_federated_users(self, idp_id\u003dNone, protocol_id\u003dNone):"},{"line_number":102,"context_line":"        hints \u003d driver_hints.Hints()"},{"line_number":103,"context_line":"        if idp_id:"},{"line_number":104,"context_line":"            hints.add_filter(\u0027idp_id\u0027, idp_id)"}],"source_content_type":"text/x-python","patch_set":6,"id":"ffe62b97_e3fb0144","line":101,"updated":"2017-03-29 18:46:44.000000000","message":"I still think this indirection isn\u0027t really necessary but we can continue to discuss and fix later if we reach a different conclusion.","commit_id":"db3a1657e630d15b7d49c325e0ac05527016765a"}],"keystone/tests/unit/test_v3_federation.py":[{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"dd2ac572f81edcc169c7417627d3e6a30e51b46a","unresolved":false,"context_lines":[{"line_number":3071,"context_line":""},{"line_number":3072,"context_line":"        return idp, protocol"},{"line_number":3073,"context_line":""},{"line_number":3074,"context_line":"    def test_delete_idp_after_federated_authentication(self):"},{"line_number":3075,"context_line":"        idp, protocol \u003d self._create_custom_idp_and_protocol()"},{"line_number":3076,"context_line":""},{"line_number":3077,"context_line":"        # Authenticate to create two new federated_user entries pointing to the"}],"source_content_type":"text/x-python","patch_set":5,"id":"1a1ced50_a25e4b77","line":3074,"updated":"2017-03-23 18:59:30.000000000","message":"nit: I like to try and work the expected outcome into the test name somewhere. For example:\n\n  def test_deleting_idp_deletes_federated_user_associated_to_idp(self):\n\nor something along those lines. It gives readers of the test the situation and the expected behavior the test is exercising all in one place.","commit_id":"d3a1d981fb970977f82eb11d4c68506ffcd455d5"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"4afd285cd1ff2d4411d06c7ce9eecfdf80f3f645","unresolved":false,"context_lines":[{"line_number":3071,"context_line":""},{"line_number":3072,"context_line":"        return idp, protocol"},{"line_number":3073,"context_line":""},{"line_number":3074,"context_line":"    def test_delete_idp_after_federated_authentication(self):"},{"line_number":3075,"context_line":"        idp, protocol \u003d self._create_custom_idp_and_protocol()"},{"line_number":3076,"context_line":""},{"line_number":3077,"context_line":"        # Authenticate to create two new federated_user entries pointing to the"}],"source_content_type":"text/x-python","patch_set":5,"id":"ffe62b97_ed7df56e","line":3074,"in_reply_to":"1a1ced50_a25e4b77","updated":"2017-03-24 14:24:54.000000000","message":"++ nice suggestion","commit_id":"d3a1d981fb970977f82eb11d4c68506ffcd455d5"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"dd2ac572f81edcc169c7417627d3e6a30e51b46a","unresolved":false,"context_lines":[{"line_number":3080,"context_line":"        user_id1 \u003d r.json_body[\u0027token\u0027][\u0027user\u0027][\u0027id\u0027]"},{"line_number":3081,"context_line":"        r \u003d self._issue_unscoped_token(idp\u003didp[\u0027id\u0027])"},{"line_number":3082,"context_line":"        user_id2 \u003d r.json_body[\u0027token\u0027][\u0027user\u0027][\u0027id\u0027]"},{"line_number":3083,"context_line":""},{"line_number":3084,"context_line":"        # Delete the identity provider"},{"line_number":3085,"context_line":"        self.federation_api.delete_idp(idp[\u0027id\u0027])"},{"line_number":3086,"context_line":"        # Assert both users have been deleted due the identity provider"}],"source_content_type":"text/x-python","patch_set":5,"id":"1a1ced50_a202cb70","line":3083,"updated":"2017-03-23 18:59:30.000000000","message":"We could add an assertion here checking that the users exist with the identity API.","commit_id":"d3a1d981fb970977f82eb11d4c68506ffcd455d5"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"4afd285cd1ff2d4411d06c7ce9eecfdf80f3f645","unresolved":false,"context_lines":[{"line_number":3080,"context_line":"        user_id1 \u003d r.json_body[\u0027token\u0027][\u0027user\u0027][\u0027id\u0027]"},{"line_number":3081,"context_line":"        r \u003d self._issue_unscoped_token(idp\u003didp[\u0027id\u0027])"},{"line_number":3082,"context_line":"        user_id2 \u003d r.json_body[\u0027token\u0027][\u0027user\u0027][\u0027id\u0027]"},{"line_number":3083,"context_line":""},{"line_number":3084,"context_line":"        # Delete the identity provider"},{"line_number":3085,"context_line":"        self.federation_api.delete_idp(idp[\u0027id\u0027])"},{"line_number":3086,"context_line":"        # Assert both users have been deleted due the identity provider"}],"source_content_type":"text/x-python","patch_set":5,"id":"ffe62b97_4de3c134","line":3083,"in_reply_to":"1a1ced50_a202cb70","updated":"2017-03-24 14:24:54.000000000","message":"does it?","commit_id":"d3a1d981fb970977f82eb11d4c68506ffcd455d5"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"dd2ac572f81edcc169c7417627d3e6a30e51b46a","unresolved":false,"context_lines":[{"line_number":3092,"context_line":"                          self.identity_api.get_user,"},{"line_number":3093,"context_line":"                          user_id2)"},{"line_number":3094,"context_line":""},{"line_number":3095,"context_line":"    def test_delete_protocol_after_federated_authentication(self):"},{"line_number":3096,"context_line":"        idp, protocol \u003d self._create_custom_idp_and_protocol()"},{"line_number":3097,"context_line":""},{"line_number":3098,"context_line":"        # Authenticate to create two new federated_user entries pointing to the"}],"source_content_type":"text/x-python","patch_set":5,"id":"1a1ced50_c2634740","line":3095,"updated":"2017-03-23 18:59:30.000000000","message":"Same nit here about test naming.","commit_id":"d3a1d981fb970977f82eb11d4c68506ffcd455d5"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"dd2ac572f81edcc169c7417627d3e6a30e51b46a","unresolved":false,"context_lines":[{"line_number":3102,"context_line":"        r \u003d self._issue_unscoped_token(idp\u003didp[\u0027id\u0027])"},{"line_number":3103,"context_line":"        user_id2 \u003d r.json_body[\u0027token\u0027][\u0027user\u0027][\u0027id\u0027]"},{"line_number":3104,"context_line":""},{"line_number":3105,"context_line":"        self.assertNotEmpty(self.identity_api.get_user(user_id1))"},{"line_number":3106,"context_line":"        self.assertNotEmpty(self.identity_api.get_user(user_id2))"},{"line_number":3107,"context_line":""},{"line_number":3108,"context_line":"        # Delete the protocol"}],"source_content_type":"text/x-python","patch_set":5,"id":"1a1ced50_e2cf6327","line":3105,"updated":"2017-03-23 18:59:30.000000000","message":"We should use these same assertions in the test above, too!\n\nGreat tests by the way.","commit_id":"d3a1d981fb970977f82eb11d4c68506ffcd455d5"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"5f175d14c241c399206b672b034381dcd903adfb","unresolved":false,"context_lines":[{"line_number":3102,"context_line":"        r \u003d self._issue_unscoped_token(idp\u003didp[\u0027id\u0027])"},{"line_number":3103,"context_line":"        user_id2 \u003d r.json_body[\u0027token\u0027][\u0027user\u0027][\u0027id\u0027]"},{"line_number":3104,"context_line":""},{"line_number":3105,"context_line":"        self.assertNotEmpty(self.identity_api.get_user(user_id1))"},{"line_number":3106,"context_line":"        self.assertNotEmpty(self.identity_api.get_user(user_id2))"},{"line_number":3107,"context_line":""},{"line_number":3108,"context_line":"        # Delete the protocol"}],"source_content_type":"text/x-python","patch_set":5,"id":"ffe62b97_777c9682","line":3105,"in_reply_to":"1a1ced50_e2cf6327","updated":"2017-03-28 12:55:56.000000000","message":"thought I had put them there too! Good catch","commit_id":"d3a1d981fb970977f82eb11d4c68506ffcd455d5"}]}