)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"ca2eb60cce37227cbbf8bf6fc4ba3854d162f405","unresolved":true,"context_lines":[{"line_number":11,"context_line":"maintaining our own defaults and saves us from updating these values"},{"line_number":12,"context_line":"according to any change in cinder itself."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"This also replaces usage of undef, which is effectively same as"},{"line_number":15,"context_line":"$::os_service_default, because $::os_service_default is now widely"},{"line_number":16,"context_line":"used."},{"line_number":17,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"59374ea3_f435d027","line":14,"range":{"start_line":14,"start_character":35,"end_line":14,"end_character":60},"updated":"2022-03-22 15:11:25.000000000","message":"They may be effectively the same, but there is a semantic difference. Netapp\u0027s undef parameters don\u0027t have a default value, and specifically rely on them being configured in order for the value to take effect.\n\nConsider \u0027netapp_copyoffload_tool_path\u0027. There isn\u0027t a default value so, in my view, having puppet\u0027s default value of \u0027undef\u0027 would inform me that I need to specify a value. Conversely, if I see $::os_service_default then I\u0027d be included to think, \"sure, let\u0027s go with that\" and I wouldn\u0027t specify a value.","commit_id":"517c78457a33422a555ce6cb1f1dbe963dfc3dea"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"ff84456f2e1bbc78ae11749be1ba9f64a4913a9e","unresolved":false,"context_lines":[{"line_number":11,"context_line":"maintaining our own defaults and saves us from updating these values"},{"line_number":12,"context_line":"according to any change in cinder itself."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"This also replaces usage of undef, which is effectively same as"},{"line_number":15,"context_line":"$::os_service_default, because $::os_service_default is now widely"},{"line_number":16,"context_line":"used."},{"line_number":17,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"008f36df_ea89bcb7","line":14,"range":{"start_line":14,"start_character":35,"end_line":14,"end_character":60},"in_reply_to":"59374ea3_f435d027","updated":"2022-03-22 15:38:40.000000000","message":"I\u0027ve reverted the change about the two parameters which currently default to undef .\n\nChecking the current implementation quickly, I still think  $::os_service_default results in the effectively same result for both these two parameters but the observation should be documented better, so I\u0027ll revisit this later and submit separate changes.","commit_id":"517c78457a33422a555ce6cb1f1dbe963dfc3dea"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"fd1303f251eee9be2354e27ee63c696cb3d5797d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"3e75cc0b_dfb45b9c","updated":"2022-03-24 00:46:05.000000000","message":"recheck","commit_id":"ffaf1132f6a94fd20363a91a63be474eda940e95"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"93e17a7769cd82ce5cb50de33504dc6e8cc6259a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"da6b2df8_10e6109d","updated":"2022-03-30 08:06:56.000000000","message":"recheck","commit_id":"accab30c062ee4d9530211dcbafe39af839750eb"}],"manifests/backend/netapp.pp":[{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"ca2eb60cce37227cbbf8bf6fc4ba3854d162f405","unresolved":true,"context_lines":[{"line_number":58,"context_line":"#   (optional) This option specifies the virtual storage server (Vserver)"},{"line_number":59,"context_line":"#   name on the storage cluster on which provisioning of block storage volumes"},{"line_number":60,"context_line":"#   should occur."},{"line_number":61,"context_line":"#   Defaults to $::os_service_default."},{"line_number":62,"context_line":"#"},{"line_number":63,"context_line":"# [*expiry_thres_minutes*]"},{"line_number":64,"context_line":"#   (optional) This parameter specifies the threshold for last access time for"}],"source_content_type":"text/x-puppet","patch_set":3,"id":"4149e8bd_99b7971d","line":61,"updated":"2022-03-22 15:11:25.000000000","message":"Per my comment on the commit message, this is another example where the cinder parameter has no default value, so $::os_service_default won\u0027t get you anything. Defaulting to undef may do a better job of signalling to the user that they need to explicitly set a value.","commit_id":"517c78457a33422a555ce6cb1f1dbe963dfc3dea"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"ff84456f2e1bbc78ae11749be1ba9f64a4913a9e","unresolved":false,"context_lines":[{"line_number":58,"context_line":"#   (optional) This option specifies the virtual storage server (Vserver)"},{"line_number":59,"context_line":"#   name on the storage cluster on which provisioning of block storage volumes"},{"line_number":60,"context_line":"#   should occur."},{"line_number":61,"context_line":"#   Defaults to $::os_service_default."},{"line_number":62,"context_line":"#"},{"line_number":63,"context_line":"# [*expiry_thres_minutes*]"},{"line_number":64,"context_line":"#   (optional) This parameter specifies the threshold for last access time for"}],"source_content_type":"text/x-puppet","patch_set":3,"id":"03eb7e05_a7c40fce","line":61,"in_reply_to":"4149e8bd_99b7971d","updated":"2022-03-22 15:38:40.000000000","message":"Done","commit_id":"517c78457a33422a555ce6cb1f1dbe963dfc3dea"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"e08916c0c866d9a9f3839224e228e7e7163da8fa","unresolved":true,"context_lines":[{"line_number":219,"context_line":"  $nfs_shares                       \u003d undef,"},{"line_number":220,"context_line":"  $nfs_shares_config                \u003d \u0027/etc/cinder/shares.conf\u0027,"},{"line_number":221,"context_line":"  $nfs_mount_options                \u003d $::os_service_default,"},{"line_number":222,"context_line":"  $netapp_copyoffload_tool_path     \u003d undef,"},{"line_number":223,"context_line":"  $netapp_host_type                 \u003d $::os_service_default,"},{"line_number":224,"context_line":"  $manage_volume_type               \u003d false,"},{"line_number":225,"context_line":"  $extra_options                    \u003d {},"}],"source_content_type":"text/x-puppet","patch_set":4,"id":"52c4d7d5_be16e62a","line":222,"updated":"2022-03-25 12:41:18.000000000","message":"I reconsidered my previous concerns about how to handle the netapp_copyoffload_tool_path parameter, as we discussed in https://review.opendev.org/c/openstack/puppet-cinder/+/834738.\n\nI now agree with your approach of using $::os_service_default, which means I would vote to approve the other patch when it passes CI.\n\nBut with that understanding, we could keep the two patches separate, or merge the other one into this. I think a single patch is cleaner, so I\u0027ll vote +2 here, but withhold +W so that you can consider whether you want to merge the other patch into this one.","commit_id":"ffaf1132f6a94fd20363a91a63be474eda940e95"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"f684556f2770b254da240a1ff8945dbd489bb50a","unresolved":false,"context_lines":[{"line_number":219,"context_line":"  $nfs_shares                       \u003d undef,"},{"line_number":220,"context_line":"  $nfs_shares_config                \u003d \u0027/etc/cinder/shares.conf\u0027,"},{"line_number":221,"context_line":"  $nfs_mount_options                \u003d $::os_service_default,"},{"line_number":222,"context_line":"  $netapp_copyoffload_tool_path     \u003d undef,"},{"line_number":223,"context_line":"  $netapp_host_type                 \u003d $::os_service_default,"},{"line_number":224,"context_line":"  $manage_volume_type               \u003d false,"},{"line_number":225,"context_line":"  $extra_options                    \u003d {},"}],"source_content_type":"text/x-puppet","patch_set":4,"id":"e132566c_7099fcab","line":222,"in_reply_to":"0e95896e_a5a6c0d9","updated":"2022-03-29 00:55:41.000000000","message":"Done","commit_id":"ffaf1132f6a94fd20363a91a63be474eda940e95"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"cca4f51a46f0482219cae7b33dd6b28721269c9d","unresolved":true,"context_lines":[{"line_number":219,"context_line":"  $nfs_shares                       \u003d undef,"},{"line_number":220,"context_line":"  $nfs_shares_config                \u003d \u0027/etc/cinder/shares.conf\u0027,"},{"line_number":221,"context_line":"  $nfs_mount_options                \u003d $::os_service_default,"},{"line_number":222,"context_line":"  $netapp_copyoffload_tool_path     \u003d undef,"},{"line_number":223,"context_line":"  $netapp_host_type                 \u003d $::os_service_default,"},{"line_number":224,"context_line":"  $manage_volume_type               \u003d false,"},{"line_number":225,"context_line":"  $extra_options                    \u003d {},"}],"source_content_type":"text/x-puppet","patch_set":4,"id":"0e95896e_a5a6c0d9","line":222,"in_reply_to":"52c4d7d5_be16e62a","updated":"2022-03-25 13:43:07.000000000","message":"Thanks Alan, I\u0027ll put WIP and squash the subsequent commit to this.\nBefore that let me take short time to retest usage of undef in my local env, to ensure our discussion depends on the right observation.","commit_id":"ffaf1132f6a94fd20363a91a63be474eda940e95"}]}