ozone icon indicating copy to clipboard operation
ozone copied to clipboard

Published 20 hours ago • verified publisher icon apache

HDDS-7347. [Multi-Tenant] Add proper error message to TenantAssignAdmin and TenantRevokeAdmin

Open aswinshakil opened this issue 2 years ago • 2 comments

What changes were proposed in this pull request?

  • Add error message when the same user is assigned as tenant admin.
  • Add error message when the non-admin user is revoked as tenant admin.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7347

How was this patch tested?

The patch was tested using unit tests.

aswinshakil avatar Oct 17 '22 22:10 aswinshakil

Why can't these APIs be idempotent? if the user is already admin, it should pass, and if the user is not admin, it revoke should pass as well. Why raise an exception?

kerneltime avatar Oct 17 '22 22:10 kerneltime

The goal is to propagate back clear messages for these scenarios to improve user experience.

aswinshakil avatar Oct 17 '22 23:10 aswinshakil

The goal is to propagate back clear messages for these scenarios to improve user experience.

I would still push back against this change. Usually, set ownership or admin access APIs/CLIs are idempotent. The only variation I can imagine is printing that the user was already admin or had access revoked, but the exit/return code/status should be successful.

kerneltime avatar Oct 18 '22 17:10 kerneltime

Thanks @aswinshakil for the effort. Closing this PR without merging it after our discussion with @kerneltime .

smengcl avatar Oct 21 '22 06:10 smengcl