)]}'
{"networking_odl/common/config.py":[{"author":{"_account_id":333,"name":"Isaku Yamahata","email":"isaku.yamahata@gmail.com","username":"yamahata"},"change_message_id":"c457fc23c2fc22ed77b789c104112d51a7d1194a","unresolved":false,"context_lines":[{"line_number":67,"context_line":"    cfg.IntOpt(\u0027odl_features_retry_interval\u0027, default\u003d5,"},{"line_number":68,"context_line":"               help\u003d_(\"Wait this many seconds before retrying the odl features\""},{"line_number":69,"context_line":"                      \" fetch\")),"},{"line_number":70,"context_line":"    cfg.StrOpt(\u0027odl_features\u0027,"},{"line_number":71,"context_line":"               help\u003d\u0027A list or dict of features supported by ODL\u0027),"},{"line_number":72,"context_line":"    cfg.BoolOpt(\u0027enable_dhcp_service\u0027, default\u003dFalse,"},{"line_number":73,"context_line":"                help\u003d_(\u0027Enables the networking-odl driver to supply special\u0027"},{"line_number":74,"context_line":"                       \u0027 neutron ports of \"dhcp\" type to OpenDaylight\u0027"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_da064a21","line":71,"range":{"start_line":70,"start_character":0,"end_line":71,"end_character":67},"updated":"2018-05-01 17:44:28.000000000","message":"This is user visible change.\nSo release notes should be added.","commit_id":"fa576f26eb77ec036e0957d25df0dc9f8b3a4e3d"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"28c34c5f93a482efcabf98b0be594b23ccf5838b","unresolved":false,"context_lines":[{"line_number":67,"context_line":"    cfg.IntOpt(\u0027odl_features_retry_interval\u0027, default\u003d5,"},{"line_number":68,"context_line":"               help\u003d_(\"Wait this many seconds before retrying the odl features\""},{"line_number":69,"context_line":"                      \" fetch\")),"},{"line_number":70,"context_line":"    cfg.StrOpt(\u0027odl_features\u0027,"},{"line_number":71,"context_line":"               help\u003d\u0027A list or dict of features supported by ODL\u0027),"},{"line_number":72,"context_line":"    cfg.BoolOpt(\u0027enable_dhcp_service\u0027, default\u003dFalse,"},{"line_number":73,"context_line":"                help\u003d_(\u0027Enables the networking-odl driver to supply special\u0027"},{"line_number":74,"context_line":"                       \u0027 neutron ports of \"dhcp\" type to OpenDaylight\u0027"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_a4d574e7","line":71,"range":{"start_line":70,"start_character":0,"end_line":71,"end_character":67},"in_reply_to":"9f6a8fd7_da064a21","updated":"2018-05-02 09:16:17.000000000","message":"Yes, I wanted to do it and forgot. Thanks.\n\nDone.","commit_id":"fa576f26eb77ec036e0957d25df0dc9f8b3a4e3d"},{"author":{"_account_id":7921,"name":"Mike Kolesnik","email":"mkolesni@redhat.com","username":"mkolesni"},"change_message_id":"1a8c056bf8f4af1022b8f1d35dcca4536d99126d","unresolved":false,"context_lines":[{"line_number":71,"context_line":"                help\u003d\u0027A list of features supported by ODL.\u0027),"},{"line_number":72,"context_line":"    cfg.StrOpt(\u0027odl_features_json\u0027,"},{"line_number":73,"context_line":"               help\u003d\u0027Features supported by ODL, in the json format returned\u0027"},{"line_number":74,"context_line":"               \u0027by ODL. Note: this config option takes precedence over\u0027"},{"line_number":75,"context_line":"               \u0027odl_features.\u0027),"},{"line_number":76,"context_line":"    cfg.BoolOpt(\u0027enable_dhcp_service\u0027, default\u003dFalse,"},{"line_number":77,"context_line":"                help\u003d_(\u0027Enables the networking-odl driver to supply special\u0027"}],"source_content_type":"text/x-python","patch_set":11,"id":"5f7c97a3_8b1c6a77","line":74,"range":{"start_line":74,"start_character":49,"end_line":74,"end_character":70},"updated":"2018-06-27 10:20:17.000000000","message":"Please add a test for this","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"},{"author":{"_account_id":7921,"name":"Mike Kolesnik","email":"mkolesni@redhat.com","username":"mkolesni"},"change_message_id":"1a8c056bf8f4af1022b8f1d35dcca4536d99126d","unresolved":false,"context_lines":[{"line_number":71,"context_line":"                help\u003d\u0027A list of features supported by ODL.\u0027),"},{"line_number":72,"context_line":"    cfg.StrOpt(\u0027odl_features_json\u0027,"},{"line_number":73,"context_line":"               help\u003d\u0027Features supported by ODL, in the json format returned\u0027"},{"line_number":74,"context_line":"               \u0027by ODL. Note: this config option takes precedence over\u0027"},{"line_number":75,"context_line":"               \u0027odl_features.\u0027),"},{"line_number":76,"context_line":"    cfg.BoolOpt(\u0027enable_dhcp_service\u0027, default\u003dFalse,"},{"line_number":77,"context_line":"                help\u003d_(\u0027Enables the networking-odl driver to supply special\u0027"}],"source_content_type":"text/x-python","patch_set":11,"id":"5f7c97a3_397ed5e2","line":74,"range":{"start_line":74,"start_character":30,"end_line":74,"end_character":31},"updated":"2018-06-27 10:20:17.000000000","message":"nit: T","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"7cde2803a722562a7dd825a11955584377a7ba1a","unresolved":false,"context_lines":[{"line_number":71,"context_line":"                help\u003d\u0027A list of features supported by ODL.\u0027),"},{"line_number":72,"context_line":"    cfg.StrOpt(\u0027odl_features_json\u0027,"},{"line_number":73,"context_line":"               help\u003d\u0027Features supported by ODL, in the json format returned\u0027"},{"line_number":74,"context_line":"               \u0027by ODL. Note: this config option takes precedence over\u0027"},{"line_number":75,"context_line":"               \u0027odl_features.\u0027),"},{"line_number":76,"context_line":"    cfg.BoolOpt(\u0027enable_dhcp_service\u0027, default\u003dFalse,"},{"line_number":77,"context_line":"                help\u003d_(\u0027Enables the networking-odl driver to supply special\u0027"}],"source_content_type":"text/x-python","patch_set":11,"id":"5f7c97a3_5e94d2e7","line":74,"range":{"start_line":74,"start_character":30,"end_line":74,"end_character":31},"in_reply_to":"5f7c97a3_397ed5e2","updated":"2018-06-27 11:44:44.000000000","message":"Done","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"7cde2803a722562a7dd825a11955584377a7ba1a","unresolved":false,"context_lines":[{"line_number":71,"context_line":"                help\u003d\u0027A list of features supported by ODL.\u0027),"},{"line_number":72,"context_line":"    cfg.StrOpt(\u0027odl_features_json\u0027,"},{"line_number":73,"context_line":"               help\u003d\u0027Features supported by ODL, in the json format returned\u0027"},{"line_number":74,"context_line":"               \u0027by ODL. Note: this config option takes precedence over\u0027"},{"line_number":75,"context_line":"               \u0027odl_features.\u0027),"},{"line_number":76,"context_line":"    cfg.BoolOpt(\u0027enable_dhcp_service\u0027, default\u003dFalse,"},{"line_number":77,"context_line":"                help\u003d_(\u0027Enables the networking-odl driver to supply special\u0027"}],"source_content_type":"text/x-python","patch_set":11,"id":"5f7c97a3_7e974eec","line":74,"range":{"start_line":74,"start_character":49,"end_line":74,"end_character":70},"in_reply_to":"5f7c97a3_8b1c6a77","updated":"2018-06-27 11:44:44.000000000","message":"Done","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"}],"networking_odl/common/odl_features.py":[{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"45d8a65993a95502d1c83d2a102cacc51cc0d5be","unresolved":false,"context_lines":[{"line_number":43,"context_line":"    feature_configs \u003d None"},{"line_number":44,"context_line":""},{"line_number":45,"context_line":"    if cfg.CONF.ml2_odl.odl_features is not None:"},{"line_number":46,"context_line":"        feature_configs \u003d set(cfg.CONF.ml2_odl.odl_features)"},{"line_number":47,"context_line":"        return"},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"    wait_interval \u003d cfg.CONF.ml2_odl.odl_features_retry_interval"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f6a8fd7_6ee52438","line":46,"range":{"start_line":46,"start_character":26,"end_line":46,"end_character":29},"updated":"2018-04-30 08:08:23.000000000","message":"shouldn\u0027t be a dict?","commit_id":"71cf6adb5af377c4aa4c335cc9d949c30816b6e0"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"45d8a65993a95502d1c83d2a102cacc51cc0d5be","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    LOG.info(\u0027Retrieved ODL features %s\u0027, features)"},{"line_number":80,"context_line":"    response \u003d dict()"},{"line_number":81,"context_line":"    for feature in features[\u0027feature\u0027]:"},{"line_number":82,"context_line":"        cfg \u003d \u0027\u0027"},{"line_number":83,"context_line":"        if \u0027configuration\u0027 in feature:"},{"line_number":84,"context_line":"            cfg \u003d feature[\u0027configuration\u0027]"},{"line_number":85,"context_line":"        response[feature[\u0027service-provider-feature\u0027].split(\u0027:\u0027)[1]] \u003d cfg"},{"line_number":86,"context_line":"    return response"},{"line_number":87,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9f6a8fd7_eeca949f","line":84,"range":{"start_line":82,"start_character":14,"end_line":84,"end_character":42},"updated":"2018-04-30 08:08:23.000000000","message":"This can be rewritten as \n\ncfg \u003d feature.get(\u0027configuration\u0027, \u0027\u0027)","commit_id":"71cf6adb5af377c4aa4c335cc9d949c30816b6e0"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"45d8a65993a95502d1c83d2a102cacc51cc0d5be","unresolved":false,"context_lines":[{"line_number":82,"context_line":"        cfg \u003d \u0027\u0027"},{"line_number":83,"context_line":"        if \u0027configuration\u0027 in feature:"},{"line_number":84,"context_line":"            cfg \u003d feature[\u0027configuration\u0027]"},{"line_number":85,"context_line":"        response[feature[\u0027service-provider-feature\u0027].split(\u0027:\u0027)[1]] \u003d cfg"},{"line_number":86,"context_line":"    return response"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9f6a8fd7_2ea08ca5","line":85,"range":{"start_line":85,"start_character":17,"end_line":85,"end_character":66},"updated":"2018-04-30 08:08:23.000000000","message":"On a side not, it\u0027s not possible that two features will have a different prefix but will be called the same? e.g.:\n\nfoo:feature\nbar:feature\n\nBecause in that case (as it was also true before), then you\u0027d be overwriting the feature in the features dict.","commit_id":"71cf6adb5af377c4aa4c335cc9d949c30816b6e0"},{"author":{"_account_id":333,"name":"Isaku Yamahata","email":"isaku.yamahata@gmail.com","username":"yamahata"},"change_message_id":"9c63a60ee94fbb234f7ba61eb38d4e62738ca638","unresolved":false,"context_lines":[{"line_number":77,"context_line":"        return None"},{"line_number":78,"context_line":""},{"line_number":79,"context_line":"    LOG.info(\u0027Retrieved ODL features %s\u0027, features)"},{"line_number":80,"context_line":"    response \u003d dict()"},{"line_number":81,"context_line":"    for feature in features[\u0027feature\u0027]:"},{"line_number":82,"context_line":"        cfg \u003d \u0027\u0027"},{"line_number":83,"context_line":"        if \u0027configuration\u0027 in feature:"},{"line_number":84,"context_line":"            cfg \u003d feature[\u0027configuration\u0027]"},{"line_number":85,"context_line":"        response[feature[\u0027service-provider-feature\u0027].split(\u0027:\u0027)[1]] \u003d cfg"},{"line_number":86,"context_line":"    return response"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9f6a8fd7_a34f0591","line":85,"range":{"start_line":80,"start_character":0,"end_line":85,"end_character":73},"updated":"2018-04-26 17:06:52.000000000","message":"use dict comprehension. and dict.get method.\n\n{feature[\u0027service-provider-feature\u0027].split(\u0027:\u0027)[1]: feature.get(\u0027configuration\u0027) for feature in features[\u0027feature\u0027]}","commit_id":"71cf6adb5af377c4aa4c335cc9d949c30816b6e0"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"45d8a65993a95502d1c83d2a102cacc51cc0d5be","unresolved":false,"context_lines":[{"line_number":77,"context_line":"        return None"},{"line_number":78,"context_line":""},{"line_number":79,"context_line":"    LOG.info(\u0027Retrieved ODL features %s\u0027, features)"},{"line_number":80,"context_line":"    response \u003d dict()"},{"line_number":81,"context_line":"    for feature in features[\u0027feature\u0027]:"},{"line_number":82,"context_line":"        cfg \u003d \u0027\u0027"},{"line_number":83,"context_line":"        if \u0027configuration\u0027 in feature:"},{"line_number":84,"context_line":"            cfg \u003d feature[\u0027configuration\u0027]"},{"line_number":85,"context_line":"        response[feature[\u0027service-provider-feature\u0027].split(\u0027:\u0027)[1]] \u003d cfg"},{"line_number":86,"context_line":"    return response"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9f6a8fd7_ae201c4b","line":85,"range":{"start_line":80,"start_character":0,"end_line":85,"end_character":73},"in_reply_to":"9f6a8fd7_363fb188","updated":"2018-04-30 08:08:23.000000000","message":"That dict comprehension has a small \"bug\" (fixed below).\n\nRegarding advantages, a dict comprehension is usually faster than a for-loop. In this case the difference is probably minimal though. Other than that, it\u0027s a matter of style. \n\nI usually prefer dict/list/etc comprehensions, as they are easy to read, pythonic and performant.\n\n{feature[\u0027service-provider-feature\u0027].split(\u0027:\u0027)[1]: feature.get(\u0027configuration\u0027, \u0027\u0027) for feature in features[\u0027feature\u0027]}","commit_id":"71cf6adb5af377c4aa4c335cc9d949c30816b6e0"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"3a7e14e0a6c3cf7c4690a997a02c502fb4a2b74e","unresolved":false,"context_lines":[{"line_number":77,"context_line":"        return None"},{"line_number":78,"context_line":""},{"line_number":79,"context_line":"    LOG.info(\u0027Retrieved ODL features %s\u0027, features)"},{"line_number":80,"context_line":"    response \u003d dict()"},{"line_number":81,"context_line":"    for feature in features[\u0027feature\u0027]:"},{"line_number":82,"context_line":"        cfg \u003d \u0027\u0027"},{"line_number":83,"context_line":"        if \u0027configuration\u0027 in feature:"},{"line_number":84,"context_line":"            cfg \u003d feature[\u0027configuration\u0027]"},{"line_number":85,"context_line":"        response[feature[\u0027service-provider-feature\u0027].split(\u0027:\u0027)[1]] \u003d cfg"},{"line_number":86,"context_line":"    return response"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9f6a8fd7_363fb188","line":85,"range":{"start_line":80,"start_character":0,"end_line":85,"end_character":73},"in_reply_to":"9f6a8fd7_a34f0591","updated":"2018-04-26 17:30:29.000000000","message":"I really see no advantage to doing it that way. Can you please point to a concrete advantage to that?","commit_id":"71cf6adb5af377c4aa4c335cc9d949c30816b6e0"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"acd92db1d02f13bd2f98ed8b5d7165c12d71168b","unresolved":false,"context_lines":[{"line_number":32,"context_line":""},{"line_number":33,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"feature_configs \u003d DEFAULT_FEATURES"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_bfc84055","line":35,"range":{"start_line":35,"start_character":0,"end_line":35,"end_character":34},"updated":"2018-05-07 15:47:12.000000000","message":"Why two variables are needed?","commit_id":"fa576f26eb77ec036e0957d25df0dc9f8b3a4e3d"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"323a78d4acdd15b4ddef8b7256eccf4e372af6b8","unresolved":false,"context_lines":[{"line_number":32,"context_line":""},{"line_number":33,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"feature_configs \u003d DEFAULT_FEATURES"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_3aa67ec3","line":35,"range":{"start_line":35,"start_character":0,"end_line":35,"end_character":34},"in_reply_to":"5f7c97a3_bfc84055","updated":"2018-05-07 16:08:44.000000000","message":"one is a constant that holds the default value for features, the other is the actual global variable that holds the configured ones","commit_id":"fa576f26eb77ec036e0957d25df0dc9f8b3a4e3d"},{"author":{"_account_id":333,"name":"Isaku Yamahata","email":"isaku.yamahata@gmail.com","username":"yamahata"},"change_message_id":"c457fc23c2fc22ed77b789c104112d51a7d1194a","unresolved":false,"context_lines":[{"line_number":48,"context_line":"    config \u003d cfg.CONF.ml2_odl.odl_features"},{"line_number":49,"context_line":"    if config is not None:"},{"line_number":50,"context_line":"        try:"},{"line_number":51,"context_line":"            feature_configs \u003d types.Dict()(config)"},{"line_number":52,"context_line":"        except ValueError:"},{"line_number":53,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":54,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_ba1cb677","line":51,"range":{"start_line":51,"start_character":30,"end_line":51,"end_character":50},"updated":"2018-05-01 17:44:28.000000000","message":"This is strange.","commit_id":"fa576f26eb77ec036e0957d25df0dc9f8b3a4e3d"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"28c34c5f93a482efcabf98b0be594b23ccf5838b","unresolved":false,"context_lines":[{"line_number":48,"context_line":"    config \u003d cfg.CONF.ml2_odl.odl_features"},{"line_number":49,"context_line":"    if config is not None:"},{"line_number":50,"context_line":"        try:"},{"line_number":51,"context_line":"            feature_configs \u003d types.Dict()(config)"},{"line_number":52,"context_line":"        except ValueError:"},{"line_number":53,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":54,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_c4cf487d","line":51,"range":{"start_line":51,"start_character":30,"end_line":51,"end_character":50},"in_reply_to":"9f6a8fd7_ba1cb677","updated":"2018-05-02 09:16:17.000000000","message":"it\u0027s the way oslo_config types work.","commit_id":"fa576f26eb77ec036e0957d25df0dc9f8b3a4e3d"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"acd92db1d02f13bd2f98ed8b5d7165c12d71168b","unresolved":false,"context_lines":[{"line_number":67,"context_line":"    return feature in feature_configs"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"def get_config(feature):"},{"line_number":71,"context_line":"    return feature_configs[feature]"},{"line_number":72,"context_line":""},{"line_number":73,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_9f730481","line":70,"range":{"start_line":70,"start_character":0,"end_line":70,"end_character":24},"updated":"2018-05-07 15:47:12.000000000","message":"is this being used outside this module?","commit_id":"fa576f26eb77ec036e0957d25df0dc9f8b3a4e3d"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"323a78d4acdd15b4ddef8b7256eccf4e372af6b8","unresolved":false,"context_lines":[{"line_number":67,"context_line":"    return feature in feature_configs"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"def get_config(feature):"},{"line_number":71,"context_line":"    return feature_configs[feature]"},{"line_number":72,"context_line":""},{"line_number":73,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_7a9b9671","line":70,"range":{"start_line":70,"start_character":0,"end_line":70,"end_character":24},"in_reply_to":"5f7c97a3_9f730481","updated":"2018-05-07 16:08:44.000000000","message":"what\u0027s the matter? what difference would that make?","commit_id":"fa576f26eb77ec036e0957d25df0dc9f8b3a4e3d"},{"author":{"_account_id":333,"name":"Isaku Yamahata","email":"isaku.yamahata@gmail.com","username":"yamahata"},"change_message_id":"c457fc23c2fc22ed77b789c104112d51a7d1194a","unresolved":false,"context_lines":[{"line_number":86,"context_line":"    LOG.info(\u0027Retrieved ODL features %s\u0027, features)"},{"line_number":87,"context_line":"    response \u003d {}"},{"line_number":88,"context_line":"    for feature in features[\u0027feature\u0027]:"},{"line_number":89,"context_line":"        cfg \u003d feature.get(\u0027configuration\u0027, \u0027\u0027)"},{"line_number":90,"context_line":"        response[feature[\u0027service-provider-feature\u0027].split(\u0027:\u0027)[1]] \u003d cfg"},{"line_number":91,"context_line":"    return response"},{"line_number":92,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_1a24c2c5","line":89,"range":{"start_line":89,"start_character":42,"end_line":89,"end_character":45},"updated":"2018-05-01 17:44:28.000000000","message":"this should be get(\u0027configuration\u0027) (the default value is None)\n\nWith deafult value of \u0027\u0027, we can\u0027t distinguish between the two cases.\n- actually \u0027\u0027 is specified.\n- configuration value is missing.","commit_id":"fa576f26eb77ec036e0957d25df0dc9f8b3a4e3d"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"28c34c5f93a482efcabf98b0be594b23ccf5838b","unresolved":false,"context_lines":[{"line_number":86,"context_line":"    LOG.info(\u0027Retrieved ODL features %s\u0027, features)"},{"line_number":87,"context_line":"    response \u003d {}"},{"line_number":88,"context_line":"    for feature in features[\u0027feature\u0027]:"},{"line_number":89,"context_line":"        cfg \u003d feature.get(\u0027configuration\u0027, \u0027\u0027)"},{"line_number":90,"context_line":"        response[feature[\u0027service-provider-feature\u0027].split(\u0027:\u0027)[1]] \u003d cfg"},{"line_number":91,"context_line":"    return response"},{"line_number":92,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_24eac4ab","line":89,"range":{"start_line":89,"start_character":42,"end_line":89,"end_character":45},"in_reply_to":"9f6a8fd7_1a24c2c5","updated":"2018-05-02 09:16:17.000000000","message":"If you add a config file that has:\n\n\n[ml2_odl]\nodl_features \u003d feature1:, feature2:option2\n\n\nthen with oslo_config with a cfg.DictOpts then you\u0027d have dictionary like this:\n\n\n{ \u0027feature1\u0027: \u0027\u0027, \u0027feature2\u0027: \u0027option2\u0027 }\n\n\nAs the way you suggest is not how oslo_config\u0027s cfg.DictOpts and/or types.Dict works. Therefore, I recommend we leave it with an empty string, to have coherence.","commit_id":"fa576f26eb77ec036e0957d25df0dc9f8b3a4e3d"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"7da3c6ffef04386518102a03ed1cdf6d9600ee26","unresolved":false,"context_lines":[{"line_number":48,"context_line":"    config \u003d cfg.CONF.ml2_odl.odl_features"},{"line_number":49,"context_line":"    if config is not None:"},{"line_number":50,"context_line":"        try:"},{"line_number":51,"context_line":"            feature_configs \u003d types.Dict()(config)"},{"line_number":52,"context_line":"        except ValueError:"},{"line_number":53,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":54,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f6a8fd7_75caa41a","line":51,"updated":"2018-05-02 10:01:22.000000000","message":"I very much suspect that this will not work. The reason I say this is because it\u0027s not just that we need to support a dict format. We need to support a dict format where the value entry is itself a string of json.","commit_id":"deab2beecdc00ed463ee277442b63a491052b22a"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"e3a3e266a4f3a29e0e41568247a159bc7e8e0f1b","unresolved":false,"context_lines":[{"line_number":48,"context_line":"    config \u003d cfg.CONF.ml2_odl.odl_features"},{"line_number":49,"context_line":"    if config is not None:"},{"line_number":50,"context_line":"        try:"},{"line_number":51,"context_line":"            feature_configs \u003d types.Dict()(config)"},{"line_number":52,"context_line":"        except ValueError:"},{"line_number":53,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":54,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f6a8fd7_9007461d","line":51,"in_reply_to":"9f6a8fd7_75caa41a","updated":"2018-05-02 11:09:21.000000000","message":"why is that? I\u0027m asking because I don\u0027t like too much the idea of introducing into the configuration files configs that are not oslo_config parseable but yet still look similar enough, as it might add a lot of confusion.\n\nWhat you are proposing is a way to be able to inject exactly the JSON that ODL created to the configuration.","commit_id":"deab2beecdc00ed463ee277442b63a491052b22a"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"2f4dde6cb54efb53922dcb2534f7b9f610331cc1","unresolved":false,"context_lines":[{"line_number":48,"context_line":"    config \u003d cfg.CONF.ml2_odl.odl_features"},{"line_number":49,"context_line":"    if config is not None:"},{"line_number":50,"context_line":"        try:"},{"line_number":51,"context_line":"            feature_configs \u003d types.Dict()(config)"},{"line_number":52,"context_line":"        except ValueError:"},{"line_number":53,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":54,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f6a8fd7_fd5c56ee","line":51,"in_reply_to":"9f6a8fd7_9007461d","updated":"2018-05-02 13:29:35.000000000","message":"Because the basic idea behind this change is to be able to pass the QoS rule type configuration which is the list of \"qos features\" and their configuration to net_odl...but net_odl is agnostic to the contents, it just needs to pass it to the QoS driver. As such, the idea is to pass the json config straight to the driver. \n\nI don\u0027t see this as a violation of oslo config since the value is now a string...which can be json. In general the config value is a stand in for the values retrieved from odl so it makes sense to support the format odl sends, no?","commit_id":"deab2beecdc00ed463ee277442b63a491052b22a"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"029db0db3d10d041d47b38071088ddf55f50b461","unresolved":false,"context_lines":[{"line_number":48,"context_line":"    config \u003d cfg.CONF.ml2_odl.odl_features"},{"line_number":49,"context_line":"    if config is not None:"},{"line_number":50,"context_line":"        try:"},{"line_number":51,"context_line":"            feature_configs \u003d types.Dict()(config)"},{"line_number":52,"context_line":"        except ValueError:"},{"line_number":53,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":54,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f6a8fd7_8dcf277e","line":51,"in_reply_to":"9f6a8fd7_fd5c56ee","updated":"2018-05-02 16:21:41.000000000","message":"Makes a lot of sense. Done.","commit_id":"deab2beecdc00ed463ee277442b63a491052b22a"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"acd92db1d02f13bd2f98ed8b5d7165c12d71168b","unresolved":false,"context_lines":[{"line_number":33,"context_line":""},{"line_number":34,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"feature_configs \u003d DEFAULT_FEATURES"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_9f278413","line":36,"range":{"start_line":36,"start_character":0,"end_line":36,"end_character":34},"updated":"2018-05-07 15:47:12.000000000","message":"Could you please write comment, why these two variables are needed? Otherwise it will take a lot of time to understand the purpose.","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"0c8865f3abc89032c30c37a3ab5283d50a66f467","unresolved":false,"context_lines":[{"line_number":33,"context_line":""},{"line_number":34,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"feature_configs \u003d DEFAULT_FEATURES"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_baeef7b1","line":36,"range":{"start_line":36,"start_character":0,"end_line":36,"end_character":34},"in_reply_to":"5f7c97a3_3a943e37","updated":"2018-05-08 15:31:18.000000000","message":"What i meant was, to write comment on this line :). Why is this extra variable needed?\n\nI see two problem with this assignment\n\n1. When someone reads the code, it will take lot of time to understand, what is it being used for?\n2. Later when someone modify code then he/she has to think too much about dictionary mutation that it does not get overridden by feature_configs. However it will be identified by test, but why to write such an error prone code.\n\nWhat was the problem with existing way of using one variable?","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"2c94e7d192d3619d4815a91599dc4a6387d541d7","unresolved":false,"context_lines":[{"line_number":33,"context_line":""},{"line_number":34,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"feature_configs \u003d DEFAULT_FEATURES"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_dd8a7168","line":36,"range":{"start_line":36,"start_character":0,"end_line":36,"end_character":34},"in_reply_to":"5f7c97a3_8b6aa1ea","updated":"2018-05-13 03:58:13.000000000","message":"I don\u0027t think, it is personal coding aesthetics or personal taste. I would have considered personal taste, if it does not impact maintenance of the code.\n\nTo understand the purpose of DEFAULT_FEATURES, you have to understand the usage of feature_config usage as well and all the places \"feature_config\", is being modified. The reason is, dictionary is mutable in python. This single statement adds to much complexity to understand this code.\n\nHowever, i would really like to hear your thoughts, and will be happy to give +1 from my side to current code, if you have valid reason for it.","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"323a78d4acdd15b4ddef8b7256eccf4e372af6b8","unresolved":false,"context_lines":[{"line_number":33,"context_line":""},{"line_number":34,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"feature_configs \u003d DEFAULT_FEATURES"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_3a943e37","line":36,"range":{"start_line":36,"start_character":0,"end_line":36,"end_character":34},"in_reply_to":"5f7c97a3_9f278413","updated":"2018-05-07 16:08:44.000000000","message":"I\u0027ve explained in the previous comment.","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"fb8e2b9d948eaf1ccb74a677d9f2c3cb3e588afb","unresolved":false,"context_lines":[{"line_number":33,"context_line":""},{"line_number":34,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"feature_configs \u003d DEFAULT_FEATURES"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_f20106b1","line":36,"range":{"start_line":36,"start_character":0,"end_line":36,"end_character":34},"in_reply_to":"5f7c97a3_b83abf95","updated":"2018-05-21 07:11:05.000000000","message":"It\u0027s used to reset the provide a default value on deinit() and _fetch_features(). I think you are making a bigger mess that it really is, as the code is self explanatory.","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"3dd772008db1dd7fa7692e00ee7ef3ff6da4cd78","unresolved":false,"context_lines":[{"line_number":33,"context_line":""},{"line_number":34,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"feature_configs \u003d DEFAULT_FEATURES"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_8b6aa1ea","line":36,"range":{"start_line":36,"start_character":0,"end_line":36,"end_character":34},"in_reply_to":"5f7c97a3_baeef7b1","updated":"2018-05-10 06:55:57.000000000","message":"I think the code is fine the way it is. The DEFAULT_FEATURES is used below to reset the feature_configs when deinit is called as well. I don\u0027t think this requires a comment since there really is nothing confusing here. Getting into conversations which boil down to nothing more than arguments about personal coding aesthetics is a waste of time and a distraction. That being said, I don\u0027t think it is necessary to have DEFAULT_FEATURES and it would be OK to just have feature_configs \u003d {}. But again, that is just my personal taste.","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"ff841dd0371fdd50b75b93b00659e38a920dad43","unresolved":false,"context_lines":[{"line_number":33,"context_line":""},{"line_number":34,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"feature_configs \u003d DEFAULT_FEATURES"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_b83abf95","line":36,"range":{"start_line":36,"start_character":0,"end_line":36,"end_character":34},"in_reply_to":"5f7c97a3_d30c9495","updated":"2018-05-17 16:33:21.000000000","message":"My initial comment still hold, Initially i wanted you to add comment. Because nowhere it is mentioned that default features has to be put here, basically what is the significance of this variable. Sooner or later someone going to use it, If he/she has docs then they know what to do. Otherwise it seems over engineering, as i thought because DEFAULT_FEATURE is not required single {} would have been suffice.","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"86c3562e91a57a66ba9a5dc4781cffda733f757f","unresolved":false,"context_lines":[{"line_number":33,"context_line":""},{"line_number":34,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"feature_configs \u003d DEFAULT_FEATURES"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_d30c9495","line":36,"range":{"start_line":36,"start_character":0,"end_line":36,"end_character":34},"in_reply_to":"5f7c97a3_dd8a7168","updated":"2018-05-14 13:50:21.000000000","message":"mmm I don\u0027t know if this is what you meant before but actually something that you said resonated. \n\nThat can\u0027t work the way it is because any change to feature_config changes DEFAULT_FEATURES. What an oversight on my part!\n\nGood catch! I added test cases to avoid this in the future.","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"2a9214702aa38c243dda4f6bd4f3e2e5a886cd01","unresolved":false,"context_lines":[{"line_number":33,"context_line":""},{"line_number":34,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"feature_configs \u003d DEFAULT_FEATURES"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_362bd230","line":36,"range":{"start_line":36,"start_character":0,"end_line":36,"end_character":34},"in_reply_to":"5f7c97a3_e3984376","updated":"2018-05-23 07:31:28.000000000","message":"I\u0027m not the only one that\u0027s calling a code self explanatory, here Josh also stepped in and said the same.","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"4203e8a6f0645f0af026cc46f188e9509a0156a7","unresolved":false,"context_lines":[{"line_number":33,"context_line":""},{"line_number":34,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"feature_configs \u003d DEFAULT_FEATURES"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_e3984376","line":36,"range":{"start_line":36,"start_character":0,"end_line":36,"end_character":34},"in_reply_to":"5f7c97a3_f20106b1","updated":"2018-05-22 16:17:01.000000000","message":"Michel, sometimes a code is obvious and self explanatory to a person writing code but not for the other person later doing maintenance etc. That\u0027s all i can say because i can not produce some mathematically proof whether it is needed or not. It is very subject and depends on the perspective.\n\nIf you think, it is not needed then i won\u0027t argue more on this.","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"acd92db1d02f13bd2f98ed8b5d7165c12d71168b","unresolved":false,"context_lines":[{"line_number":70,"context_line":"    return feature in feature_configs"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"def get_config(feature):"},{"line_number":74,"context_line":"    return feature_configs[feature]"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_3f7e70e4","line":74,"range":{"start_line":73,"start_character":0,"end_line":74,"end_character":35},"updated":"2018-05-07 15:47:12.000000000","message":"is it being used outside the module?","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"2a9214702aa38c243dda4f6bd4f3e2e5a886cd01","unresolved":false,"context_lines":[{"line_number":70,"context_line":"    return feature in feature_configs"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"def get_config(feature):"},{"line_number":74,"context_line":"    return feature_configs[feature]"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_56bc4edb","line":74,"range":{"start_line":73,"start_character":0,"end_line":74,"end_character":35},"in_reply_to":"5f7c97a3_0388dfc3","updated":"2018-05-23 07:31:28.000000000","message":"Done","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"3dd772008db1dd7fa7692e00ee7ef3ff6da4cd78","unresolved":false,"context_lines":[{"line_number":70,"context_line":"    return feature in feature_configs"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"def get_config(feature):"},{"line_number":74,"context_line":"    return feature_configs[feature]"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_eb1a35ca","line":74,"range":{"start_line":73,"start_character":0,"end_line":74,"end_character":35},"in_reply_to":"5f7c97a3_3a174754","updated":"2018-05-10 06:55:57.000000000","message":"It will be used for QoS rule types. That code is on its way.","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"86c3562e91a57a66ba9a5dc4781cffda733f757f","unresolved":false,"context_lines":[{"line_number":70,"context_line":"    return feature in feature_configs"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"def get_config(feature):"},{"line_number":74,"context_line":"    return feature_configs[feature]"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_b639c6d7","line":74,"range":{"start_line":73,"start_character":0,"end_line":74,"end_character":35},"in_reply_to":"5f7c97a3_3a174754","updated":"2018-05-14 13:50:21.000000000","message":"Rajiv, I think there is a difference between creating a method that\u0027s only for tests and a method that\u0027s part of an API. This case is part of the API and it\u0027s clearly useful for the code to come.\n\nAs you I\u0027m against methods or functionality that exists only for a test case, but this is not the case.","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"323a78d4acdd15b4ddef8b7256eccf4e372af6b8","unresolved":false,"context_lines":[{"line_number":70,"context_line":"    return feature in feature_configs"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"def get_config(feature):"},{"line_number":74,"context_line":"    return feature_configs[feature]"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_5aac9a8b","line":74,"range":{"start_line":73,"start_character":0,"end_line":74,"end_character":35},"in_reply_to":"5f7c97a3_3f7e70e4","updated":"2018-05-07 16:08:44.000000000","message":"I\u0027ve replied to this too","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"0c8865f3abc89032c30c37a3ab5283d50a66f467","unresolved":false,"context_lines":[{"line_number":70,"context_line":"    return feature in feature_configs"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"def get_config(feature):"},{"line_number":74,"context_line":"    return feature_configs[feature]"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_3a174754","line":74,"range":{"start_line":73,"start_character":0,"end_line":74,"end_character":35},"in_reply_to":"5f7c97a3_5aac9a8b","updated":"2018-05-08 15:31:18.000000000","message":"Where is it being used except test cases? Why are we writing code for test case?\n\nIf later on that test is deleted then no one is going to remember this function to delete. It is unnecessary function.","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"fb8e2b9d948eaf1ccb74a677d9f2c3cb3e588afb","unresolved":false,"context_lines":[{"line_number":70,"context_line":"    return feature in feature_configs"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"def get_config(feature):"},{"line_number":74,"context_line":"    return feature_configs[feature]"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_d2308aa1","line":74,"range":{"start_line":73,"start_character":0,"end_line":74,"end_character":35},"in_reply_to":"5f7c97a3_788667de","updated":"2018-05-21 07:11:05.000000000","message":"by the name of the function, by the fact that it\u0027s a public function, by the fact that we can think and understand that it\u0027s something you are going to need in the future (taking into consideration that the commit is exactly to be able to support configs)","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"ff841dd0371fdd50b75b93b00659e38a920dad43","unresolved":false,"context_lines":[{"line_number":70,"context_line":"    return feature in feature_configs"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"def get_config(feature):"},{"line_number":74,"context_line":"    return feature_configs[feature]"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_788667de","line":74,"range":{"start_line":73,"start_character":0,"end_line":74,"end_character":35},"in_reply_to":"5f7c97a3_b639c6d7","updated":"2018-05-17 16:33:21.000000000","message":"Michel Agree. How do i know, it is an api or method for test. This is an API, is not conveyed anywhere(docs, comment or commit message).","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"4203e8a6f0645f0af026cc46f188e9509a0156a7","unresolved":false,"context_lines":[{"line_number":70,"context_line":"    return feature in feature_configs"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"def get_config(feature):"},{"line_number":74,"context_line":"    return feature_configs[feature]"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_0388dfc3","line":74,"range":{"start_line":73,"start_character":0,"end_line":74,"end_character":35},"in_reply_to":"5f7c97a3_d2308aa1","updated":"2018-05-22 16:17:01.000000000","message":"by the name of the function, by the fact that it\u0027s a public function\n\n\u003e These things are always present in the API code, does it mean, we should not write comment?\n\nby the fact that we can think and understand that it\u0027s something you are going to need in the future\n\n\u003e Well, if we leave the on going conversation, how does a person know, we are going to use it in future?\n\nEven commit message does not convey, we are providing new API for giving the configuration for a feature.\n\n\nBTW, For better understanding and knowing standards, i would like to know, \"When should, we document something and when we should not?\"\n\nI think, this conversation is getting too long, we have wasted unnecessary a lot of time on discussing it. Let\u0027s leave it. you can do, whatever you want, it is not worth stopping this patch for this comment.","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"acd92db1d02f13bd2f98ed8b5d7165c12d71168b","unresolved":false,"context_lines":[{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    LOG.info(\u0027Retrieved ODL features %s\u0027, features)"},{"line_number":90,"context_line":"    response \u003d {}"},{"line_number":91,"context_line":"    for feature in features[\u0027feature\u0027]:"},{"line_number":92,"context_line":"        cfg \u003d feature.get(\u0027configuration\u0027, \u0027\u0027)"},{"line_number":93,"context_line":"        response[feature[\u0027service-provider-feature\u0027].split(\u0027:\u0027)[1]] \u003d cfg"},{"line_number":94,"context_line":"    return response"},{"line_number":95,"context_line":""},{"line_number":96,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_7f8e2879","line":93,"range":{"start_line":91,"start_character":0,"end_line":93,"end_character":73},"updated":"2018-05-07 15:47:12.000000000","message":"This code is not readable. It might be readable to you because you have understood response from ODL but not for reviewers. If someone has to understand code then he/she has to refer OpenDaylight guide. This part definitely need documentation.","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"86c3562e91a57a66ba9a5dc4781cffda733f757f","unresolved":false,"context_lines":[{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    LOG.info(\u0027Retrieved ODL features %s\u0027, features)"},{"line_number":90,"context_line":"    response \u003d {}"},{"line_number":91,"context_line":"    for feature in features[\u0027feature\u0027]:"},{"line_number":92,"context_line":"        cfg \u003d feature.get(\u0027configuration\u0027, \u0027\u0027)"},{"line_number":93,"context_line":"        response[feature[\u0027service-provider-feature\u0027].split(\u0027:\u0027)[1]] \u003d cfg"},{"line_number":94,"context_line":"    return response"},{"line_number":95,"context_line":""},{"line_number":96,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_b648e62c","line":93,"range":{"start_line":91,"start_character":0,"end_line":93,"end_character":73},"in_reply_to":"5f7c97a3_05930ca5","updated":"2018-05-14 13:50:21.000000000","message":"You got me wrong, I don\u0027t like adding modifications that\u0027s not in the scope of the change. I prefer a different patch for those things. In this case it can be justified though.","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"0c8865f3abc89032c30c37a3ab5283d50a66f467","unresolved":false,"context_lines":[{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    LOG.info(\u0027Retrieved ODL features %s\u0027, features)"},{"line_number":90,"context_line":"    response \u003d {}"},{"line_number":91,"context_line":"    for feature in features[\u0027feature\u0027]:"},{"line_number":92,"context_line":"        cfg \u003d feature.get(\u0027configuration\u0027, \u0027\u0027)"},{"line_number":93,"context_line":"        response[feature[\u0027service-provider-feature\u0027].split(\u0027:\u0027)[1]] \u003d cfg"},{"line_number":94,"context_line":"    return response"},{"line_number":95,"context_line":""},{"line_number":96,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_05930ca5","line":93,"range":{"start_line":91,"start_character":0,"end_line":93,"end_character":73},"in_reply_to":"5f7c97a3_5ada5a17","updated":"2018-05-08 15:31:18.000000000","message":"I am reviewing now, how should i check that this code is correct or not? It would have been easier for you to put the reference with comment then to make reviewer to find docs. Anyway, i think, this code is changed and adding comment does not do any harm. If a mistake has been committed, it does not mean, it has to be done next time as well.\n\nI expect at least from you, if you touch code, it will be in better condition than it was :)","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"323a78d4acdd15b4ddef8b7256eccf4e372af6b8","unresolved":false,"context_lines":[{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    LOG.info(\u0027Retrieved ODL features %s\u0027, features)"},{"line_number":90,"context_line":"    response \u003d {}"},{"line_number":91,"context_line":"    for feature in features[\u0027feature\u0027]:"},{"line_number":92,"context_line":"        cfg \u003d feature.get(\u0027configuration\u0027, \u0027\u0027)"},{"line_number":93,"context_line":"        response[feature[\u0027service-provider-feature\u0027].split(\u0027:\u0027)[1]] \u003d cfg"},{"line_number":94,"context_line":"    return response"},{"line_number":95,"context_line":""},{"line_number":96,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_5ada5a17","line":93,"range":{"start_line":91,"start_character":0,"end_line":93,"end_character":73},"in_reply_to":"5f7c97a3_7f8e2879","updated":"2018-05-07 16:08:44.000000000","message":"considering that this was the code before and that this actually doesn\u0027t change it much, you are welcome to add another patch that documents that part :)","commit_id":"48c64d8e6d6ba70e539277e8eb7475f304715f18"},{"author":{"_account_id":7921,"name":"Mike Kolesnik","email":"mkolesni@redhat.com","username":"mkolesni"},"change_message_id":"4f7c26d66cbd3bed91d46028ae5f4ab9e76aafdf","unresolved":false,"context_lines":[{"line_number":32,"context_line":""},{"line_number":33,"context_line":"OPERATIONAL_PORT_STATUS \u003d \u0027operational-port-status\u0027"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"feature_configs \u003d copy.copy(DEFAULT_FEATURES)"},{"line_number":38,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_611dcbbc","line":35,"range":{"start_line":35,"start_character":19,"end_line":35,"end_character":21},"updated":"2018-06-11 09:14:52.000000000","message":"Wouldn\u0027t it make more sense to use a (currently empty) frozenset and each time you need it build a dict from it?","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":7921,"name":"Mike Kolesnik","email":"mkolesni@redhat.com","username":"mkolesni"},"change_message_id":"1a8c056bf8f4af1022b8f1d35dcca4536d99126d","unresolved":false,"context_lines":[{"line_number":32,"context_line":""},{"line_number":33,"context_line":"OPERATIONAL_PORT_STATUS \u003d \u0027operational-port-status\u0027"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"feature_configs \u003d copy.copy(DEFAULT_FEATURES)"},{"line_number":38,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_f954dd55","line":35,"range":{"start_line":35,"start_character":19,"end_line":35,"end_character":21},"in_reply_to":"5f7c97a3_611dcbbc","updated":"2018-06-27 10:20:17.000000000","message":"This wasn\u0027t addressed.. :(","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"2b4fbfb4ec7cfb0c2aabda0db45f4b9e466a721d","unresolved":false,"context_lines":[{"line_number":34,"context_line":""},{"line_number":35,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"feature_configs \u003d copy.copy(DEFAULT_FEATURES)"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_f3b2f0e4","line":37,"range":{"start_line":37,"start_character":0,"end_line":37,"end_character":45},"updated":"2018-05-30 14:28:06.000000000","message":"I thought about our discussion(What happen if opendaylight does not have needed https://git.opendaylight.org/gerrit/#/c/70331/)\n\nDo you think, keeping default value at driver side, will be a good idea, rather than in this module?\n\nWhy i think do?\nLet driver handle its default.\n\nWhat do you think, it is my initial thought?","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"bf64a1dab616997082a787655d65ce38bc2ece39","unresolved":false,"context_lines":[{"line_number":34,"context_line":""},{"line_number":35,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"feature_configs \u003d copy.copy(DEFAULT_FEATURES)"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_5d114a5c","line":37,"range":{"start_line":37,"start_character":0,"end_line":37,"end_character":45},"in_reply_to":"5f7c97a3_1d2992e4","updated":"2018-06-01 10:17:24.000000000","message":"Is there any scenario, where value of DEFAULT_FEATURES, can be other than {}?","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"ffb7167f4704dc4f7f6ee4d7dc536dc8a84a3bb2","unresolved":false,"context_lines":[{"line_number":34,"context_line":""},{"line_number":35,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"feature_configs \u003d copy.copy(DEFAULT_FEATURES)"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_9c3b2dc7","line":37,"range":{"start_line":37,"start_character":0,"end_line":37,"end_character":45},"in_reply_to":"5f7c97a3_5d114a5c","updated":"2018-06-05 08:12:19.000000000","message":"Not for now","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"e94b5bbf3f0e1e49d62ab5c5b296fa295c1a0e54","unresolved":false,"context_lines":[{"line_number":34,"context_line":""},{"line_number":35,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"feature_configs \u003d copy.copy(DEFAULT_FEATURES)"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_c75ea175","line":37,"range":{"start_line":37,"start_character":0,"end_line":37,"end_character":45},"in_reply_to":"5f7c97a3_911c6b14","updated":"2018-06-05 17:51:29.000000000","message":"Got it.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"2531cf804ba251adc55568cfc8c2e7548e8fdcbe","unresolved":false,"context_lines":[{"line_number":34,"context_line":""},{"line_number":35,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"feature_configs \u003d copy.copy(DEFAULT_FEATURES)"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_f1259faf","line":37,"range":{"start_line":37,"start_character":0,"end_line":37,"end_character":45},"in_reply_to":"5f7c97a3_9c3b2dc7","updated":"2018-06-05 15:54:09.000000000","message":"If we don\u0027t have any scenario then why are we write un necessary copy.copy and writing test cases to verify they are different. Rather just use {}. I think, this way things will be simpler.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"d51a499c8a19a23269cd7bd4fe5943fcfcca3db1","unresolved":false,"context_lines":[{"line_number":34,"context_line":""},{"line_number":35,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"feature_configs \u003d copy.copy(DEFAULT_FEATURES)"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_911c6b14","line":37,"range":{"start_line":37,"start_character":0,"end_line":37,"end_character":45},"in_reply_to":"5f7c97a3_f1259faf","updated":"2018-06-05 16:05:43.000000000","message":"thing is it\u0027s being used in several places (^F for it) and as you can see in the first patchsets Josh forgot to change the instances where it was used. This is to avoid that in the future. It\u0027s more maintainable.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"f26312096fca72255ef1bcfa50e1ce3755fbe254","unresolved":false,"context_lines":[{"line_number":34,"context_line":""},{"line_number":35,"context_line":"DEFAULT_FEATURES \u003d {}"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"feature_configs \u003d copy.copy(DEFAULT_FEATURES)"},{"line_number":38,"context_line":""},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"def init():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_1d2992e4","line":37,"range":{"start_line":37,"start_character":0,"end_line":37,"end_character":45},"in_reply_to":"5f7c97a3_f3b2f0e4","updated":"2018-06-01 09:52:31.000000000","message":"Each driver will know how to react to the lack of a configuration retrieved from this module. Thus in fact the default value is left to the driver.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":7921,"name":"Mike Kolesnik","email":"mkolesni@redhat.com","username":"mkolesni"},"change_message_id":"4f7c26d66cbd3bed91d46028ae5f4ab9e76aafdf","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    if config is not None:"},{"line_number":52,"context_line":"        try:"},{"line_number":53,"context_line":"            json \u003d jsonutils.loads(config)"},{"line_number":54,"context_line":"        except ValueError:"},{"line_number":55,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":56,"context_line":"        else:"},{"line_number":57,"context_line":"            feature_configs \u003d _load_features(json)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_268fe923","line":54,"range":{"start_line":54,"start_character":8,"end_line":54,"end_character":26},"updated":"2018-06-11 09:14:52.000000000","message":"I have a problem with this logic..\n\nIf the json is broken (i.e. typo from the user in the config file) then you\u0027ll try to load it as list, which would succeed but not in the way you wanted..\n\ni.e. \n  config\u003d\u0027{\"dummy\": \"dummy\" \"bubu\": \"bubu\"}\u0027\nwill result in that string loaded as a single item list..","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"52ed5162a11b674c7d17fef90724fb35f05a69cc","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    if config is not None:"},{"line_number":52,"context_line":"        try:"},{"line_number":53,"context_line":"            json \u003d jsonutils.loads(config)"},{"line_number":54,"context_line":"        except ValueError:"},{"line_number":55,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":56,"context_line":"        else:"},{"line_number":57,"context_line":"            feature_configs \u003d _load_features(json)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_72d6f551","line":54,"range":{"start_line":54,"start_character":8,"end_line":54,"end_character":26},"in_reply_to":"5f7c97a3_268fe923","updated":"2018-06-11 15:48:29.000000000","message":"Good catch. I\u0027m removing backwards compatibility, which I\u0027m not even sure we need.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"ffa2dc990215f0cd51e44555a4c31397305c2bf8","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    if config is not None:"},{"line_number":52,"context_line":"        try:"},{"line_number":53,"context_line":"            json \u003d jsonutils.loads(config)"},{"line_number":54,"context_line":"        except ValueError:"},{"line_number":55,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":56,"context_line":"        else:"},{"line_number":57,"context_line":"            feature_configs \u003d _load_features(json)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_cc4c1341","line":54,"range":{"start_line":54,"start_character":8,"end_line":54,"end_character":26},"in_reply_to":"5f7c97a3_268fe923","updated":"2018-06-11 10:36:51.000000000","message":"So how about the following?\nif jsonutils.loads fails\n   if config[0] \u003d\u003d \u0027{\u0027\n      then we assume that it was supposed to be json and\n      consider it an error condition\n   else\n      parse as list","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"78c180f95feef16662c38683b05690039a392417","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    if config is not None:"},{"line_number":52,"context_line":"        try:"},{"line_number":53,"context_line":"            json \u003d jsonutils.loads(config)"},{"line_number":54,"context_line":"        except ValueError:"},{"line_number":55,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":56,"context_line":"        else:"},{"line_number":57,"context_line":"            feature_configs \u003d _load_features(json)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_f172f649","line":54,"range":{"start_line":54,"start_character":8,"end_line":54,"end_character":26},"in_reply_to":"5f7c97a3_72d6f551","updated":"2018-06-12 02:54:51.000000000","message":"Then we need to (a) clearly document the format and (b) we need to notify the tripple-o folks who currently configure a list of items during installation.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"18e0a0753610be7005c57f6cc47190e8582442d6","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    if config is not None:"},{"line_number":52,"context_line":"        try:"},{"line_number":53,"context_line":"            json \u003d jsonutils.loads(config)"},{"line_number":54,"context_line":"        except ValueError:"},{"line_number":55,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":56,"context_line":"        else:"},{"line_number":57,"context_line":"            feature_configs \u003d _load_features(json)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_a9b2aa04","line":54,"range":{"start_line":54,"start_character":8,"end_line":54,"end_character":26},"in_reply_to":"5f7c97a3_7341e538","updated":"2018-06-13 12:29:05.000000000","message":"http://codesearch.openstack.org/?q\u003dodl_features\u0026i\u003dnope\u0026files\u003d\u0026repos\u003d\n\nAccording to that, apparently there are a number of usages... \n\nAlso, from reading these usages, it\u0027s going to be much more complex to use a json instead of the list (or a dict for that matter). \n\nPerhaps we should stick to using the list or a dict...","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"a4710564bd277a783361397bd660b7095c062f39","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    if config is not None:"},{"line_number":52,"context_line":"        try:"},{"line_number":53,"context_line":"            json \u003d jsonutils.loads(config)"},{"line_number":54,"context_line":"        except ValueError:"},{"line_number":55,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":56,"context_line":"        else:"},{"line_number":57,"context_line":"            feature_configs \u003d _load_features(json)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_90e6fcaf","line":54,"range":{"start_line":54,"start_character":8,"end_line":54,"end_character":26},"in_reply_to":"5f7c97a3_75cc7260","updated":"2018-06-13 07:10:28.000000000","message":"Personally, I think that elegance is overrated but I would be happy to go with either of Mike\u0027s other suggestions, specifying that json strings in the config value need to start with \"json:\" or a separate config value for json.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"a52487d0fd7bce5a9caabc4ccf551f134ebb8585","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    if config is not None:"},{"line_number":52,"context_line":"        try:"},{"line_number":53,"context_line":"            json \u003d jsonutils.loads(config)"},{"line_number":54,"context_line":"        except ValueError:"},{"line_number":55,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":56,"context_line":"        else:"},{"line_number":57,"context_line":"            feature_configs \u003d _load_features(json)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_75cc7260","line":54,"range":{"start_line":54,"start_character":8,"end_line":54,"end_character":26},"in_reply_to":"5f7c97a3_8c14c54e","updated":"2018-06-13 05:51:07.000000000","message":"As Mike pointed out when we discussed that, it\u0027s not an elegant solution. For example, if there is a typo and the admin misses the \u0027{\u0027 at the beginning we hit the same issue.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"4954963b523bfba27883023564748ab48256fe59","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    if config is not None:"},{"line_number":52,"context_line":"        try:"},{"line_number":53,"context_line":"            json \u003d jsonutils.loads(config)"},{"line_number":54,"context_line":"        except ValueError:"},{"line_number":55,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":56,"context_line":"        else:"},{"line_number":57,"context_line":"            feature_configs \u003d _load_features(json)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_e7dc5346","line":54,"range":{"start_line":54,"start_character":8,"end_line":54,"end_character":26},"in_reply_to":"5f7c97a3_90e6fcaf","updated":"2018-06-13 08:24:44.000000000","message":"why is it so important to still have backwards compatibility? AFAIK this is not being used yet and there is not a sensible reason to have both formats -which brings about additional coding complexity for no purpose.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"d98060ee8f68586613d8a2e1a8a587eb5a706ff3","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    if config is not None:"},{"line_number":52,"context_line":"        try:"},{"line_number":53,"context_line":"            json \u003d jsonutils.loads(config)"},{"line_number":54,"context_line":"        except ValueError:"},{"line_number":55,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":56,"context_line":"        else:"},{"line_number":57,"context_line":"            feature_configs \u003d _load_features(json)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_7341e538","line":54,"range":{"start_line":54,"start_character":8,"end_line":54,"end_character":26},"in_reply_to":"5f7c97a3_e7dc5346","updated":"2018-06-13 10:42:53.000000000","message":"It *is* being used...by us...in triple-o","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"230fd1c892a823148c7eebc71324b63b6d0f6382","unresolved":false,"context_lines":[{"line_number":51,"context_line":"    if config is not None:"},{"line_number":52,"context_line":"        try:"},{"line_number":53,"context_line":"            json \u003d jsonutils.loads(config)"},{"line_number":54,"context_line":"        except ValueError:"},{"line_number":55,"context_line":"            feature_configs \u003d {feature: \u0027\u0027 for feature in types.List()(config)}"},{"line_number":56,"context_line":"        else:"},{"line_number":57,"context_line":"            feature_configs \u003d _load_features(json)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_8c14c54e","line":54,"range":{"start_line":54,"start_character":8,"end_line":54,"end_character":26},"in_reply_to":"5f7c97a3_f172f649","updated":"2018-06-12 04:48:51.000000000","message":"I changed my mind. I am against breaking backwards compatibility. I see no reason not to check that the first char is a \u0027{\u0027 which is a clear indication of the user\u0027s intentions. Why not.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"3c46400ce347ef7090113195a252f7196ae7061c","unresolved":false,"context_lines":[{"line_number":72,"context_line":""},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"def get_config(feature):"},{"line_number":75,"context_line":"    return feature_configs[feature]"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_c616c2a8","line":75,"range":{"start_line":75,"start_character":0,"end_line":75,"end_character":35},"updated":"2018-06-04 15:17:05.000000000","message":"If a feature does not exist, it returns KeyError. Is it intended? If it is, does this exception makes sense for external module?","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"61ff907dff9f3e2f3682af9311a51c4060bdf872","unresolved":false,"context_lines":[{"line_number":72,"context_line":""},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"def get_config(feature):"},{"line_number":75,"context_line":"    return feature_configs[feature]"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_f6f02c00","line":75,"range":{"start_line":75,"start_character":0,"end_line":75,"end_character":35},"in_reply_to":"5f7c97a3_67e46d0f","updated":"2018-06-05 21:53:58.000000000","message":"Forget if it\u0027s a dictionary or not... Think about what you give the function... A key to look at a configuration value... The key is not found KeyError... Even if you changed the datastructure it will still make sense to return KeyError if it\u0027s not found...","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"d51a499c8a19a23269cd7bd4fe5943fcfcca3db1","unresolved":false,"context_lines":[{"line_number":72,"context_line":""},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"def get_config(feature):"},{"line_number":75,"context_line":"    return feature_configs[feature]"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_d1b8a3e6","line":75,"range":{"start_line":75,"start_character":0,"end_line":75,"end_character":35},"in_reply_to":"5f7c97a3_71674fec","updated":"2018-06-05 16:05:43.000000000","message":"You are calling get_config with a key to look for. The key is not found and such KeyError is thrown. It makes a lot of sense to me -you look for a key, the key is not found, you get a KeyError.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"f0ab1362134201e3c2b8cbffd345269afef960b3","unresolved":false,"context_lines":[{"line_number":72,"context_line":""},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"def get_config(feature):"},{"line_number":75,"context_line":"    return feature_configs[feature]"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_5b2a37e7","line":75,"range":{"start_line":75,"start_character":0,"end_line":75,"end_character":35},"in_reply_to":"5f7c97a3_754f2934","updated":"2018-06-07 14:28:08.000000000","message":"BTW it is not the answer of the questions, i asked.\n\n\u003e Because this is python and python programmers understand what a KeyError is\n\nI read it(Raised when a mapping (dictionary) key is not found in the set of existing keys.) and examples as well. I still do not believe, you are doing correct thing here. This is an external interface, and feature is not Key to external interface. What we does inside a module stays inside it, should not emit its implementation details outside(like used DS). Tomorrow, if list is used as a data structure then also, we are going to raise KeyError, what will key signify there. What is the relation between key and feature?\n\nIf above reasons are not enough, This is single line function and if it had multiple calls to methods and there any of them failed due to KeyError, an external system will think feature is not found, and real cause will never be known. It will possible mask some of the issues.\n\n\u003e While this is written as a generic module, it is only used in handful of places in networking_odl. If we ever export this as a generic API, perhaps we could have a FeatureNotFound.\n\nIt is interesting, it is a written as generic module but does not have generic API. I don\u0027t know, why this is not a generic API. What differentiate this API from generic API? How do we draw a line and if we drew, how does it fall on other side of it.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"40fd27fa4a0d8c6caf4d94c3ac055d54adb37f25","unresolved":false,"context_lines":[{"line_number":72,"context_line":""},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"def get_config(feature):"},{"line_number":75,"context_line":"    return feature_configs[feature]"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_754f2934","line":75,"range":{"start_line":75,"start_character":0,"end_line":75,"end_character":35},"in_reply_to":"5f7c97a3_89df52d5","updated":"2018-06-06 08:29:49.000000000","message":"Because this is python and python programmers understand what a KeyError is. While this is written as a generic module, it is only used in a handful of places in networking_odl. If we ever export this as a generic API, perhaps we could have a FeatureNotFound. For now, this really should be sufficient.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"ffb7167f4704dc4f7f6ee4d7dc536dc8a84a3bb2","unresolved":false,"context_lines":[{"line_number":72,"context_line":""},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"def get_config(feature):"},{"line_number":75,"context_line":"    return feature_configs[feature]"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_fc7341b3","line":75,"range":{"start_line":75,"start_character":0,"end_line":75,"end_character":35},"in_reply_to":"5f7c97a3_c616c2a8","updated":"2018-06-05 08:12:19.000000000","message":"It does make sense, yes.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"e94b5bbf3f0e1e49d62ab5c5b296fa295c1a0e54","unresolved":false,"context_lines":[{"line_number":72,"context_line":""},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"def get_config(feature):"},{"line_number":75,"context_line":"    return feature_configs[feature]"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_67e46d0f","line":75,"range":{"start_line":75,"start_character":0,"end_line":75,"end_character":35},"in_reply_to":"5f7c97a3_d1b8a3e6","updated":"2018-06-05 17:51:29.000000000","message":"Why an external module should know that getting a configuration is fetched from dictionary.\n\nWhy that module should know about it\u0027s internal data structure or even it\u0027s implementation? Tomorrow(like this patch changes from set to dictionary) if other data structure is used all the client of this module will have to change.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"fd963b7ecc07883bf6d9e8c38ee13b7c73d199f9","unresolved":false,"context_lines":[{"line_number":72,"context_line":""},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"def get_config(feature):"},{"line_number":75,"context_line":"    return feature_configs[feature]"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_89df52d5","line":75,"range":{"start_line":75,"start_character":0,"end_line":75,"end_character":35},"in_reply_to":"5f7c97a3_f6f02c00","updated":"2018-06-06 04:23:03.000000000","message":"I am thinking about this function following way.\n\nIt is function which takes a feature name and returns me feature configuration, so if a feature is not found then it should return FeatureNotFound.\n\nWhy are we thinking in term of keys? What has keys to do outside the module?","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"2531cf804ba251adc55568cfc8c2e7548e8fdcbe","unresolved":false,"context_lines":[{"line_number":72,"context_line":""},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"def get_config(feature):"},{"line_number":75,"context_line":"    return feature_configs[feature]"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"def deinit():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_71674fec","line":75,"range":{"start_line":75,"start_character":0,"end_line":75,"end_character":35},"in_reply_to":"5f7c97a3_fc7341b3","updated":"2018-06-05 15:54:09.000000000","message":"I think, it should throw some other exception like FeatureNotFound or may be return None(indicating, i don\u0027t have any configuration for this feature). This sounds weird, we are fetching configuration from get_config if configuration does not exist it throws KeyError. Here KeyError does not indicate correct message to caller.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"802ba97d44af684f6f07b56d4bd6a1db5e8b40f6","unresolved":false,"context_lines":[{"line_number":47,"context_line":"    feature_configs \u003d None"},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"    if cfg.CONF.ml2_odl.odl_features_json is not None:"},{"line_number":50,"context_line":"        json \u003d jsonutils.loads(cfg.CONF.ml2_odl.odl_features_json)"},{"line_number":51,"context_line":"        feature_configs \u003d _load_features(json)"},{"line_number":52,"context_line":"        return"},{"line_number":53,"context_line":""}],"source_content_type":"text/x-python","patch_set":14,"id":"5f7c97a3_e118c884","line":50,"range":{"start_line":50,"start_character":8,"end_line":50,"end_character":66},"updated":"2018-07-09 08:30:10.000000000","message":"Considering that we receive a string and that string can be malformed and not parsed by the jsonutils, I\u0027d say we should raise an exception (ConfigFileValueError) if this happens.","commit_id":"1ca4cff225b734e5040a7aa07cabde2d749c20bd"}],"networking_odl/tests/base.py":[{"author":{"_account_id":7921,"name":"Mike Kolesnik","email":"mkolesni@redhat.com","username":"mkolesni"},"change_message_id":"1a8c056bf8f4af1022b8f1d35dcca4536d99126d","unresolved":false,"context_lines":[{"line_number":75,"context_line":"        if cfg.CONF.ml2_odl.password is None:"},{"line_number":76,"context_line":"            self.cfg.config(password\u003d\u0027somepass\u0027, group\u003d\u0027ml2_odl\u0027)"},{"line_number":77,"context_line":"        # make sure _fetch_features is not called, it\u0027ll block the main thread"},{"line_number":78,"context_line":"        self.cfg.config(odl_features\u003d\u0027{\"features\": {\"feature\": []}}\u0027,"},{"line_number":79,"context_line":"                        group\u003d\u0027ml2_odl\u0027)"},{"line_number":80,"context_line":"        odl_features.init()"},{"line_number":81,"context_line":"        self.addCleanup(odl_features.deinit)"}],"source_content_type":"text/x-python","patch_set":11,"id":"5f7c97a3_d945619c","line":78,"range":{"start_line":78,"start_character":24,"end_line":78,"end_character":36},"updated":"2018-06-27 10:20:17.000000000","message":"odl_features_json?","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"7cde2803a722562a7dd825a11955584377a7ba1a","unresolved":false,"context_lines":[{"line_number":75,"context_line":"        if cfg.CONF.ml2_odl.password is None:"},{"line_number":76,"context_line":"            self.cfg.config(password\u003d\u0027somepass\u0027, group\u003d\u0027ml2_odl\u0027)"},{"line_number":77,"context_line":"        # make sure _fetch_features is not called, it\u0027ll block the main thread"},{"line_number":78,"context_line":"        self.cfg.config(odl_features\u003d\u0027{\"features\": {\"feature\": []}}\u0027,"},{"line_number":79,"context_line":"                        group\u003d\u0027ml2_odl\u0027)"},{"line_number":80,"context_line":"        odl_features.init()"},{"line_number":81,"context_line":"        self.addCleanup(odl_features.deinit)"}],"source_content_type":"text/x-python","patch_set":11,"id":"5f7c97a3_fe827ea4","line":78,"range":{"start_line":78,"start_character":24,"end_line":78,"end_character":36},"in_reply_to":"5f7c97a3_d945619c","updated":"2018-06-27 11:44:44.000000000","message":"Done","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"}],"networking_odl/tests/unit/common/test_odl_features.py":[{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"45d8a65993a95502d1c83d2a102cacc51cc0d5be","unresolved":false,"context_lines":[{"line_number":70,"context_line":"        mocked_client.return_value \u003d response"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":"        odl_features.init()"},{"line_number":73,"context_line":"        self.assertTrue(odl_features.has(odl_features.OPERATIONAL_PORT_STATUS))"},{"line_number":74,"context_line":"        self.assertEqual(odl_features.get_config("},{"line_number":75,"context_line":"            odl_features.OPERATIONAL_PORT_STATUS), \u0027\u0027)"},{"line_number":76,"context_line":"        self.assertTrue(odl_features.has(\u0027feature-with-config\u0027))"},{"line_number":77,"context_line":"        self.assertEqual(odl_features.get_config("},{"line_number":78,"context_line":"            \u0027feature-with-config\u0027), \u0027steal-your-face\u0027)"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":"    def test_init_from_config(self):"},{"line_number":81,"context_line":"        self.cfg.config(odl_features\u003d\u0027thing1,thing2\u0027, group\u003d\u0027ml2_odl\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f6a8fd7_ce79d02f","line":78,"range":{"start_line":73,"start_character":5,"end_line":78,"end_character":54},"updated":"2018-04-30 08:08:23.000000000","message":"I prefer separation of tests, not to have a one-test-it-all test, i.e.: have tests for init, has() and get_config() separately.","commit_id":"71cf6adb5af377c4aa4c335cc9d949c30816b6e0"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"b58dc74b2f5c68aac97b9b88d1f3f1f7f2605309","unresolved":false,"context_lines":[{"line_number":91,"context_line":"            \u0027thing2\u0027: \u0027\u0027,"},{"line_number":92,"context_line":"        })"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"    def test_init_from_config_dict(self):"},{"line_number":95,"context_line":"        self.cfg.config(odl_features\u003dself.feature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"        self._assert_odl_feature_config({"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_af2da5a9","line":94,"updated":"2018-05-03 08:23:21.000000000","message":"shouldn\u0027t this be changed to handle the json format, not the dict?","commit_id":"c677bce2ff38b3e6263d15642339598ff01e8714"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"7a7814a30df97af0c23c82ffd5d634c4cc97bfd0","unresolved":false,"context_lines":[{"line_number":91,"context_line":"            \u0027thing2\u0027: \u0027\u0027,"},{"line_number":92,"context_line":"        })"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"    def test_init_from_config_dict(self):"},{"line_number":95,"context_line":"        self.cfg.config(odl_features\u003dself.feature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"        self._assert_odl_feature_config({"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_30722ce7","line":94,"in_reply_to":"5f7c97a3_af2da5a9","updated":"2018-05-07 07:49:23.000000000","message":"Done","commit_id":"c677bce2ff38b3e6263d15642339598ff01e8714"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"ecd624ed4580467ff2538c41667a650ef324cf92","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright (c) 2017 OpenStack Foundation"},{"line_number":2,"context_line":"# All Rights Reserved."},{"line_number":3,"context_line":"#"},{"line_number":4,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"},{"line_number":5,"context_line":"#    not use this file except in compliance with the License. You may obtain"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_43dc9745","line":2,"range":{"start_line":1,"start_character":0,"end_line":2,"end_character":22},"updated":"2018-05-22 15:52:27.000000000","message":"This comment is to grab attention.\n\nI have asked a question on patch, in previous comment. Could you please answer that?\n\nCould you please review https://review.openstack.org/#/c/567626/ ?\n\n\nCould you please also review full sync and recovery patch?","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"2a9214702aa38c243dda4f6bd4f3e2e5a886cd01","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright (c) 2017 OpenStack Foundation"},{"line_number":2,"context_line":"# All Rights Reserved."},{"line_number":3,"context_line":"#"},{"line_number":4,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"},{"line_number":5,"context_line":"#    not use this file except in compliance with the License. You may obtain"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_9691869c","line":2,"range":{"start_line":1,"start_character":0,"end_line":2,"end_character":22},"in_reply_to":"5f7c97a3_43dc9745","updated":"2018-05-23 07:31:28.000000000","message":"Can you please put a comment on the question that I forgot to answer or reiterate the question?\n\nI must have not seeing and I\u0027m looking for it and can\u0027t find it easily...\n\nPS: Asking for reviews of other patches is not the place here... Put a comment on those patches and add me as reviewer, if you want my attention there...","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"a4d2c2099a446fd65445a691467c0e95329cccda","unresolved":false,"context_lines":[{"line_number":99,"context_line":"            \u0027feature-with-config\u0027: \u0027steal-your-face\u0027,"},{"line_number":100,"context_line":"        })"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027, return_value\u003dTrue)"},{"line_number":103,"context_line":"    def test_init_from_config_does_not_call__fetch_features(self, mock_fetch):"},{"line_number":104,"context_line":"        # make sure that without a config it calls _fetch_features"},{"line_number":105,"context_line":"        self.cfg.config(odl_features\u003dNone, group\u003d\u0027ml2_odl\u0027)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_5ddb5d4e","line":102,"range":{"start_line":102,"start_character":0,"end_line":102,"end_character":74},"updated":"2018-05-17 16:05:00.000000000","message":"Private method should not be mocked, it is bound to implementation.","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"ecd624ed4580467ff2538c41667a650ef324cf92","unresolved":false,"context_lines":[{"line_number":99,"context_line":"            \u0027feature-with-config\u0027: \u0027steal-your-face\u0027,"},{"line_number":100,"context_line":"        })"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027, return_value\u003dTrue)"},{"line_number":103,"context_line":"    def test_init_from_config_does_not_call__fetch_features(self, mock_fetch):"},{"line_number":104,"context_line":"        # make sure that without a config it calls _fetch_features"},{"line_number":105,"context_line":"        self.cfg.config(odl_features\u003dNone, group\u003d\u0027ml2_odl\u0027)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_a3624bac","line":102,"range":{"start_line":102,"start_character":0,"end_line":102,"end_character":74},"in_reply_to":"5f7c97a3_526eba96","updated":"2018-05-22 15:52:27.000000000","message":"Could you please explain, why it is not possible to not rely on implementation, in this cases? What made you do it this way?","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"3c4ea5c601bf93d0c69b19630800095cc14bed9a","unresolved":false,"context_lines":[{"line_number":99,"context_line":"            \u0027feature-with-config\u0027: \u0027steal-your-face\u0027,"},{"line_number":100,"context_line":"        })"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027, return_value\u003dTrue)"},{"line_number":103,"context_line":"    def test_init_from_config_does_not_call__fetch_features(self, mock_fetch):"},{"line_number":104,"context_line":"        # make sure that without a config it calls _fetch_features"},{"line_number":105,"context_line":"        self.cfg.config(odl_features\u003dNone, group\u003d\u0027ml2_odl\u0027)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_526eba96","line":102,"range":{"start_line":102,"start_character":0,"end_line":102,"end_character":74},"in_reply_to":"5f7c97a3_5ddb5d4e","updated":"2018-05-21 07:04:59.000000000","message":"A unit test is a whitebox test and it\u0027s okey to rely on implementation as long as you are testing behavior -which this one is doing. There are some cases where just it\u0027s not possible to not rely in at least a bit of implementation.\n\nAlso, a method that\u0027s private just means it\u0027s not meant to be consumed in a public API, but it\u0027s okey to use them in tests.","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"2a9214702aa38c243dda4f6bd4f3e2e5a886cd01","unresolved":false,"context_lines":[{"line_number":99,"context_line":"            \u0027feature-with-config\u0027: \u0027steal-your-face\u0027,"},{"line_number":100,"context_line":"        })"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027, return_value\u003dTrue)"},{"line_number":103,"context_line":"    def test_init_from_config_does_not_call__fetch_features(self, mock_fetch):"},{"line_number":104,"context_line":"        # make sure that without a config it calls _fetch_features"},{"line_number":105,"context_line":"        self.cfg.config(odl_features\u003dNone, group\u003d\u0027ml2_odl\u0027)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_f6c0fa7e","line":102,"range":{"start_line":102,"start_character":0,"end_line":102,"end_character":74},"in_reply_to":"5f7c97a3_a3624bac","updated":"2018-05-23 07:31:28.000000000","message":"The way to test behavior without relying on implementation is to actually make a request to a mocked ODL service. It\u0027s doable but setting up a mocked ODL service is beyond the complexity of the test as it should be.\n\nSo here, instead, I simplify and pre-suppose that _fetch_features is and will be the mechanism to fetch features. Therefore, without dwelling into the details of how to fetch the features, I make sure that it\u0027s not called.","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"a4d2c2099a446fd65445a691467c0e95329cccda","unresolved":false,"context_lines":[{"line_number":106,"context_line":"        odl_features.init()"},{"line_number":107,"context_line":"        mock_fetch.assert_called_once()"},{"line_number":108,"context_line":"        mock_fetch.reset_mock()"},{"line_number":109,"context_line":"        # now to the actual test"},{"line_number":110,"context_line":"        self.cfg.config(odl_features\u003dself.feature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":111,"context_line":"        odl_features.init()"},{"line_number":112,"context_line":"        mock_fetch.assert_not_called()"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_7de099fb","line":111,"range":{"start_line":109,"start_character":0,"end_line":111,"end_character":27},"updated":"2018-05-17 16:05:00.000000000","message":"What use case does it cover? Are you doing it for SIGHUP thing?","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"d51dc38583e9f0509374c28a2d138fc251430627","unresolved":false,"context_lines":[{"line_number":106,"context_line":"        odl_features.init()"},{"line_number":107,"context_line":"        mock_fetch.assert_called_once()"},{"line_number":108,"context_line":"        mock_fetch.reset_mock()"},{"line_number":109,"context_line":"        # now to the actual test"},{"line_number":110,"context_line":"        self.cfg.config(odl_features\u003dself.feature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":111,"context_line":"        odl_features.init()"},{"line_number":112,"context_line":"        mock_fetch.assert_not_called()"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_f9b78685","line":111,"range":{"start_line":109,"start_character":0,"end_line":111,"end_character":27},"in_reply_to":"5f7c97a3_3612f20a","updated":"2018-05-28 07:42:43.000000000","message":"I separated them into two, as agreed.","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"2a9214702aa38c243dda4f6bd4f3e2e5a886cd01","unresolved":false,"context_lines":[{"line_number":106,"context_line":"        odl_features.init()"},{"line_number":107,"context_line":"        mock_fetch.assert_called_once()"},{"line_number":108,"context_line":"        mock_fetch.reset_mock()"},{"line_number":109,"context_line":"        # now to the actual test"},{"line_number":110,"context_line":"        self.cfg.config(odl_features\u003dself.feature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":111,"context_line":"        odl_features.init()"},{"line_number":112,"context_line":"        mock_fetch.assert_not_called()"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_3612f20a","line":111,"range":{"start_line":109,"start_character":0,"end_line":111,"end_character":27},"in_reply_to":"5f7c97a3_430ed7e5","updated":"2018-05-23 07:31:28.000000000","message":"How so? L105-108 make sure that if there is no configuration then features are fetched from ODL. This is to ensure that we can actually test the behavior we want. L109-112 is the proper test where we set the config, call init and see that it\u0027s not fetching features from ODL.","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"3c4ea5c601bf93d0c69b19630800095cc14bed9a","unresolved":false,"context_lines":[{"line_number":106,"context_line":"        odl_features.init()"},{"line_number":107,"context_line":"        mock_fetch.assert_called_once()"},{"line_number":108,"context_line":"        mock_fetch.reset_mock()"},{"line_number":109,"context_line":"        # now to the actual test"},{"line_number":110,"context_line":"        self.cfg.config(odl_features\u003dself.feature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":111,"context_line":"        odl_features.init()"},{"line_number":112,"context_line":"        mock_fetch.assert_not_called()"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_d2396aa3","line":111,"range":{"start_line":109,"start_character":0,"end_line":111,"end_character":27},"in_reply_to":"5f7c97a3_7de099fb","updated":"2018-05-21 07:04:59.000000000","message":"That the configuration can be taken from a variable and that when this happen, the features are not fetched.","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"ecd624ed4580467ff2538c41667a650ef324cf92","unresolved":false,"context_lines":[{"line_number":106,"context_line":"        odl_features.init()"},{"line_number":107,"context_line":"        mock_fetch.assert_called_once()"},{"line_number":108,"context_line":"        mock_fetch.reset_mock()"},{"line_number":109,"context_line":"        # now to the actual test"},{"line_number":110,"context_line":"        self.cfg.config(odl_features\u003dself.feature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":111,"context_line":"        odl_features.init()"},{"line_number":112,"context_line":"        mock_fetch.assert_not_called()"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_430ed7e5","line":111,"range":{"start_line":109,"start_character":0,"end_line":111,"end_character":27},"in_reply_to":"5f7c97a3_d2396aa3","updated":"2018-05-22 15:52:27.000000000","message":"Your explanation does not seems to match with test case. When is this scenario possible? If a process is started, no matter, how many times you call init, every time the same value of config parameter will be presented. Why configuration parameter is set in line 110 but not in 105?","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"a4d2c2099a446fd65445a691467c0e95329cccda","unresolved":false,"context_lines":[{"line_number":126,"context_line":"        self.assertNotEqual(odl_features.DEFAULT_FEATURES,"},{"line_number":127,"context_line":"                            odl_features.feature_configs)"},{"line_number":128,"context_line":""},{"line_number":129,"context_line":"    def test_deinit_does_not_mutate_default_features(self):"},{"line_number":130,"context_line":"        default_features_old_id \u003d id(odl_features.DEFAULT_FEATURES)"},{"line_number":131,"context_line":"        feature_configs_old_id \u003d id(odl_features.feature_configs)"},{"line_number":132,"context_line":"        odl_features.deinit()"},{"line_number":133,"context_line":"        default_features_new_id \u003d id(odl_features.DEFAULT_FEATURES)"},{"line_number":134,"context_line":"        feature_configs_new_id \u003d id(odl_features.feature_configs)"},{"line_number":135,"context_line":"        self.assertEqual(default_features_old_id, default_features_new_id)"},{"line_number":136,"context_line":"        self.assertNotEqual(feature_configs_old_id, feature_configs_new_id)"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"    def test_deinit_resets_to_default_features(self):"},{"line_number":139,"context_line":"        odl_features.deinit()"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_d833fbf5","line":136,"range":{"start_line":129,"start_character":0,"end_line":136,"end_character":75},"updated":"2018-05-17 16:05:00.000000000","message":"Seems like too much bound to implementation. Behavior is not being checked rather reference is being checked. Test should not care about id.\n\nWhat\u0027s your view?","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"ecd624ed4580467ff2538c41667a650ef324cf92","unresolved":false,"context_lines":[{"line_number":126,"context_line":"        self.assertNotEqual(odl_features.DEFAULT_FEATURES,"},{"line_number":127,"context_line":"                            odl_features.feature_configs)"},{"line_number":128,"context_line":""},{"line_number":129,"context_line":"    def test_deinit_does_not_mutate_default_features(self):"},{"line_number":130,"context_line":"        default_features_old_id \u003d id(odl_features.DEFAULT_FEATURES)"},{"line_number":131,"context_line":"        feature_configs_old_id \u003d id(odl_features.feature_configs)"},{"line_number":132,"context_line":"        odl_features.deinit()"},{"line_number":133,"context_line":"        default_features_new_id \u003d id(odl_features.DEFAULT_FEATURES)"},{"line_number":134,"context_line":"        feature_configs_new_id \u003d id(odl_features.feature_configs)"},{"line_number":135,"context_line":"        self.assertEqual(default_features_old_id, default_features_new_id)"},{"line_number":136,"context_line":"        self.assertNotEqual(feature_configs_old_id, feature_configs_new_id)"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"    def test_deinit_resets_to_default_features(self):"},{"line_number":139,"context_line":"        odl_features.deinit()"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_832dcf47","line":136,"range":{"start_line":129,"start_character":0,"end_line":136,"end_character":75},"in_reply_to":"5f7c97a3_12ba8209","updated":"2018-05-22 15:52:27.000000000","message":"You could test based on values like values does not change if one variable is changed other should not be changed. It does not make sense to compare addresses.","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"d51dc38583e9f0509374c28a2d138fc251430627","unresolved":false,"context_lines":[{"line_number":126,"context_line":"        self.assertNotEqual(odl_features.DEFAULT_FEATURES,"},{"line_number":127,"context_line":"                            odl_features.feature_configs)"},{"line_number":128,"context_line":""},{"line_number":129,"context_line":"    def test_deinit_does_not_mutate_default_features(self):"},{"line_number":130,"context_line":"        default_features_old_id \u003d id(odl_features.DEFAULT_FEATURES)"},{"line_number":131,"context_line":"        feature_configs_old_id \u003d id(odl_features.feature_configs)"},{"line_number":132,"context_line":"        odl_features.deinit()"},{"line_number":133,"context_line":"        default_features_new_id \u003d id(odl_features.DEFAULT_FEATURES)"},{"line_number":134,"context_line":"        feature_configs_new_id \u003d id(odl_features.feature_configs)"},{"line_number":135,"context_line":"        self.assertEqual(default_features_old_id, default_features_new_id)"},{"line_number":136,"context_line":"        self.assertNotEqual(feature_configs_old_id, feature_configs_new_id)"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"    def test_deinit_resets_to_default_features(self):"},{"line_number":139,"context_line":"        odl_features.deinit()"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_1917e253","line":136,"range":{"start_line":129,"start_character":0,"end_line":136,"end_character":75},"in_reply_to":"5f7c97a3_562e8e43","updated":"2018-05-28 07:42:43.000000000","message":"Modified the test to reflect what we agreed.","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"2a9214702aa38c243dda4f6bd4f3e2e5a886cd01","unresolved":false,"context_lines":[{"line_number":126,"context_line":"        self.assertNotEqual(odl_features.DEFAULT_FEATURES,"},{"line_number":127,"context_line":"                            odl_features.feature_configs)"},{"line_number":128,"context_line":""},{"line_number":129,"context_line":"    def test_deinit_does_not_mutate_default_features(self):"},{"line_number":130,"context_line":"        default_features_old_id \u003d id(odl_features.DEFAULT_FEATURES)"},{"line_number":131,"context_line":"        feature_configs_old_id \u003d id(odl_features.feature_configs)"},{"line_number":132,"context_line":"        odl_features.deinit()"},{"line_number":133,"context_line":"        default_features_new_id \u003d id(odl_features.DEFAULT_FEATURES)"},{"line_number":134,"context_line":"        feature_configs_new_id \u003d id(odl_features.feature_configs)"},{"line_number":135,"context_line":"        self.assertEqual(default_features_old_id, default_features_new_id)"},{"line_number":136,"context_line":"        self.assertNotEqual(feature_configs_old_id, feature_configs_new_id)"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"    def test_deinit_resets_to_default_features(self):"},{"line_number":139,"context_line":"        odl_features.deinit()"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_562e8e43","line":136,"range":{"start_line":129,"start_character":0,"end_line":136,"end_character":75},"in_reply_to":"5f7c97a3_832dcf47","updated":"2018-05-23 07:31:28.000000000","message":"After a deinit both dicts would be empty dicts and still it could be the same dict that was mutated. So no.","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"3c4ea5c601bf93d0c69b19630800095cc14bed9a","unresolved":false,"context_lines":[{"line_number":126,"context_line":"        self.assertNotEqual(odl_features.DEFAULT_FEATURES,"},{"line_number":127,"context_line":"                            odl_features.feature_configs)"},{"line_number":128,"context_line":""},{"line_number":129,"context_line":"    def test_deinit_does_not_mutate_default_features(self):"},{"line_number":130,"context_line":"        default_features_old_id \u003d id(odl_features.DEFAULT_FEATURES)"},{"line_number":131,"context_line":"        feature_configs_old_id \u003d id(odl_features.feature_configs)"},{"line_number":132,"context_line":"        odl_features.deinit()"},{"line_number":133,"context_line":"        default_features_new_id \u003d id(odl_features.DEFAULT_FEATURES)"},{"line_number":134,"context_line":"        feature_configs_new_id \u003d id(odl_features.feature_configs)"},{"line_number":135,"context_line":"        self.assertEqual(default_features_old_id, default_features_new_id)"},{"line_number":136,"context_line":"        self.assertNotEqual(feature_configs_old_id, feature_configs_new_id)"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"    def test_deinit_resets_to_default_features(self):"},{"line_number":139,"context_line":"        odl_features.deinit()"}],"source_content_type":"text/x-python","patch_set":8,"id":"5f7c97a3_12ba8209","line":136,"range":{"start_line":129,"start_character":0,"end_line":136,"end_character":75},"in_reply_to":"5f7c97a3_d833fbf5","updated":"2018-05-21 07:04:59.000000000","message":"It\u0027s to avoid the bug that you raised. Usually bugs found should also be present on tests, so they don\u0027t repeat again.","commit_id":"0590c8d50a4198c7fffec0d328511aa96cbbff9d"},{"author":{"_account_id":7921,"name":"Mike Kolesnik","email":"mkolesni@redhat.com","username":"mkolesni"},"change_message_id":"4f7c26d66cbd3bed91d46028ae5f4ab9e76aafdf","unresolved":false,"context_lines":[{"line_number":99,"context_line":"            \u0027feature-with-config\u0027: \u0027steal-your-face\u0027,"},{"line_number":100,"context_line":"        })"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027, return_value\u003dTrue)"},{"line_number":103,"context_line":"    def test_init_without_config_does_call__fetch_features(self, mock_fetch):"},{"line_number":104,"context_line":"        self.cfg.config(odl_features\u003dNone, group\u003d\u0027ml2_odl\u0027)"},{"line_number":105,"context_line":"        odl_features.init()"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_a17d231d","line":102,"range":{"start_line":102,"start_character":68,"end_line":102,"end_character":73},"updated":"2018-06-11 09:14:52.000000000","message":"I agree with rajiv, if you want some value put some value otherwise if it doesn\u0027t matter put a mock or something.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"52ed5162a11b674c7d17fef90724fb35f05a69cc","unresolved":false,"context_lines":[{"line_number":99,"context_line":"            \u0027feature-with-config\u0027: \u0027steal-your-face\u0027,"},{"line_number":100,"context_line":"        })"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027, return_value\u003dTrue)"},{"line_number":103,"context_line":"    def test_init_without_config_does_call__fetch_features(self, mock_fetch):"},{"line_number":104,"context_line":"        self.cfg.config(odl_features\u003dNone, group\u003d\u0027ml2_odl\u0027)"},{"line_number":105,"context_line":"        odl_features.init()"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_18d7a5c1","line":102,"range":{"start_line":102,"start_character":68,"end_line":102,"end_character":73},"in_reply_to":"5f7c97a3_a17d231d","updated":"2018-06-11 15:48:29.000000000","message":"Done","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":7921,"name":"Mike Kolesnik","email":"mkolesni@redhat.com","username":"mkolesni"},"change_message_id":"4f7c26d66cbd3bed91d46028ae5f4ab9e76aafdf","unresolved":false,"context_lines":[{"line_number":100,"context_line":"        })"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027, return_value\u003dTrue)"},{"line_number":103,"context_line":"    def test_init_without_config_does_call__fetch_features(self, mock_fetch):"},{"line_number":104,"context_line":"        self.cfg.config(odl_features\u003dNone, group\u003d\u0027ml2_odl\u0027)"},{"line_number":105,"context_line":"        odl_features.init()"},{"line_number":106,"context_line":"        mock_fetch.assert_called_once()"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_619aab6f","line":103,"range":{"start_line":103,"start_character":33,"end_line":103,"end_character":42},"updated":"2018-06-11 09:14:52.000000000","message":"nit: calls?","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"52ed5162a11b674c7d17fef90724fb35f05a69cc","unresolved":false,"context_lines":[{"line_number":100,"context_line":"        })"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027, return_value\u003dTrue)"},{"line_number":103,"context_line":"    def test_init_without_config_does_call__fetch_features(self, mock_fetch):"},{"line_number":104,"context_line":"        self.cfg.config(odl_features\u003dNone, group\u003d\u0027ml2_odl\u0027)"},{"line_number":105,"context_line":"        odl_features.init()"},{"line_number":106,"context_line":"        mock_fetch.assert_called_once()"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_b8df91d5","line":103,"range":{"start_line":103,"start_character":33,"end_line":103,"end_character":42},"in_reply_to":"5f7c97a3_619aab6f","updated":"2018-06-11 15:48:29.000000000","message":"Done","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"2b4fbfb4ec7cfb0c2aabda0db45f4b9e466a721d","unresolved":false,"context_lines":[{"line_number":105,"context_line":"        odl_features.init()"},{"line_number":106,"context_line":"        mock_fetch.assert_called_once()"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027, return_value\u003dTrue)"},{"line_number":109,"context_line":"    def test_init_with_config_does_not_call__fetch_features(self, mock_fetch):"},{"line_number":110,"context_line":"        self.cfg.config(odl_features\u003dself.feature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":111,"context_line":"        odl_features.init()"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_93d13c26","line":108,"range":{"start_line":108,"start_character":55,"end_line":108,"end_character":74},"updated":"2018-05-30 14:28:06.000000000","message":"Why it is returning True? Does actual implementation returns True ever?","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"f26312096fca72255ef1bcfa50e1ce3755fbe254","unresolved":false,"context_lines":[{"line_number":105,"context_line":"        odl_features.init()"},{"line_number":106,"context_line":"        mock_fetch.assert_called_once()"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027, return_value\u003dTrue)"},{"line_number":109,"context_line":"    def test_init_with_config_does_not_call__fetch_features(self, mock_fetch):"},{"line_number":110,"context_line":"        self.cfg.config(odl_features\u003dself.feature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":111,"context_line":"        odl_features.init()"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_9d7a82ee","line":108,"range":{"start_line":108,"start_character":55,"end_line":108,"end_character":74},"in_reply_to":"5f7c97a3_93d13c26","updated":"2018-06-01 09:52:31.000000000","message":"It doesn\u0027t matter what happens in the implementation, I just need it to break the execution if it\u0027s called.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"bf64a1dab616997082a787655d65ce38bc2ece39","unresolved":false,"context_lines":[{"line_number":105,"context_line":"        odl_features.init()"},{"line_number":106,"context_line":"        mock_fetch.assert_called_once()"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027, return_value\u003dTrue)"},{"line_number":109,"context_line":"    def test_init_with_config_does_not_call__fetch_features(self, mock_fetch):"},{"line_number":110,"context_line":"        self.cfg.config(odl_features\u003dself.feature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":111,"context_line":"        odl_features.init()"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_b07a0585","line":108,"range":{"start_line":108,"start_character":55,"end_line":108,"end_character":74},"in_reply_to":"5f7c97a3_9d7a82ee","updated":"2018-06-01 10:17:24.000000000","message":"It does not digest well, while reading code, that _fetch_feature will return true. It gives wrong impression.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":7921,"name":"Mike Kolesnik","email":"mkolesni@redhat.com","username":"mkolesni"},"change_message_id":"4f7c26d66cbd3bed91d46028ae5f4ab9e76aafdf","unresolved":false,"context_lines":[{"line_number":105,"context_line":"        odl_features.init()"},{"line_number":106,"context_line":"        mock_fetch.assert_called_once()"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027, return_value\u003dTrue)"},{"line_number":109,"context_line":"    def test_init_with_config_does_not_call__fetch_features(self, mock_fetch):"},{"line_number":110,"context_line":"        self.cfg.config(odl_features\u003dself.feature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":111,"context_line":"        odl_features.init()"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_e1a6bbc0","line":108,"range":{"start_line":108,"start_character":55,"end_line":108,"end_character":74},"in_reply_to":"5f7c97a3_b07a0585","updated":"2018-06-11 09:14:52.000000000","message":"In this case you don\u0027t expect it to be called, so you shouldn\u0027t set up a return value at all..","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"52ed5162a11b674c7d17fef90724fb35f05a69cc","unresolved":false,"context_lines":[{"line_number":105,"context_line":"        odl_features.init()"},{"line_number":106,"context_line":"        mock_fetch.assert_called_once()"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027, return_value\u003dTrue)"},{"line_number":109,"context_line":"    def test_init_with_config_does_not_call__fetch_features(self, mock_fetch):"},{"line_number":110,"context_line":"        self.cfg.config(odl_features\u003dself.feature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":111,"context_line":"        odl_features.init()"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_789499e8","line":108,"range":{"start_line":108,"start_character":55,"end_line":108,"end_character":74},"in_reply_to":"5f7c97a3_e1a6bbc0","updated":"2018-06-11 15:48:29.000000000","message":"Done","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"2b4fbfb4ec7cfb0c2aabda0db45f4b9e466a721d","unresolved":false,"context_lines":[{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027, return_value\u003dTrue)"},{"line_number":109,"context_line":"    def test_init_with_config_does_not_call__fetch_features(self, mock_fetch):"},{"line_number":110,"context_line":"        self.cfg.config(odl_features\u003dself.feature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":111,"context_line":"        odl_features.init()"},{"line_number":112,"context_line":"        mock_fetch.assert_not_called()"},{"line_number":113,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_53c72465","line":110,"range":{"start_line":110,"start_character":0,"end_line":110,"end_character":72},"updated":"2018-05-30 14:28:06.000000000","message":"Does this need cleanup?","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"f26312096fca72255ef1bcfa50e1ce3755fbe254","unresolved":false,"context_lines":[{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027, return_value\u003dTrue)"},{"line_number":109,"context_line":"    def test_init_with_config_does_not_call__fetch_features(self, mock_fetch):"},{"line_number":110,"context_line":"        self.cfg.config(odl_features\u003dself.feature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":111,"context_line":"        odl_features.init()"},{"line_number":112,"context_line":"        mock_fetch.assert_not_called()"},{"line_number":113,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_bd7dfee7","line":110,"range":{"start_line":110,"start_character":0,"end_line":110,"end_character":72},"in_reply_to":"5f7c97a3_53c72465","updated":"2018-06-01 09:52:31.000000000","message":"no, self.cfg is a fixture.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"2b4fbfb4ec7cfb0c2aabda0db45f4b9e466a721d","unresolved":false,"context_lines":[{"line_number":130,"context_line":"        # we call it before initing anything, to force the reassignment"},{"line_number":131,"context_line":"        # of the global features variable."},{"line_number":132,"context_line":"        odl_features.deinit()"},{"line_number":133,"context_line":"        odl_features.feature_configs[\u0027test\u0027] \u003d True"},{"line_number":134,"context_line":"        self.assertNotEqual(odl_features.DEFAULT_FEATURES,"},{"line_number":135,"context_line":"                            odl_features.feature_configs)"},{"line_number":136,"context_line":"        # now we do it again, to make sure that it assigns it to default"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_7397c046","line":133,"range":{"start_line":133,"start_character":0,"end_line":133,"end_character":51},"updated":"2018-05-30 14:28:06.000000000","message":"Do you think, setting it through configuration will be natural way of mutating this variable?","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"f26312096fca72255ef1bcfa50e1ce3755fbe254","unresolved":false,"context_lines":[{"line_number":130,"context_line":"        # we call it before initing anything, to force the reassignment"},{"line_number":131,"context_line":"        # of the global features variable."},{"line_number":132,"context_line":"        odl_features.deinit()"},{"line_number":133,"context_line":"        odl_features.feature_configs[\u0027test\u0027] \u003d True"},{"line_number":134,"context_line":"        self.assertNotEqual(odl_features.DEFAULT_FEATURES,"},{"line_number":135,"context_line":"                            odl_features.feature_configs)"},{"line_number":136,"context_line":"        # now we do it again, to make sure that it assigns it to default"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_7d83e6cb","line":133,"range":{"start_line":133,"start_character":0,"end_line":133,"end_character":51},"in_reply_to":"5f7c97a3_7397c046","updated":"2018-06-01 09:52:31.000000000","message":"for test purposes I think it\u0027s okey.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":13912,"name":"rajiv","email":"thenakliman@gmail.com","username":"rajiv.kumar"},"change_message_id":"bf64a1dab616997082a787655d65ce38bc2ece39","unresolved":false,"context_lines":[{"line_number":130,"context_line":"        # we call it before initing anything, to force the reassignment"},{"line_number":131,"context_line":"        # of the global features variable."},{"line_number":132,"context_line":"        odl_features.deinit()"},{"line_number":133,"context_line":"        odl_features.feature_configs[\u0027test\u0027] \u003d True"},{"line_number":134,"context_line":"        self.assertNotEqual(odl_features.DEFAULT_FEATURES,"},{"line_number":135,"context_line":"                            odl_features.feature_configs)"},{"line_number":136,"context_line":"        # now we do it again, to make sure that it assigns it to default"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f7c97a3_1d0f5237","line":133,"range":{"start_line":133,"start_character":0,"end_line":133,"end_character":51},"in_reply_to":"5f7c97a3_7d83e6cb","updated":"2018-06-01 10:17:24.000000000","message":"I would prefer other way around, i can live with this one as well.","commit_id":"06e07c1f8ff93ad8c4646b9bd253a17cfc314db5"},{"author":{"_account_id":7921,"name":"Mike Kolesnik","email":"mkolesni@redhat.com","username":"mkolesni"},"change_message_id":"1a8c056bf8f4af1022b8f1d35dcca4536d99126d","unresolved":false,"context_lines":[{"line_number":69,"context_line":"    @mock.patch.object(OpenDaylightRestClient, \u0027request\u0027)"},{"line_number":70,"context_line":"    def test_init(self, mocked_client):"},{"line_number":71,"context_line":"        self.cfg.config(odl_features\u003dNone, group\u003d\u0027ml2_odl\u0027)"},{"line_number":72,"context_line":"        self.cfg.config(odl_features_json\u003dNone, group\u003d\u0027ml2_odl\u0027)"},{"line_number":73,"context_line":"        response \u003d mock.MagicMock()"},{"line_number":74,"context_line":"        response.status_code \u003d 200"},{"line_number":75,"context_line":"        response.json \u003d mock.MagicMock("}],"source_content_type":"text/x-python","patch_set":11,"id":"5f7c97a3_e8cf2454","line":72,"range":{"start_line":72,"start_character":0,"end_line":72,"end_character":64},"updated":"2018-06-27 10:20:17.000000000","message":"If I comment out this line the test still passes, is it necessary or is the test missing some assertion?","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"7cde2803a722562a7dd825a11955584377a7ba1a","unresolved":false,"context_lines":[{"line_number":69,"context_line":"    @mock.patch.object(OpenDaylightRestClient, \u0027request\u0027)"},{"line_number":70,"context_line":"    def test_init(self, mocked_client):"},{"line_number":71,"context_line":"        self.cfg.config(odl_features\u003dNone, group\u003d\u0027ml2_odl\u0027)"},{"line_number":72,"context_line":"        self.cfg.config(odl_features_json\u003dNone, group\u003d\u0027ml2_odl\u0027)"},{"line_number":73,"context_line":"        response \u003d mock.MagicMock()"},{"line_number":74,"context_line":"        response.status_code \u003d 200"},{"line_number":75,"context_line":"        response.json \u003d mock.MagicMock("}],"source_content_type":"text/x-python","patch_set":11,"id":"5f7c97a3_fe703edc","line":72,"range":{"start_line":72,"start_character":0,"end_line":72,"end_character":64},"in_reply_to":"5f7c97a3_e8cf2454","updated":"2018-06-27 11:44:44.000000000","message":"It\u0027s required to clear the configuration which is global and static so lingers between tests.","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"},{"author":{"_account_id":7921,"name":"Mike Kolesnik","email":"mkolesni@redhat.com","username":"mkolesni"},"change_message_id":"1a8c056bf8f4af1022b8f1d35dcca4536d99126d","unresolved":false,"context_lines":[{"line_number":97,"context_line":""},{"line_number":98,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027)"},{"line_number":99,"context_line":"    def test_init_without_config_calls__fetch_features(self, mock_fetch):"},{"line_number":100,"context_line":"        self.cfg.config(odl_features_json\u003dNone, group\u003d\u0027ml2_odl\u0027)"},{"line_number":101,"context_line":"        self.cfg.config(odl_features\u003dNone, group\u003d\u0027ml2_odl\u0027)"},{"line_number":102,"context_line":"        odl_features.init()"},{"line_number":103,"context_line":"        mock_fetch.assert_called_once()"}],"source_content_type":"text/x-python","patch_set":11,"id":"5f7c97a3_a8436caf","line":100,"range":{"start_line":100,"start_character":0,"end_line":100,"end_character":64},"updated":"2018-06-27 10:20:17.000000000","message":"If I comment out this line the test still passes, is it necessary or is the test missing some assertion?","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"7cde2803a722562a7dd825a11955584377a7ba1a","unresolved":false,"context_lines":[{"line_number":97,"context_line":""},{"line_number":98,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027)"},{"line_number":99,"context_line":"    def test_init_without_config_calls__fetch_features(self, mock_fetch):"},{"line_number":100,"context_line":"        self.cfg.config(odl_features_json\u003dNone, group\u003d\u0027ml2_odl\u0027)"},{"line_number":101,"context_line":"        self.cfg.config(odl_features\u003dNone, group\u003d\u0027ml2_odl\u0027)"},{"line_number":102,"context_line":"        odl_features.init()"},{"line_number":103,"context_line":"        mock_fetch.assert_called_once()"}],"source_content_type":"text/x-python","patch_set":11,"id":"5f7c97a3_9e7b4abd","line":100,"range":{"start_line":100,"start_character":0,"end_line":100,"end_character":64},"in_reply_to":"5f7c97a3_a8436caf","updated":"2018-06-27 11:44:44.000000000","message":"It\u0027s required to clear the configuration which is global and static so lingers between tests.","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"},{"author":{"_account_id":7921,"name":"Mike Kolesnik","email":"mkolesni@redhat.com","username":"mkolesni"},"change_message_id":"1a8c056bf8f4af1022b8f1d35dcca4536d99126d","unresolved":false,"context_lines":[{"line_number":104,"context_line":""},{"line_number":105,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027)"},{"line_number":106,"context_line":"    def test_init_from_config_list(self, mock_fetch):"},{"line_number":107,"context_line":"        self.cfg.config(odl_features\u003d\u0027thing1,thing2\u0027, group\u003d\u0027ml2_odl\u0027)"},{"line_number":108,"context_line":"        odl_features.init()"},{"line_number":109,"context_line":"        self.assertTrue(odl_features.has(\u0027thing1\u0027))"},{"line_number":110,"context_line":"        self.assertTrue(odl_features.has(\u0027thing2\u0027))"}],"source_content_type":"text/x-python","patch_set":11,"id":"5f7c97a3_2b419663","line":107,"range":{"start_line":107,"start_character":38,"end_line":107,"end_character":52},"updated":"2018-06-27 10:20:17.000000000","message":"self.feature_list?","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"7cde2803a722562a7dd825a11955584377a7ba1a","unresolved":false,"context_lines":[{"line_number":104,"context_line":""},{"line_number":105,"context_line":"    @mock.patch.object(odl_features, \u0027_fetch_features\u0027)"},{"line_number":106,"context_line":"    def test_init_from_config_list(self, mock_fetch):"},{"line_number":107,"context_line":"        self.cfg.config(odl_features\u003d\u0027thing1,thing2\u0027, group\u003d\u0027ml2_odl\u0027)"},{"line_number":108,"context_line":"        odl_features.init()"},{"line_number":109,"context_line":"        self.assertTrue(odl_features.has(\u0027thing1\u0027))"},{"line_number":110,"context_line":"        self.assertTrue(odl_features.has(\u0027thing2\u0027))"}],"source_content_type":"text/x-python","patch_set":11,"id":"5f7c97a3_3e447672","line":107,"range":{"start_line":107,"start_character":38,"end_line":107,"end_character":52},"in_reply_to":"5f7c97a3_2b419663","updated":"2018-06-27 11:44:44.000000000","message":"Done","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"}],"networking_odl/tests/unit/ml2/test_pseudo_agentdb_binding.py":[{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"45d8a65993a95502d1c83d2a102cacc51cc0d5be","unresolved":false,"context_lines":[{"line_number":642,"context_line":"    @mock.patch.object(provisioning_blocks, \u0027add_provisioning_component\u0027)"},{"line_number":643,"context_line":"    def test_prepare_inital_port_status_with_websocket("},{"line_number":644,"context_line":"            self, mocked_add_provisioning_component):"},{"line_number":645,"context_line":"        odl_features.feature_configs[odl_features.OPERATIONAL_PORT_STATUS] \u003d \u0027\u0027"},{"line_number":646,"context_line":"        port_ctx \u003d self._fake_port_context("},{"line_number":647,"context_line":"            fake_segments\u003d[self.test_valid_segment])"},{"line_number":648,"context_line":"        initial_port_status \u003d self.mgr._prepare_initial_port_status(port_ctx)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f6a8fd7_0e9d8858","line":645,"range":{"start_line":645,"start_character":8,"end_line":645,"end_character":79},"updated":"2018-04-30 08:08:23.000000000","message":"I\u0027d rather this be separate from implementation if possible (i.e.: loading the configs and let it init handle it).","commit_id":"71cf6adb5af377c4aa4c335cc9d949c30816b6e0"},{"author":{"_account_id":7921,"name":"Mike Kolesnik","email":"mkolesni@redhat.com","username":"mkolesni"},"change_message_id":"1a8c056bf8f4af1022b8f1d35dcca4536d99126d","unresolved":false,"context_lines":[{"line_number":642,"context_line":"    @mock.patch.object(provisioning_blocks, \u0027add_provisioning_component\u0027)"},{"line_number":643,"context_line":"    def test_prepare_inital_port_status_with_websocket("},{"line_number":644,"context_line":"            self, mocked_add_provisioning_component):"},{"line_number":645,"context_line":"        feature_json \u003d \"\"\"{\"features\": {\"feature\":"},{"line_number":646,"context_line":"                            [{\"service-provider-feature\":"},{"line_number":647,"context_line":"                            \"neutron-extensions:operational-port-status\"}]}}\"\"\""},{"line_number":648,"context_line":"        self.cfg.config(odl_features\u003dfeature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":649,"context_line":"        self.addCleanup(odl_features.deinit)"},{"line_number":650,"context_line":"        odl_features.init()"},{"line_number":651,"context_line":"        port_ctx \u003d self._fake_port_context("}],"source_content_type":"text/x-python","patch_set":11,"id":"5f7c97a3_fe359edb","line":648,"range":{"start_line":645,"start_character":0,"end_line":648,"end_character":67},"updated":"2018-06-27 10:20:17.000000000","message":"nit: this change isn\u0027t necessary right now, not sure it makes sense to do it since it complicates the test a bit","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"7cde2803a722562a7dd825a11955584377a7ba1a","unresolved":false,"context_lines":[{"line_number":642,"context_line":"    @mock.patch.object(provisioning_blocks, \u0027add_provisioning_component\u0027)"},{"line_number":643,"context_line":"    def test_prepare_inital_port_status_with_websocket("},{"line_number":644,"context_line":"            self, mocked_add_provisioning_component):"},{"line_number":645,"context_line":"        feature_json \u003d \"\"\"{\"features\": {\"feature\":"},{"line_number":646,"context_line":"                            [{\"service-provider-feature\":"},{"line_number":647,"context_line":"                            \"neutron-extensions:operational-port-status\"}]}}\"\"\""},{"line_number":648,"context_line":"        self.cfg.config(odl_features\u003dfeature_json, group\u003d\u0027ml2_odl\u0027)"},{"line_number":649,"context_line":"        self.addCleanup(odl_features.deinit)"},{"line_number":650,"context_line":"        odl_features.init()"},{"line_number":651,"context_line":"        port_ctx \u003d self._fake_port_context("}],"source_content_type":"text/x-python","patch_set":11,"id":"5f7c97a3_8130cb9f","line":648,"range":{"start_line":645,"start_character":0,"end_line":648,"end_character":67},"in_reply_to":"5f7c97a3_fe359edb","updated":"2018-06-27 11:44:44.000000000","message":"I\u0027d like to leave it because I would like to deprecate odl_features if possible and just stay with odl_features_json. Either way, it works here.","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"}],"releasenotes/notes/odl_features-option-type-change-367385ae7d1e949e.yaml":[{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"b58dc74b2f5c68aac97b9b88d1f3f1f7f2605309","unresolved":false,"context_lines":[{"line_number":6,"context_line":"    feature retrieval– or a list."},{"line_number":7,"context_line":"features:"},{"line_number":8,"context_line":"  - |"},{"line_number":9,"context_line":"    Option odl_features now also accepts a JSON compatible with the JSON"},{"line_number":10,"context_line":"    response from the ODL\u0027s API when retrieving features –lists"},{"line_number":11,"context_line":"    are still supported to be backwards compatible. This allows to"},{"line_number":12,"context_line":"    give a feature as a configuration."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"    The allowed options are a list ( e.g. odl_features \u003d feature1,"},{"line_number":15,"context_line":"    feature2 ) or a JSON that mimics the ODL API for feature"}],"source_content_type":"text/x-yaml","patch_set":6,"id":"5f7c97a3_0aab67d6","line":12,"range":{"start_line":9,"start_character":0,"end_line":12,"end_character":38},"updated":"2018-05-03 08:23:21.000000000","message":"The odl_features option now accepts a JSON compatible with the JSON\n    response from ODL\u0027s API for retrieving features (\"/restconf/operational/neutron:neutron/features\"). The odl_feature option still supports a comma separated list of features for backwards compatibility. If this option is configured, networking_odl will not query ODL for its feature support and will instead use the configured value.","commit_id":"c677bce2ff38b3e6263d15642339598ff01e8714"},{"author":{"_account_id":26507,"name":"Michel Peterson","email":"michel@redhat.com","username":"mpeterson"},"change_message_id":"7a7814a30df97af0c23c82ffd5d634c4cc97bfd0","unresolved":false,"context_lines":[{"line_number":6,"context_line":"    feature retrieval– or a list."},{"line_number":7,"context_line":"features:"},{"line_number":8,"context_line":"  - |"},{"line_number":9,"context_line":"    Option odl_features now also accepts a JSON compatible with the JSON"},{"line_number":10,"context_line":"    response from the ODL\u0027s API when retrieving features –lists"},{"line_number":11,"context_line":"    are still supported to be backwards compatible. This allows to"},{"line_number":12,"context_line":"    give a feature as a configuration."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"    The allowed options are a list ( e.g. odl_features \u003d feature1,"},{"line_number":15,"context_line":"    feature2 ) or a JSON that mimics the ODL API for feature"}],"source_content_type":"text/x-yaml","patch_set":6,"id":"5f7c97a3_7068a4f4","line":12,"range":{"start_line":9,"start_character":0,"end_line":12,"end_character":38},"in_reply_to":"5f7c97a3_0aab67d6","updated":"2018-05-07 07:49:23.000000000","message":"Done","commit_id":"c677bce2ff38b3e6263d15642339598ff01e8714"},{"author":{"_account_id":7921,"name":"Mike Kolesnik","email":"mkolesni@redhat.com","username":"mkolesni"},"change_message_id":"1a8c056bf8f4af1022b8f1d35dcca4536d99126d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"5f7c97a3_7e4f4e69","updated":"2018-06-27 10:20:17.000000000","message":"This needs to be updated","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"},{"author":{"_account_id":22380,"name":"Josh Hershberg","email":"jhershbe@redhat.com","username":"jhershbe"},"change_message_id":"7cde2803a722562a7dd825a11955584377a7ba1a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"5f7c97a3_3c20fe99","in_reply_to":"5f7c97a3_7e4f4e69","updated":"2018-06-27 11:44:44.000000000","message":"Done","commit_id":"0370abeed15088c2a8c8ac0648fb708410600d82"}]}