)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"98862490ac8d365ab4af76e864b8b4fb98aec9e4","unresolved":true,"context_lines":[{"line_number":9,"context_line":"Currently the netapp_copyoffload_tool_path parameter defaults to undef,"},{"line_number":10,"context_line":"which results in an empty value in cinder.conf like;"},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"[netapp_backend]"},{"line_number":13,"context_line":"netapp_copyoffload_tool_path\u003d"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"However the current netapp driver treats this value in the same manner"},{"line_number":16,"context_line":"as the service default value (None), as it always check the value is"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"c66646b9_40e65d4c","line":13,"range":{"start_line":12,"start_character":0,"end_line":13,"end_character":29},"updated":"2022-03-27 14:41:08.000000000","message":"Hmm... I have retested this but it seems the behavior I described here is wrong, and usage of undef just leaves the parameter \"unamanged\".\n\nI think this change is still valid even with that behavior so I\u0027ll go ahead to squash this into the base change but let me know if you have any concern.","commit_id":"eab0ac38bf291bf7554c20df9c0356ac386c5db1"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"acf2ae4317ff127a326da1f1b82422e9f180f6ec","unresolved":true,"context_lines":[{"line_number":9,"context_line":"Currently the netapp_copyoffload_tool_path parameter defaults to undef,"},{"line_number":10,"context_line":"which results in an empty value in cinder.conf like;"},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"[netapp_backend]"},{"line_number":13,"context_line":"netapp_copyoffload_tool_path\u003d"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"However the current netapp driver treats this value in the same manner"},{"line_number":16,"context_line":"as the service default value (None), as it always check the value is"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"b5e13bfc_9b0514c6","line":13,"range":{"start_line":12,"start_character":0,"end_line":13,"end_character":29},"in_reply_to":"c66646b9_40e65d4c","updated":"2022-03-27 14:56:48.000000000","message":"@Alan\n\nDo you sharing your thoughts before I actually squash this change to the base one ? \nThe previous discussion was based on my wrong observation(sorry about that). I think this change is still valid, but would like to hear your opinion.","commit_id":"eab0ac38bf291bf7554c20df9c0356ac386c5db1"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"d670e151208b94b6b5b4f108cee413545b11d1b6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"0fc4c20b_f57e1f29","updated":"2022-03-25 12:42:42.000000000","message":"I reconsidered my position and now agree with the approach taken in this patch. But this now suggests it might be worth merging this change into https://review.opendev.org/c/openstack/puppet-cinder/+/834588.\n\nI apologize if I\u0027m confusing things or causing churn.","commit_id":"eab0ac38bf291bf7554c20df9c0356ac386c5db1"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"993c29b6a53cc6abaa9753918768a85953cd3aca","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"3cf07523_a8bcfeab","updated":"2022-03-22 16:14:54.000000000","message":"Sorry, but I  don\u0027t like this approach, and I refer back to my previous comment that it\u0027s misleading to use $::os_service_default when cinder doesn\u0027t provide a default value. FWIW, cinder is highly unlikely to change the netapp_copyoffload_tool_path option.\n\nIf the goal is to avoid adding empty values to cinder.conf then how about something like this:\n\n  if $netapp_copyoffload_tool_path !\u003d undef {\n    cinder_config {\n      \"${name}/netapp_copyoffload_tool_path\": value \u003d\u003e $netapp_copyoffload_tool_path;\n    }\n  }\n\nI think that would also continue to work in the unlikely event that cinder *does* switch to using an actual default value.","commit_id":"eab0ac38bf291bf7554c20df9c0356ac386c5db1"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"ab8f5d41c32531761ecf2bc00c4dd57d37c98ba4","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"67803fb2_da784b19","in_reply_to":"3cf07523_a8bcfeab","updated":"2022-03-22 16:35:29.000000000","message":"I\u0027m afraid I\u0027ve not yet fully understood your concerns.\n\nFor netapp_copyoffload_tool_path option, no default value is defined by oslo.config interface, which makes the parameter \"default to None\". The current netapp driver treats None and \u0027\u0027 in the same manner, and disables copyoffload feature for both these values.\n\nThe current default is just redundant because it always sets the optional parameter, in a wired format which eventually results in service default.\n\n\n\u003e If the goal is to avoid adding empty values to cinder.conf then how about something like this:\n\nI don\u0027t agree with this. This eventually results in missing netapp_copyoffload_tool_path, which is effectively same as the current change, and leaves the parameter unmanaged if a user set the parameter once but remove that later.","commit_id":"eab0ac38bf291bf7554c20df9c0356ac386c5db1"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"28678733773916633fc4b869668edcdf048f93ca","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"6bd338f3_913b8462","in_reply_to":"67803fb2_da784b19","updated":"2022-03-22 17:30:30.000000000","message":"\u003e I\u0027m afraid I\u0027ve not yet fully understood your concerns.\n\u003e \n\u003e For netapp_copyoffload_tool_path option, no default value is defined by oslo.config interface, which makes the parameter \"default to None\". The current netapp driver treats None and \u0027\u0027 in the same manner, and disables copyoffload feature for both these values.\n\nI agree.\n\n\u003e The current default is just redundant because it always sets the optional parameter, in a wired format which eventually results in service default.\n\nI think you\u0027re saying the current code (which this patch would modify) sets an empty/blank value \"netapp_copyoffload_tool_path\u003d\" which I agree is redundant and undesirable.\n\n\u003e \u003e If the goal is to avoid adding empty values to cinder.conf then how about something like this:\n\u003e \n\u003e I don\u0027t agree with this. This eventually results in missing netapp_copyoffload_tool_path, which is effectively same as the current change, and leaves the parameter unmanaged if a user set the parameter once but remove that later.\n\nThis could work:\n\n  if $netapp_copyoffload_tool_path \u003d\u003d undef {\n    cinder_config {\n      \"${name}/netapp_copyoffload_tool_path\": ensure \u003d\u003e absent;\n    }\n  } else {\n    cinder_config {\n      \"${name}/netapp_copyoffload_tool_path\": value \u003d\u003e $netapp_copyoffload_tool_path;\n    }\n  }\n\nBut it\u0027s also getting rather unwieldy, and I\u0027m starting to rethink my objection to using $::os_service_default for the default value. I\u0027ll take a few minutes to think about it while we wait for CI results.","commit_id":"eab0ac38bf291bf7554c20df9c0356ac386c5db1"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"65c344f99c0f7a5c0b3499c09cfa0a091b556b7c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"0a22e512_76c8dd82","updated":"2022-03-27 15:08:29.000000000","message":"Dumping my test results for reference.\n https://paste.opendev.org/show/bCrE2Shxso0FStLPVnD8/","commit_id":"77ad11fac06796e8758bd88209a434f9b2ce629e"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"44cf762ff0e49c9e7305f0e0a32996d1fa7fcec0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"00ce8276_29f7f8a8","updated":"2022-03-28 15:59:26.000000000","message":"Try this:\n\n  [\u0027netapp_vserver\u0027, \u0027netapp_copyoffload_tool_path\u0027].each |String $netapp_opt| {\n    $netapp_val \u003d getvar(\"${netapp_opt}\")\n    if $netapp_val \u003d\u003d undef {\n      cinder_config {\n        \"${name}/${netapp_opt}\": ensure \u003d\u003e absent;\n      }\n    } else {\n      cinder_config {\n        \"${name}/${netapp_opt}\": value \u003d\u003e $netapp_val;\n      }\n  }\n","commit_id":"77ad11fac06796e8758bd88209a434f9b2ce629e"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"1a9bca797f8f7f17c91ad211f99c0e350a66fe45","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"13ae2313_f6a8ef6b","in_reply_to":"00ce8276_29f7f8a8","updated":"2022-03-28 16:24:37.000000000","message":"If a parameter is set to undef then puppet automatically replaces the parameter by its default value.\n https://paste.opendev.org/show/bQbJ3akBzrnNbA2TPBRc/\n\nBecause of this behavior, the current implementation is effectively same as what you wrote.\n\n- If netapp_vserver is not set, then it defaults to $::os_service_default and is removed (effectively same as ensure \u003d\u003e absent)\n- If netapp_vserver is set to undef, puppet replaces the undef value by the default value which is $::os_service_default and remove the parameter\n- If netapp_vserver is set to $::os_service_default then the parameter is removed from cinder.conf","commit_id":"77ad11fac06796e8758bd88209a434f9b2ce629e"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"688d02e1f6e917dae488cd27c459066141d0ce21","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":2,"id":"6db87412_897875c0","in_reply_to":"13ae2313_f6a8ef6b","updated":"2022-03-28 17:30:29.000000000","message":"Oh, OK, I misread the situation. I incorrectly thought you were previously saying $::os_service_default  was not properly unmanaging the parameters, which is why I tested my approach. Howevever, I now see that $::os_service_default *does* do the right thing (L76 of your pastebin).\n\nSo I\u0027m OK using $::os_service_default, and you can consider merging that into the other patch.","commit_id":"77ad11fac06796e8758bd88209a434f9b2ce629e"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"7e7997ce3db569b127b02984db44a5b9629eeefd","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":2,"id":"d49edc7c_8e59141f","in_reply_to":"6db87412_897875c0","updated":"2022-03-29 00:54:50.000000000","message":"Thanks for checking ! I\u0027ve updated the patch.","commit_id":"77ad11fac06796e8758bd88209a434f9b2ce629e"}]}