)]}'
{"designate/backend/impl_bind9.py":[{"author":{"_account_id":8174,"name":"Tim Simmons","email":"tim.r.simmons@gmail.com","username":"TimSimmons"},"change_message_id":"4a13c399ed17cfcf5040a8e3545c057d668e3f92","unresolved":false,"context_lines":[{"line_number":195,"context_line":"                                          servers\u003dservers,"},{"line_number":196,"context_line":"                                          domain\u003ddomain,"},{"line_number":197,"context_line":"                                          records\u003drecords)"},{"line_number":198,"context_line":"        self._sync_domains()"},{"line_number":199,"context_line":"        rndc_call \u003d self._rndc_base() + [\u0027reload\u0027]"},{"line_number":200,"context_line":"        LOG.debug(\u0027Calling RNDC with: %s\u0027 % \" \".join(rndc_call))"},{"line_number":201,"context_line":"        utils.execute(*rndc_call)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2F3gQ%3D","line":198,"updated":"2013-09-18 00:16:06.000000000","message":"I just wanted to register that if this change and mine are merged at the same time, this line will break because the sync_domains() function is gone. But since this doesn\u0027t affect the bind9 config file (it\u0027s not creating/deleting zones), I don\u0027t believe there are any substitutions that need to be made, the line can just be removed.","commit_id":"7f1232437fe5b0a48fbcc2beba92a05673c8674c"},{"author":{"_account_id":741,"name":"Kiall Mac Innes","email":"kiall@macinnes.ie","username":"kiall"},"change_message_id":"c7c5365920e999c4dc201693d113e5e6484be363","unresolved":false,"context_lines":[{"line_number":195,"context_line":"                                          servers\u003dservers,"},{"line_number":196,"context_line":"                                          domain\u003ddomain,"},{"line_number":197,"context_line":"                                          records\u003drecords)"},{"line_number":198,"context_line":"        self._sync_domains()"},{"line_number":199,"context_line":"        rndc_call \u003d self._rndc_base() + [\u0027reload\u0027]"},{"line_number":200,"context_line":"        LOG.debug(\u0027Calling RNDC with: %s\u0027 % \" \".join(rndc_call))"},{"line_number":201,"context_line":"        utils.execute(*rndc_call)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2F0X0%3D","line":198,"in_reply_to":"AAAAS3%2F%2F3gQ%3D","updated":"2013-09-18 18:24:15.000000000","message":"We tend to go with every landed patch should leave the application functional.. \n\nSo, if your\u0027s lands first.. then we would want to remove this line.. On the other hand, if this patch lands first, it would be up to you to rebase and remove this line :)","commit_id":"7f1232437fe5b0a48fbcc2beba92a05673c8674c"},{"author":{"_account_id":741,"name":"Kiall Mac Innes","email":"kiall@macinnes.ie","username":"kiall"},"change_message_id":"ebb2ef42314b4452b4f08b65619eca432e72e72b","unresolved":false,"context_lines":[{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    # TODO(eankutse): Improve this so it scales. Need to design"},{"line_number":176,"context_line":"    # for it in the new Pool Manager/Agent for the backend that is"},{"line_number":177,"context_line":"    # being proposed"},{"line_number":178,"context_line":"    def _sync_domains_on_server_change(self):"},{"line_number":179,"context_line":"        LOG.debug(\u0027Synchronising domains on server change\u0027)"},{"line_number":180,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAS3%2F%2FvBM%3D","line":177,"updated":"2013-09-20 10:58:22.000000000","message":"Total and utter nit - don\u0027t upload a new patchset unless there\u0027s something else ;)\n\nGenerally, the conventions in Python/OpenStack are to leave these sort comments inside the function itself :)","commit_id":"baecdb0d5af4696c9891bef8aa7a043a0083d972"}],"designate/backend/impl_dnsmasq.py":[{"author":{"_account_id":741,"name":"Kiall Mac Innes","email":"kiall@macinnes.ie","username":"kiall"},"change_message_id":"45cc50bd5b8cbaa81380172f93c66e40c243dcd1","unresolved":false,"context_lines":[{"line_number":50,"context_line":""},{"line_number":51,"context_line":"        self._sync_domains()"},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"    def create_server(self, context, server):"},{"line_number":54,"context_line":"        LOG.debug(\u0027Create Server\u0027)"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"        domains \u003d self.central_service.find_domains(self.admin_context)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAASn%2F%2FcM4%3D","line":53,"updated":"2013-09-04 18:41:17.000000000","message":"So, dnsmasq only supports A and AAAA records, and doesn\u0027t need to list the \"nameservers\"\n\nWe can just make all three methods noop\u0027s:\n\n    def create_server(self, context, server):\n        pass\n\n    def update_server(self, context, server):\n        pass\n\n    def delete_server(self, context, server):\n        pass","commit_id":"46cb1c804b6f3a1a9bb33eba627c1da818502268"},{"author":{"_account_id":741,"name":"Kiall Mac Innes","email":"kiall@macinnes.ie","username":"kiall"},"change_message_id":"ebb2ef42314b4452b4f08b65619eca432e72e72b","unresolved":false,"context_lines":[{"line_number":51,"context_line":"        self._sync_domains()"},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"    # Since dnsmasq only supports A and AAAA records, create_server,"},{"line_number":54,"context_line":"    # update_server, and delete_server can be noop\u0027s"},{"line_number":55,"context_line":"    def create_server(self, context, server):"},{"line_number":56,"context_line":"        LOG.debug(\u0027Create Server - noop\u0027)"},{"line_number":57,"context_line":"        pass"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAS3%2F%2FvBA%3D","line":54,"updated":"2013-09-20 10:58:22.000000000","message":"On the other hand, this one applies to the three methods below .. So having it here is perfect :)","commit_id":"baecdb0d5af4696c9891bef8aa7a043a0083d972"}],"designate/backend/impl_fake.py":[{"author":{"_account_id":8094,"name":"Vinod Mangalpally","email":"mrvinod@outlook.com","username":"vinodmr"},"change_message_id":"679460bebfe07a1581bae640343f9ea3285b29dd","unresolved":false,"context_lines":[{"line_number":38,"context_line":"        LOG.info(\u0027Create Server %r\u0027 % server)"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"    def update_server(self, context, server, old_server_name):"},{"line_number":41,"context_line":"        LOG.debug(\u0027Update Server %r\u0027 % server)"},{"line_number":42,"context_line":""},{"line_number":43,"context_line":"    def delete_server(self, context, server):"},{"line_number":44,"context_line":"        LOG.debug(\u0027Delete Server %r\u0027 % server)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2F0xY%3D","line":41,"updated":"2013-09-18 17:43:37.000000000","message":"If you are logging, could you log the old_server_name too here?\nAlso here and below you have LOG.debug but above you have LOG.info.  Could you make all of them the same?","commit_id":"7f1232437fe5b0a48fbcc2beba92a05673c8674c"},{"author":{"_account_id":741,"name":"Kiall Mac Innes","email":"kiall@macinnes.ie","username":"kiall"},"change_message_id":"c7c5365920e999c4dc201693d113e5e6484be363","unresolved":false,"context_lines":[{"line_number":38,"context_line":"        LOG.info(\u0027Create Server %r\u0027 % server)"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"    def update_server(self, context, server, old_server_name):"},{"line_number":41,"context_line":"        LOG.debug(\u0027Update Server %r\u0027 % server)"},{"line_number":42,"context_line":""},{"line_number":43,"context_line":"    def delete_server(self, context, server):"},{"line_number":44,"context_line":"        LOG.debug(\u0027Delete Server %r\u0027 % server)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2F0Xk%3D","line":41,"in_reply_to":"AAAAS3%2F%2F0xY%3D","updated":"2013-09-18 18:24:15.000000000","message":"Graham had a good rant at me today after spending 30 mins trying to understand why the *_domain we\u0027re never being called.. \n\nWe should fix these to all be the same level, probably INFO :)","commit_id":"7f1232437fe5b0a48fbcc2beba92a05673c8674c"}],"designate/backend/impl_powerdns/__init__.py":[{"author":{"_account_id":741,"name":"Kiall Mac Innes","email":"kiall@macinnes.ie","username":"kiall"},"change_message_id":"45cc50bd5b8cbaa81380172f93c66e40c243dcd1","unresolved":false,"context_lines":[{"line_number":365,"context_line":"        else:"},{"line_number":366,"context_line":"            return record"},{"line_number":367,"context_line":""},{"line_number":368,"context_line":"    def _update_domains_on_server_changes(self, server):"},{"line_number":369,"context_line":"        domains \u003d self.central_service.find_servers(self.admin_context)"},{"line_number":370,"context_line":"        for domain in domains:"},{"line_number":371,"context_line":"            record_m \u003d models.Record()"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAASn%2F%2FcJc%3D","line":368,"updated":"2013-09-04 18:41:17.000000000","message":"Ideally, these two methods would be done in SQL.. The adding a TSIG key shows an example of doing a bulk INSERT..\n\nAn example for INSERT is here: https://github.com/stackforge/designate/blob/master/designate/backend/impl_powerdns/__init__.py#L71-78\n\nUpdate can probably be an: UPDATE records SET name \u003d \"newServerName\" where designate_id \u003d \"serverId\" - We potentially still have to update some/add of the SOA records though..","commit_id":"46cb1c804b6f3a1a9bb33eba627c1da818502268"},{"author":{"_account_id":741,"name":"Kiall Mac Innes","email":"kiall@macinnes.ie","username":"kiall"},"change_message_id":"45cc50bd5b8cbaa81380172f93c66e40c243dcd1","unresolved":false,"context_lines":[{"line_number":379,"context_line":"            })"},{"line_number":380,"context_line":"            record_m.save(self.session)"},{"line_number":381,"context_line":""},{"line_number":382,"context_line":"    def _update_domains_on_server_delete(self, server):"},{"line_number":383,"context_line":""},{"line_number":384,"context_line":"        \"\"\""},{"line_number":385,"context_line":"        for each domain, delete the NS record whose id (designate_id) equals"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAASn%2F%2FcH4%3D","line":382,"updated":"2013-09-04 18:41:17.000000000","message":"Again, we can break out into something a bit lower level here rather than issuing a DELETE per domain.. I\u0027ll dig out an example.","commit_id":"46cb1c804b6f3a1a9bb33eba627c1da818502268"},{"author":{"_account_id":741,"name":"Kiall Mac Innes","email":"kiall@macinnes.ie","username":"kiall"},"change_message_id":"ebb2ef42314b4452b4f08b65619eca432e72e72b","unresolved":false,"context_lines":[{"line_number":453,"context_line":"                    .where(and_("},{"line_number":454,"context_line":"                        models.Record.__table__.c.type \u003d\u003d \"NS\","},{"line_number":455,"context_line":"                        models.Record.__table__.c.designate_id"},{"line_number":456,"context_line":"                        \u003d\u003d server[\u0027id\u0027]))).fetchone()[\u0027content\u0027]"},{"line_number":457,"context_line":""},{"line_number":458,"context_line":"            LOG.debug(\"old server name read from a backend NS record:\""},{"line_number":459,"context_line":"                      \" %s\" % old_server_name)"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAS3%2F%2Fu%2Fo%3D","line":456,"updated":"2013-09-20 10:58:22.000000000","message":"This could be simplified to:\n\n    old_server \u003d self.session.query(models.Record)\\\n                             .filter_by(type\u003d\u0027NS\u0027, designate_id\u003dserver[\u0027id\u0027])\\\n                             .first()\n\n    old_server_name \u003d old_server[\u0027content\u0027]","commit_id":"baecdb0d5af4696c9891bef8aa7a043a0083d972"},{"author":{"_account_id":741,"name":"Kiall Mac Innes","email":"kiall@macinnes.ie","username":"kiall"},"change_message_id":"ebb2ef42314b4452b4f08b65619eca432e72e72b","unresolved":false,"context_lines":[{"line_number":466,"context_line":"                .where(and_("},{"line_number":467,"context_line":"                    models.Record.__table__.c.type \u003d\u003d \"NS\","},{"line_number":468,"context_line":"                    models.Record.__table__.c.designate_id \u003d\u003d server[\u0027id\u0027]))"},{"line_number":469,"context_line":"                .values(content\u003dns_rec_content))"},{"line_number":470,"context_line":""},{"line_number":471,"context_line":"            # Then update all SOA records as necessary"},{"line_number":472,"context_line":"            # Do the SOA last, ensuring we don\u0027t trigger a NOTIFY"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAS3%2F%2FvAA%3D","line":469,"updated":"2013-09-20 10:58:22.000000000","message":"This could be simplified to:\n\n    self.session.query(models.Record).\\\n                .filter_by(type\u003d\u0027NS\u0027, designate_id\u003dserver[\u0027id\u0027])\\\n                .update({\"content\": ns_rec_content})","commit_id":"baecdb0d5af4696c9891bef8aa7a043a0083d972"},{"author":{"_account_id":741,"name":"Kiall Mac Innes","email":"kiall@macinnes.ie","username":"kiall"},"change_message_id":"ebb2ef42314b4452b4f08b65619eca432e72e72b","unresolved":false,"context_lines":[{"line_number":511,"context_line":"        # make sure the replacement server name is not the one"},{"line_number":512,"context_line":"        # we are about to delete"},{"line_number":513,"context_line":"        if replacement_server_name \u003d\u003d server[\u0027name\u0027]:"},{"line_number":514,"context_line":"            if len(servers) \u003e 1:"},{"line_number":515,"context_line":"                replacement_server_name \u003d servers[1][\u0027name\u0027]"},{"line_number":516,"context_line":"            else:"},{"line_number":517,"context_line":"                raise exceptions.LastServerDeleteNotAllowed("}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAS3%2F%2Fu%2FU%3D","line":514,"updated":"2013-09-20 10:58:22.000000000","message":"should this be \"\u003e\u003d\" rather than \"\u003e\" and servers[0] below?\n\nIf we\u0027re returning the server being deleted in this list, and your trying to skip it .. There\u0027s no guarantee to the ordering of the items returned. We may be better off with a loop that breaks when it finds a server who\u0027s ID !\u003d the one being deleted.\n\nAlso - I think this logic belongs in central, as it applies to every backend?","commit_id":"baecdb0d5af4696c9891bef8aa7a043a0083d972"},{"author":{"_account_id":741,"name":"Kiall Mac Innes","email":"kiall@macinnes.ie","username":"kiall"},"change_message_id":"ebb2ef42314b4452b4f08b65619eca432e72e72b","unresolved":false,"context_lines":[{"line_number":543,"context_line":"        else:"},{"line_number":544,"context_line":"            # ok to commit the delete,"},{"line_number":545,"context_line":"            self.session.commit()"},{"line_number":546,"context_line":"            # then continue with SOA record updates"},{"line_number":547,"context_line":"            try:"},{"line_number":548,"context_line":"                # update all SOA records as necessary"},{"line_number":549,"context_line":"                # Do the SOA last, ensuring we don\u0027t trigger a"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAS3%2F%2Fu%2Bw%3D","line":546,"updated":"2013-09-20 10:58:22.000000000","message":"I think we would want both the DELETE and UPDATE queries to be performed in the same TX?","commit_id":"baecdb0d5af4696c9891bef8aa7a043a0083d972"}],"designate/backend/impl_rpc.py":[{"author":{"_account_id":8094,"name":"Vinod Mangalpally","email":"mrvinod@outlook.com","username":"vinodmr"},"change_message_id":"679460bebfe07a1581bae640343f9ea3285b29dd","unresolved":false,"context_lines":[{"line_number":33,"context_line":"        return agent_api.create_server(context, server)"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"    def update_server(self, context, server, old_server_name):"},{"line_number":36,"context_line":"        return agent_api.update_server(context, server)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def delete_server(self, context, server):"},{"line_number":39,"context_line":"        return agent_api.delete_server(context, server)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2F0yE%3D","line":36,"updated":"2013-09-18 17:43:37.000000000","message":"In designate/agent/rpcapi.py, I see update_server taking server_id and values but here we pass in just the server\nSimilarly delete_server takes server_id.\nIs there something else that happens before the agent calls are invoked?\nAlso is the old_server_name not passed on here?","commit_id":"7f1232437fe5b0a48fbcc2beba92a05673c8674c"},{"author":{"_account_id":741,"name":"Kiall Mac Innes","email":"kiall@macinnes.ie","username":"kiall"},"change_message_id":"c7c5365920e999c4dc201693d113e5e6484be363","unresolved":false,"context_lines":[{"line_number":33,"context_line":"        return agent_api.create_server(context, server)"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"    def update_server(self, context, server, old_server_name):"},{"line_number":36,"context_line":"        return agent_api.update_server(context, server)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def delete_server(self, context, server):"},{"line_number":39,"context_line":"        return agent_api.delete_server(context, server)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2F0Wc%3D","line":36,"in_reply_to":"AAAAS3%2F%2F0yE%3D","updated":"2013-09-18 18:24:15.000000000","message":"Nice catch :)\n\nOn first glance, it looks like the agent RPC API is incorrect. I\u0027m not sure anyone actually uses the agent!\n\nI think that should be fixed in a separate review (if at all, as Graham\u0027s pool changes replace the agent service)","commit_id":"7f1232437fe5b0a48fbcc2beba92a05673c8674c"}],"designate/central/service.py":[{"author":{"_account_id":8094,"name":"Vinod Mangalpally","email":"mrvinod@outlook.com","username":"vinodmr"},"change_message_id":"679460bebfe07a1581bae640343f9ea3285b29dd","unresolved":false,"context_lines":[{"line_number":272,"context_line":"    def create_server(self, context, values):"},{"line_number":273,"context_line":"        policy.check(\u0027create_server\u0027, context)"},{"line_number":274,"context_line":""},{"line_number":275,"context_line":"        with self.storage_api.create_server(context, values) as server:"},{"line_number":276,"context_line":"            # Update backend with the new server.."},{"line_number":277,"context_line":"            with wrap_backend_call():"},{"line_number":278,"context_line":"                self.backend.create_server(context, server)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2F0z8%3D","line":275,"updated":"2013-09-18 17:43:37.000000000","message":"Just wanted to clarify - is there similar code to update the values in storage as is done in designate/backend/impl_powerdns/__init__.py?\nKiall do you know where the code is for storage?","commit_id":"7f1232437fe5b0a48fbcc2beba92a05673c8674c"},{"author":{"_account_id":741,"name":"Kiall Mac Innes","email":"kiall@macinnes.ie","username":"kiall"},"change_message_id":"c7c5365920e999c4dc201693d113e5e6484be363","unresolved":false,"context_lines":[{"line_number":272,"context_line":"    def create_server(self, context, values):"},{"line_number":273,"context_line":"        policy.check(\u0027create_server\u0027, context)"},{"line_number":274,"context_line":""},{"line_number":275,"context_line":"        with self.storage_api.create_server(context, values) as server:"},{"line_number":276,"context_line":"            # Update backend with the new server.."},{"line_number":277,"context_line":"            with wrap_backend_call():"},{"line_number":278,"context_line":"                self.backend.create_server(context, server)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2F0WA%3D","line":275,"in_reply_to":"AAAAS3%2F%2F0z8%3D","updated":"2013-09-18 18:24:15.000000000","message":"The storage code to make these changes is preexisting[1], and much simpler due to not being mixed in with all the records.\n\n[1]: https://github.com/stackforge/designate/blob/master/designate/storage/impl_sqlalchemy/__init__.py#L160-203","commit_id":"7f1232437fe5b0a48fbcc2beba92a05673c8674c"},{"author":{"_account_id":8094,"name":"Vinod Mangalpally","email":"mrvinod@outlook.com","username":"vinodmr"},"change_message_id":"679460bebfe07a1581bae640343f9ea3285b29dd","unresolved":false,"context_lines":[{"line_number":310,"context_line":"        policy.check(\u0027delete_server\u0027, context, {\u0027server_id\u0027: server_id})"},{"line_number":311,"context_line":""},{"line_number":312,"context_line":"        with self.storage_api.delete_server(context, server_id) as server:"},{"line_number":313,"context_line":"            # Update backend with the new server.."},{"line_number":314,"context_line":"            with wrap_backend_call():"},{"line_number":315,"context_line":"                self.backend.delete_server(context, server)"},{"line_number":316,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2F0zk%3D","line":313,"updated":"2013-09-18 17:43:37.000000000","message":"Could you update the comments here?","commit_id":"7f1232437fe5b0a48fbcc2beba92a05673c8674c"}],"designate/exceptions.py":[{"author":{"_account_id":741,"name":"Kiall Mac Innes","email":"kiall@macinnes.ie","username":"kiall"},"change_message_id":"ebb2ef42314b4452b4f08b65619eca432e72e72b","unresolved":false,"context_lines":[{"line_number":156,"context_line":"    error_type \u003d \u0027record_not_found\u0027"},{"line_number":157,"context_line":""},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"class LastServerDeleteNotAllowed(Base, Backend):"},{"line_number":160,"context_line":"    error_type \u003d \u0027last_server_delete_not_allowed\u0027"},{"line_number":161,"context_line":""},{"line_number":162,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAS3%2F%2Fu%2BY%3D","line":159,"updated":"2013-09-20 10:58:22.000000000","message":"This shouldn\u0027t extend the Backend exception here, at least not with the current pattern we follow :)\n\nAlso - I would probably have this extend BadRequest instead.","commit_id":"baecdb0d5af4696c9891bef8aa7a043a0083d972"}]}