Primary issue is that (at least on my reading) the api-ref and the commit message/release note conflict over whether users are allowed to make the 3 action API calls. The api-ref says that they can, with success dependent on satisfying a safe delete (as for the Attachments API delete call), but the commit message/relnote say the action calls are service-only. The code looks like it's implementing what the api-ref says.
docs: changes are good, read well, render correctly in HTML, links all work (only discuss configuration, so not affected by the above)
api-ref: nit: os-detach, os-force_detach are missing the 409 in the response codes list (only mentioning it because you have it for the os-terminate_connection action)
cinder/volume/api.py
nit: line 2583 (reason=) s/atachment/attachment/
cinder/tests/unit/api/contrib/test_admin_actions.py
nit: line 1040: mock_deletion_allowed returns True, but the real function either raises or returns None; I think it would be better to return None from the mock
cinder/tests/unit/api/v3/test_attachments.py
nit: test_attachment_deletion_allowed_service_call() relies on the default keystone_auth.service_token_roles containing 'service' (which is fine), but since there was talk at some point of hard-coding a 'service' check, it would be good to have a test that uses a non-default value for the config opt
cinder/tests/unit/policies/test_volume_actions.py
in test_owner_can_attach_detach_volume, line 973: I think you deleted 'body = {"os-detach":{}}' by mistake and not the test is checking 2 attach calls
release note: if you revise, the single backticks produce italics; you need double backticks for monospace font
The code in volume/api.py looks fine and the tests are thorough
reviewed cinder patch cc20649efa7383f495
Primary issue is that (at least on my reading) the api-ref and the commit message/release note conflict over whether users are allowed to make the 3 action API calls. The api-ref says that they can, with success dependent on satisfying a safe delete (as for the Attachments API delete call), but the commit message/relnote say the action calls are service-only. The code looks like it's implementing what the api-ref says.
docs: changes are good, read well, render correctly in HTML, links all work (only discuss configuration, so not affected by the above)
api-ref: nit: os-detach, os-force_detach are missing the 409 in the response codes list (only mentioning it because you have it for the os-terminate_ connection action)
cinder/ volume/ api.py attachment/
nit: line 2583 (reason=) s/atachment/
cinder/ tests/unit/ api/contrib/ test_admin_ actions. py allowed returns True, but the real function either raises or returns None; I think it would be better to return None from the mock
nit: line 1040: mock_deletion_
cinder/ tests/unit/ api/v3/ test_attachment s.py _deletion_ allowed_ service_ call() relies on the default keystone_ auth.service_ token_roles containing 'service' (which is fine), but since there was talk at some point of hard-coding a 'service' check, it would be good to have a test that uses a non-default value for the config opt
nit: test_attachment
cinder/ tests/unit/ policies/ test_volume_ actions. py can_attach_ detach_ volume, line 973: I think you deleted 'body = {"os-detach":{}}' by mistake and not the test is checking 2 attach calls
in test_owner_
release note: if you revise, the single backticks produce italics; you need double backticks for monospace font
The code in volume/api.py looks fine and the tests are thorough