)]}'
{"kolla_ansible/cmd/genpwd.py":[{"author":{"_account_id":26008,"name":"Bogdan Katynski","email":"bogdan.katynski@workday.com","username":"bodgix"},"change_message_id":"33025aa21bc62354eb77935183b3e1ff529910f0","unresolved":false,"context_lines":[{"line_number":143,"context_line":"    return schemas[\u0027DEFAULT\u0027]"},{"line_number":144,"context_line":""},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"def genpwd(passwords, schemas\u003dKOLLA_PASSWORDS_SCHEMAS):"},{"line_number":147,"context_line":"    for ansible_variable_key, password in passwords.items():"},{"line_number":148,"context_line":"        key_schema \u003d get_schema_for_key(schemas, ansible_variable_key)"},{"line_number":149,"context_line":"        if not key_schema[\u0027skip\u0027](password):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_5bfd6238","line":146,"range":{"start_line":146,"start_character":0,"end_line":146,"end_character":10},"updated":"2019-02-10 23:50:35.000000000","message":"This is much better. The previous `genpwd` was just doing too much and was hard to understand not to mention testing.\n\nI was wondering however if, instead of the hash with functions have you considered creating a class hierarchy with a common API, possibly a base class with some common functionality and using a factory method that returns an instance of the correct class?\n\nBasically the factory method replaces your hash, and unless you want to log, you don\u0027t even need a separate method to skip. With that design, your genpwd becomes (in semi-pseudo code):\n\n  for ansible_variable_key, password in passwords.items():\n    passwd_handler \u003d PasswordFactory.build(ansible_variable_key, password)\n    passwords[ansible_variable_key] \u003d passwd_handler.generate_if_empty()","commit_id":"d275fc4a20f966f0a2117120e67c3e68350be2f8"},{"author":{"_account_id":23942,"name":"Matt Kucia","email":"maciej@kucia.net","username":"maciejkucia"},"change_message_id":"2d06c9c9ae885be562e72b44657c3f6f01ec40fa","unresolved":false,"context_lines":[{"line_number":143,"context_line":"    return schemas[\u0027DEFAULT\u0027]"},{"line_number":144,"context_line":""},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"def genpwd(passwords, schemas\u003dKOLLA_PASSWORDS_SCHEMAS):"},{"line_number":147,"context_line":"    for ansible_variable_key, password in passwords.items():"},{"line_number":148,"context_line":"        key_schema \u003d get_schema_for_key(schemas, ansible_variable_key)"},{"line_number":149,"context_line":"        if not key_schema[\u0027skip\u0027](password):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_9545d617","line":146,"range":{"start_line":146,"start_character":0,"end_line":146,"end_character":10},"in_reply_to":"9fdfeff1_5bfd6238","updated":"2019-02-11 08:01:15.000000000","message":"Actually I did but checking schema consistency is harder this way. Please see the other change with unit tests.","commit_id":"d275fc4a20f966f0a2117120e67c3e68350be2f8"},{"author":{"_account_id":26008,"name":"Bogdan Katynski","email":"bogdan.katynski@workday.com","username":"bodgix"},"change_message_id":"53327f204ec9717f7f569666ae329e22cab72b60","unresolved":false,"context_lines":[{"line_number":143,"context_line":"    return schemas[\u0027DEFAULT\u0027]"},{"line_number":144,"context_line":""},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"def genpwd(passwords, schemas\u003dKOLLA_PASSWORDS_SCHEMAS):"},{"line_number":147,"context_line":"    for ansible_variable_key, password in passwords.items():"},{"line_number":148,"context_line":"        key_schema \u003d get_schema_for_key(schemas, ansible_variable_key)"},{"line_number":149,"context_line":"        if not key_schema[\u0027skip\u0027](password):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_9883cba9","line":146,"range":{"start_line":146,"start_character":0,"end_line":146,"end_character":10},"in_reply_to":"9fdfeff1_9545d617","updated":"2019-02-11 10:26:40.000000000","message":"I don\u0027t think it\u0027s harder. You could initialize a let\u0027s say RsaPassword class with an empty private key / public key / both and see if a new one is generated, and in the next case, you initialize it with both private and public set and check if they\u0027re getting returned from the \"generator\".\n\nActually I think that those unit tests would be better because they would test actual implementations of the skip/generate logic for different kinds of passwords instead of mocked.\n\nI was looking at your unit tests and they actually only visit lines 149-152 but because you\u0027ve mocked the generators and skip functions, your unit tests do not check what I think are the most important things:\n\n- when is a new rsa_key generated and when is an existing one used\n- when is a new uuid token generated and when is an existing one used\n\netc.","commit_id":"d275fc4a20f966f0a2117120e67c3e68350be2f8"}]}