Accept both perm and perms in acl objects #349
Conversation
src/node-zk.cpp
Outdated
| @@ -999,7 +999,9 @@ class ZooKeeper: public Nan::ObjectWrap { | |||
|
|
|||
| Nan::Utf8String _scheme (toString(toLocalVal(obj, LOCAL_STRING("scheme")))); | |||
| Nan::Utf8String _auth (toString(toLocalVal(obj, LOCAL_STRING("auth")))); | |||
| uint32_t _perms = toUint(toLocalVal(obj, LOCAL_STRING("perm"))); | |||
| // check if perm or perms exists, later can be changed to accept just 'perms' on next major release | |||
There was a problem hiding this comment.
I would like to handle the "perm" and "perms" in the JavaScript code, instead of in the C++ AddOn code. Would you consider changing it into something similar to the Work in progress here? master...DavidVujic:node-zookeeper:acl-perm-variations
There was a problem hiding this comment.
@DavidVujic Yes, I can do that.
My idea was to make it consistent on the addon level, not just JavaScript. Since it's inconsistent at the addon level I did a fix there. The next major release can be made perms consistent on every level, and can be easily achieved later by removing just the property check.
There was a problem hiding this comment.
@DavidVujic should we prioritize perms over perm if both are given for consistency?
There was a problem hiding this comment.
Good question! With the mapping of the acl code I think it will be dependent on the order of the keys. I think that is ok and good enough.
|
This is great! Thank you for this Pull Request @maharjanraj I also had started with this, with an approach very similar to yours. 😄 |
|
@DavidVujic I have updated the Pull Request. Please review. |
|
I'll merge and publish a new version as soon as possible (within a couple of days at most). Huge thank you for this contribution @maharjanraj 👏 |
Accept both perm and perms while creating an acl object
Description
Accept both perm and perms in acl objects while setting acl for a zookeeper node
Motivation and Context
get_acl returns acl objects with perms key, while set_acl accepts only perm key, these need to be consistent, so updated the code to take both, so there is no any breaking change and it's consistent.
How Has This Been Tested?
Added an integration test: tests/integration/getAndSetAcltest.js
Types of changes
Checklist: