)]}'
{"manifests/api.pp":[{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"3fdcf1b434197f17203792b4684dc31f01c187b9","unresolved":true,"context_lines":[{"line_number":106,"context_line":"#"},{"line_number":107,"context_line":"# DEPRECATED PARAMETERS"},{"line_number":108,"context_line":"#"},{"line_number":109,"context_line":"# [*keymgr_encryption_api_url*]"},{"line_number":110,"context_line":"#   (optional) Key Manager service URL"},{"line_number":111,"context_line":"#   Example of valid value: https://localhost:9311/v1"},{"line_number":112,"context_line":"#   Defaults to undef"}],"source_content_type":"text/x-puppet","patch_set":8,"id":"6dddca11_2659e53d","side":"PARENT","line":109,"updated":"2021-01-25 19:12:35.000000000","message":"Removal of these deprecated parameters makes sense, but I see you submitted another patch that does the same.\n\nhttps://review.opendev.org/c/openstack/puppet-cinder/+/772137","commit_id":"65c493a3cbc02b4c28e0425394a9bc8b69ae0a7a"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"ef36cd4f5456a194f2b68a3c664c906ba57e2045","unresolved":false,"context_lines":[{"line_number":106,"context_line":"#"},{"line_number":107,"context_line":"# DEPRECATED PARAMETERS"},{"line_number":108,"context_line":"#"},{"line_number":109,"context_line":"# [*keymgr_encryption_api_url*]"},{"line_number":110,"context_line":"#   (optional) Key Manager service URL"},{"line_number":111,"context_line":"#   Example of valid value: https://localhost:9311/v1"},{"line_number":112,"context_line":"#   Defaults to undef"}],"source_content_type":"text/x-puppet","patch_set":8,"id":"2c187b33_3aa5b391","side":"PARENT","line":109,"in_reply_to":"2e5d4c81_f37e17a0","updated":"2021-01-27 11:09:15.000000000","message":"I abandoned that one and merged the change into this because I was willing to reduce number of \"small cleanup\" patches.\nHowever now I feel it would be simpler to complete cleanup first and then think about next refactoring, so I\u0027ll restore the previous cleanup patch later.","commit_id":"65c493a3cbc02b4c28e0425394a9bc8b69ae0a7a"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"6983eaf9bb6f9ecfd0f575fbc28e6402cd2ff808","unresolved":false,"context_lines":[{"line_number":106,"context_line":"#"},{"line_number":107,"context_line":"# DEPRECATED PARAMETERS"},{"line_number":108,"context_line":"#"},{"line_number":109,"context_line":"# [*keymgr_encryption_api_url*]"},{"line_number":110,"context_line":"#   (optional) Key Manager service URL"},{"line_number":111,"context_line":"#   Example of valid value: https://localhost:9311/v1"},{"line_number":112,"context_line":"#   Defaults to undef"}],"source_content_type":"text/x-puppet","patch_set":8,"id":"2e5d4c81_f37e17a0","side":"PARENT","line":109,"in_reply_to":"6dddca11_2659e53d","updated":"2021-01-25 19:24:03.000000000","message":"Oh, I just noticed you abandoned the other patch.","commit_id":"65c493a3cbc02b4c28e0425394a9bc8b69ae0a7a"}],"manifests/init.pp":[{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"3fdcf1b434197f17203792b4684dc31f01c187b9","unresolved":true,"context_lines":[{"line_number":472,"context_line":"    \u0027DEFAULT/enable_v3_api\u0027: value \u003d\u003e $enable_v3_api;"},{"line_number":473,"context_line":"  }"},{"line_number":474,"context_line":""},{"line_number":475,"context_line":"  if $keymgr_backend !\u003d undef {"},{"line_number":476,"context_line":"    warning(\u0027The keymgr_backend parameter is deprecated. Use the cinder::key_manager class\u0027)"},{"line_number":477,"context_line":"  }"},{"line_number":478,"context_line":"  include cinder::key_manager"}],"source_content_type":"text/x-puppet","patch_set":8,"id":"53198b12_15bcdd57","line":475,"updated":"2021-01-25 19:12:35.000000000","message":"The code from L475..L486 highlights a general point I want to make. I realize castellan is a generic key manager interface and barbican is just one specific implementation. Creating a new cinder::key_manager class makes sense from an absraction standpoint, but the code changes introduce a lot of churn if, in reality, everyone plans to use barbican.\n\nIn other words, I think we can reduce code churn *today* by defering until a time when there\u0027s an actual need for key manager other than barbican.","commit_id":"c0d94605a0fc82f3f3b8c5b09f948bc8bd56b4f1"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"4213cf3e7193bc3aadbc3889093a7e3304860d96","unresolved":false,"context_lines":[{"line_number":472,"context_line":"    \u0027DEFAULT/enable_v3_api\u0027: value \u003d\u003e $enable_v3_api;"},{"line_number":473,"context_line":"  }"},{"line_number":474,"context_line":""},{"line_number":475,"context_line":"  if $keymgr_backend !\u003d undef {"},{"line_number":476,"context_line":"    warning(\u0027The keymgr_backend parameter is deprecated. Use the cinder::key_manager class\u0027)"},{"line_number":477,"context_line":"  }"},{"line_number":478,"context_line":"  include cinder::key_manager"}],"source_content_type":"text/x-puppet","patch_set":8,"id":"cb3ad47d_3f6849c3","line":475,"in_reply_to":"398f3276_6e53b971","updated":"2021-01-28 00:58:55.000000000","message":"You make good points, thank you.","commit_id":"c0d94605a0fc82f3f3b8c5b09f948bc8bd56b4f1"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"ef36cd4f5456a194f2b68a3c664c906ba57e2045","unresolved":true,"context_lines":[{"line_number":472,"context_line":"    \u0027DEFAULT/enable_v3_api\u0027: value \u003d\u003e $enable_v3_api;"},{"line_number":473,"context_line":"  }"},{"line_number":474,"context_line":""},{"line_number":475,"context_line":"  if $keymgr_backend !\u003d undef {"},{"line_number":476,"context_line":"    warning(\u0027The keymgr_backend parameter is deprecated. Use the cinder::key_manager class\u0027)"},{"line_number":477,"context_line":"  }"},{"line_number":478,"context_line":"  include cinder::key_manager"}],"source_content_type":"text/x-puppet","patch_set":8,"id":"398f3276_6e53b971","line":475,"in_reply_to":"53198b12_15bcdd57","updated":"2021-01-27 11:09:15.000000000","message":"Yeah. I found the same when I was creating this patch, but because of the following reasons I decided the current implementation which would required multiple new classes.\n\n1.\nIf we merge key_manager and barbican into the same class, it becomes difficult to determine the safe but reasonable names.\nFor example if we implement the single key_manager class then we should prefix all barbican parameters with barbican_* to avoid unexpected collisions in the future. But this requires us to have ugly barbican_barbican_* parameters or maintain complicated parameter mappings.\n\n2.\nI think from cinder\u0027s perspective vault is another option (though personally I\u0027ve never seen it used)\n\n3.\nAFAIK castellan is used in 3 projects, nova, cinder and glance currently, and it\u0027s better to have the consistent implementation among them. Since each service can implement some customization (for example nova has its own implementation of key manager), I feel it\u0027s better to depend on the structure of library instead of usage of each services, otherwise we would need to adjust implementations in all projects if one of these project make any change affecting the structure we\u0027ll put.","commit_id":"c0d94605a0fc82f3f3b8c5b09f948bc8bd56b4f1"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"4213cf3e7193bc3aadbc3889093a7e3304860d96","unresolved":true,"context_lines":[{"line_number":475,"context_line":"  if $keymgr_backend !\u003d undef {"},{"line_number":476,"context_line":"    warning(\u0027The keymgr_backend parameter is deprecated. Use the cinder::key_manager class\u0027)"},{"line_number":477,"context_line":"  }"},{"line_number":478,"context_line":"  include cinder::key_manager"},{"line_number":479,"context_line":""},{"line_number":480,"context_line":"  [\u0027keymgr_encryption_api_url\u0027, \u0027keymgr_encryption_auth_url\u0027].each |String $barbican_opt| {"},{"line_number":481,"context_line":"    if getvar(\"${barbican_opt}\") !\u003d undef {"}],"source_content_type":"text/x-puppet","patch_set":9,"id":"147d5076_5b90abd1","line":478,"updated":"2021-01-28 00:58:55.000000000","message":"The deprecation warnings at L475 and L480 look fine, but maybe move L478 so all the includes are grouped together.","commit_id":"e749318b7ac78834c1350008042a66047a5a9f45"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"4213cf3e7193bc3aadbc3889093a7e3304860d96","unresolved":true,"context_lines":[{"line_number":482,"context_line":"      warning(\"The ${barbican_opt} parameter is deprecated. Use the cinder::key_manager::barbican class\")"},{"line_number":483,"context_line":"    }"},{"line_number":484,"context_line":"  }"},{"line_number":485,"context_line":"  include cinder::key_manager::barbican"},{"line_number":486,"context_line":""},{"line_number":487,"context_line":"  oslo::concurrency { \u0027cinder_config\u0027:"},{"line_number":488,"context_line":"    lock_path \u003d\u003e $lock_path"}],"source_content_type":"text/x-puppet","patch_set":9,"id":"98cbefac_134f5d69","line":485,"updated":"2021-01-28 00:58:55.000000000","message":"I suggest moving this into key_manager class. I\u0027ll add another comment there.","commit_id":"e749318b7ac78834c1350008042a66047a5a9f45"}],"manifests/key_manager.pp":[{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"4213cf3e7193bc3aadbc3889093a7e3304860d96","unresolved":true,"context_lines":[{"line_number":13,"context_line":") {"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"  include cinder::deps"},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"  $backend_real \u003d pick($cinder::keymgr_backend, $backend)"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"  cinder_config {"}],"source_content_type":"text/x-puppet","patch_set":9,"id":"1d9aa3db_13e07a31","line":16,"updated":"2021-01-28 00:58:55.000000000","message":"For now, maybe this is the place to include cinder::key_manager::barbican.\n\nAnd if we want to add support for other backends, this code could parse the $backend, maybe something like this:\n\n  if $backend \u003d\u003d \u0027barbican\u0027 {\n    include cinder::key_manager::barbican\n  }","commit_id":"e749318b7ac78834c1350008042a66047a5a9f45"}],"manifests/key_manager/barbican.pp":[{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"4213cf3e7193bc3aadbc3889093a7e3304860d96","unresolved":true,"context_lines":[{"line_number":36,"context_line":"  $retry_delay            \u003d $::os_service_default,"},{"line_number":37,"context_line":"  $number_of_retries      \u003d $::os_service_default,"},{"line_number":38,"context_line":"  $barbican_endpoint_type \u003d $::os_service_default,"},{"line_number":39,"context_line":") {"},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"  include cinder::deps"},{"line_number":42,"context_line":""}],"source_content_type":"text/x-puppet","patch_set":9,"id":"097486ec_2adacb25","line":39,"updated":"2021-01-28 00:58:55.000000000","message":"For completeness, how about adding these barbican options:\n\n[1] https://opendev.org/openstack/castellan/src/branch/master/castellan/key_manager/barbican_key_manager.py#L64\n[2] https://opendev.org/openstack/castellan/src/branch/master/castellan/key_manager/barbican_key_manager.py#L69","commit_id":"e749318b7ac78834c1350008042a66047a5a9f45"}],"spec/classes/cinder_init_spec.rb":[{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"c0ffb9537619208a8b95e8bd9f996b52be262e37","unresolved":true,"context_lines":[{"line_number":46,"context_line":"        is_expected.to contain_cinder_config(\u0027DEFAULT/host\u0027).with_value(\u0027\u003cSERVICE DEFAULT\u003e\u0027)"},{"line_number":47,"context_line":"        is_expected.to contain_cinder_config(\u0027DEFAULT/enable_new_services\u0027).with_value(\u0027\u003cSERVICE DEFAULT\u003e\u0027)"},{"line_number":48,"context_line":"        is_expected.to contain_cinder_config(\u0027oslo_concurrency/lock_path\u0027).with(:value \u003d\u003e \u0027/var/lock/cinder\u0027)"},{"line_number":49,"context_line":"        is_expected.to contain_cinder_config(\u0027key_manager/backend\u0027).with_value(\u0027\u003cSERVICE DEFAULT\u003e\u0027)"},{"line_number":50,"context_line":"        is_expected.to contain_cinder_config(\u0027barbican/barbican_endpoint\u0027).with_value(\u0027\u003cSERVICE DEFAULT\u003e\u0027)"},{"line_number":51,"context_line":"        is_expected.to contain_cinder_config(\u0027barbican/auth_endpoint\u0027).with_value(\u0027\u003cSERVICE DEFAULT\u003e\u0027)"},{"line_number":52,"context_line":""}],"source_content_type":"text/x-ruby","patch_set":17,"id":"63e6b8ff_9253693a","side":"PARENT","line":49,"updated":"2021-05-11 17:39:11.000000000","message":"Removing these make sense, but a good enhancement (maybe a follow-up patch?) would be to verify the new cinder::key_manager and cinder::key_manager::barbican resources are pulled if the now-legacy cinder::init parameters are defined.","commit_id":"8e233cfb92ba158416b425ec86aca8ba9ecb66bf"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"9c6efdd0bd62df1489e5e7bc431c5dbd264d2991","unresolved":true,"context_lines":[{"line_number":46,"context_line":"        is_expected.to contain_cinder_config(\u0027DEFAULT/host\u0027).with_value(\u0027\u003cSERVICE DEFAULT\u003e\u0027)"},{"line_number":47,"context_line":"        is_expected.to contain_cinder_config(\u0027DEFAULT/enable_new_services\u0027).with_value(\u0027\u003cSERVICE DEFAULT\u003e\u0027)"},{"line_number":48,"context_line":"        is_expected.to contain_cinder_config(\u0027oslo_concurrency/lock_path\u0027).with(:value \u003d\u003e \u0027/var/lock/cinder\u0027)"},{"line_number":49,"context_line":"        is_expected.to contain_cinder_config(\u0027key_manager/backend\u0027).with_value(\u0027\u003cSERVICE DEFAULT\u003e\u0027)"},{"line_number":50,"context_line":"        is_expected.to contain_cinder_config(\u0027barbican/barbican_endpoint\u0027).with_value(\u0027\u003cSERVICE DEFAULT\u003e\u0027)"},{"line_number":51,"context_line":"        is_expected.to contain_cinder_config(\u0027barbican/auth_endpoint\u0027).with_value(\u0027\u003cSERVICE DEFAULT\u003e\u0027)"},{"line_number":52,"context_line":""}],"source_content_type":"text/x-ruby","patch_set":17,"id":"c09a9132_6382ba1b","side":"PARENT","line":49,"in_reply_to":"63e6b8ff_9253693a","updated":"2021-05-11 23:21:42.000000000","message":"That makes sense. I\u0027ll work on it later.","commit_id":"8e233cfb92ba158416b425ec86aca8ba9ecb66bf"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"11bf3b19e9d5996b10e1cfd5c7696db65ddf0f31","unresolved":true,"context_lines":[{"line_number":259,"context_line":"      )}"},{"line_number":260,"context_line":"    end"},{"line_number":261,"context_line":""},{"line_number":262,"context_line":"    context \u0027with keymgr parameters\u0027 do"},{"line_number":263,"context_line":"      let :params do"},{"line_number":264,"context_line":"        req_params.merge!({"},{"line_number":265,"context_line":"          :keymgr_backend             \u003d\u003e \u0027barbican\u0027,"}],"source_content_type":"text/x-ruby","patch_set":17,"id":"21ce2eb0_15d28a12","line":262,"updated":"2021-05-12 06:49:43.000000000","message":"@Alan I noticed that I left these test cases which ensure the deprecated parameters work. So I think we don\u0027t need any additional test cases.","commit_id":"6dc69ab67f9b72ab5c7653f9f268ab1e0a7a83e7"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"62f5e58aa9847fe0c0872c4bab08ba24d1012477","unresolved":false,"context_lines":[{"line_number":259,"context_line":"      )}"},{"line_number":260,"context_line":"    end"},{"line_number":261,"context_line":""},{"line_number":262,"context_line":"    context \u0027with keymgr parameters\u0027 do"},{"line_number":263,"context_line":"      let :params do"},{"line_number":264,"context_line":"        req_params.merge!({"},{"line_number":265,"context_line":"          :keymgr_backend             \u003d\u003e \u0027barbican\u0027,"}],"source_content_type":"text/x-ruby","patch_set":17,"id":"b1c1f3bc_8b17c0dd","line":262,"in_reply_to":"21ce2eb0_15d28a12","updated":"2021-05-14 18:20:25.000000000","message":"You\u0027re absolutely correct, and I apologize for the distraction because I apparently missed this!!","commit_id":"6dc69ab67f9b72ab5c7653f9f268ab1e0a7a83e7"}]}