)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"c420d1d17880a88f3fc7b82c7abed404170ec80f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"17646e74_776bd612","updated":"2022-04-01 19:05:16.000000000","message":"I\u0027m not sure how you want to proceed, but I\u0027m struggling to decide how puppet should handle driver parameters that really don\u0027t feel like they should be optional, even if the cinder code has a blank string for some default values.","commit_id":"5107b9ba21d7f970d6551c5f3608684d6e8b0cbe"}],"manifests/backend/san.pp":[{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"c420d1d17880a88f3fc7b82c7abed404170ec80f","unresolved":true,"context_lines":[{"line_number":19,"context_line":"#   Defaults to $::os_service_default."},{"line_number":20,"context_line":"#"},{"line_number":21,"context_line":"# [*san_thin_provision*]"},{"line_number":22,"context_line":"#   (optional) Use thin provisioning for SAN volumes? Defaults to true."},{"line_number":23,"context_line":"#   Defaults to $::os_service_default."},{"line_number":24,"context_line":"#"},{"line_number":25,"context_line":"# [*san_ip*]"}],"source_content_type":"text/x-puppet","patch_set":4,"id":"6dacafe0_6895573a","line":22,"range":{"start_line":22,"start_character":53,"end_line":22,"end_character":70},"updated":"2022-04-01 19:05:16.000000000","message":"nit (should be deleted)","commit_id":"5107b9ba21d7f970d6551c5f3608684d6e8b0cbe"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"ecfa97d4f6bd5cf1e1582cff9d827ee0cc40ff02","unresolved":false,"context_lines":[{"line_number":19,"context_line":"#   Defaults to $::os_service_default."},{"line_number":20,"context_line":"#"},{"line_number":21,"context_line":"# [*san_thin_provision*]"},{"line_number":22,"context_line":"#   (optional) Use thin provisioning for SAN volumes? Defaults to true."},{"line_number":23,"context_line":"#   Defaults to $::os_service_default."},{"line_number":24,"context_line":"#"},{"line_number":25,"context_line":"# [*san_ip*]"}],"source_content_type":"text/x-puppet","patch_set":4,"id":"e3819e63_3f284511","line":22,"range":{"start_line":22,"start_character":53,"end_line":22,"end_character":70},"in_reply_to":"6dacafe0_6895573a","updated":"2022-04-02 02:35:32.000000000","message":"Done","commit_id":"5107b9ba21d7f970d6551c5f3608684d6e8b0cbe"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"c420d1d17880a88f3fc7b82c7abed404170ec80f","unresolved":true,"context_lines":[{"line_number":23,"context_line":"#   Defaults to $::os_service_default."},{"line_number":24,"context_line":"#"},{"line_number":25,"context_line":"# [*san_ip*]"},{"line_number":26,"context_line":"#   (optional) IP address of SAN controller."},{"line_number":27,"context_line":"#   Defaults to $::os_service_default."},{"line_number":28,"context_line":"#"},{"line_number":29,"context_line":"# [*san_login*]"},{"line_number":30,"context_line":"#   (optional) Username for SAN controller. Defaults to \u0027admin\u0027."}],"source_content_type":"text/x-puppet","patch_set":4,"id":"698a0c61_af41fc7c","line":27,"range":{"start_line":26,"start_character":0,"end_line":27,"end_character":38},"updated":"2022-04-01 19:05:16.000000000","message":"I have no idea whether cinder\u0027s SAN backend is in widespread use in 2022, but the code that added support for it is quite old. The cinder driver dates back to 2012 and the puppet-cinder resource in 2014.\n\nI mention this because I suspect puppet-cinder\u0027s use of \u0027undef\u0027 for parameters like this may have been influenced by the cinder driver\u0027s default value of \u0027\u0027 (blank string) [1].\n\n[1] https://opendev.org/openstack/cinder/src/branch/master/cinder/volume/drivers/san/san.py#L44\n\nThis is very odd, because nothing will work unless the user provides a valid IP address for their device. I believe the cinder driver\u0027s option should not have any default value, and the corresponding puppet-cinder argument should be required (not optional).\n\nThe san_password is another example of something that doesn\u0027t seem like there would be any valid default value, unless it\u0027s related to the san_private_key parameter. Maybe you need to define only one of them, and the other can be left blank? I don\u0027t know enough about the SAN driver, and it doesn\u0027t look like there\u0027s is much code activity in that area.","commit_id":"5107b9ba21d7f970d6551c5f3608684d6e8b0cbe"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"ecfa97d4f6bd5cf1e1582cff9d827ee0cc40ff02","unresolved":true,"context_lines":[{"line_number":23,"context_line":"#   Defaults to $::os_service_default."},{"line_number":24,"context_line":"#"},{"line_number":25,"context_line":"# [*san_ip*]"},{"line_number":26,"context_line":"#   (optional) IP address of SAN controller."},{"line_number":27,"context_line":"#   Defaults to $::os_service_default."},{"line_number":28,"context_line":"#"},{"line_number":29,"context_line":"# [*san_login*]"},{"line_number":30,"context_line":"#   (optional) Username for SAN controller. Defaults to \u0027admin\u0027."}],"source_content_type":"text/x-puppet","patch_set":4,"id":"7cf8fe2a_a3576e2e","line":27,"range":{"start_line":26,"start_character":0,"end_line":27,"end_character":38},"in_reply_to":"698a0c61_af41fc7c","updated":"2022-04-02 02:35:32.000000000","message":"I understand that point and actually I feel the current implementation in puppet-cinder is \"incomplete\" and needs some improvement to validate the required parameters.\n\nHowever parameter validation is a different topic, IMO. The current implementation does not require these parameters because of the default value (undef) and does not fail even if a user doesn\u0027t give the parameters required to make the driver functional. This issue is not fixed or changed by my change. The only difference is the new implementation removes the parameter by default, but this doesn\u0027t cause difference because the cinder.conf file provided by packages do not include effective parameters (but only comment lines) by default.\n\nIdeally we need to introduce a validation to ensure the required parameters are given. I\u0027ll look into that later, but it would requires some effort to dig into cinder code, so I\u0027d leave that as a separate topic.","commit_id":"5107b9ba21d7f970d6551c5f3608684d6e8b0cbe"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"06f01c109b47d8978d01d2c0cfb623bcedfd4867","unresolved":true,"context_lines":[{"line_number":23,"context_line":"#   Defaults to $::os_service_default."},{"line_number":24,"context_line":"#"},{"line_number":25,"context_line":"# [*san_ip*]"},{"line_number":26,"context_line":"#   (optional) IP address of SAN controller."},{"line_number":27,"context_line":"#   Defaults to $::os_service_default."},{"line_number":28,"context_line":"#"},{"line_number":29,"context_line":"# [*san_login*]"},{"line_number":30,"context_line":"#   (optional) Username for SAN controller. Defaults to \u0027admin\u0027."}],"source_content_type":"text/x-puppet","patch_set":4,"id":"cc723173_9552679a","line":27,"range":{"start_line":26,"start_character":0,"end_line":27,"end_character":38},"in_reply_to":"7cf8fe2a_a3576e2e","updated":"2022-04-04 16:28:47.000000000","message":"Although I suspect there aren\u0027t any actual deployments that will benefit, I do agree the change will avoid parameters like this becoming unmanaged.","commit_id":"5107b9ba21d7f970d6551c5f3608684d6e8b0cbe"}]}