-
Notifications
You must be signed in to change notification settings - Fork 23.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add mongodb_role module #45488
Add mongodb_role module #45488
Conversation
4432932
to
714a21c
Compare
@jkramarz you'll need to fix linter errors to get this merged Also, if you wish to be a maintainer, self-add to: |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
def role_find(module, client, db_name, role, authentication_restrictions_supported): | ||
db = client[db_name] | ||
|
||
if authentication_restrictions_supported: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactor:
additional_kwargs = (
{'showAuthenticationRestrictions': True}
if authentication_restrictions_supported
else {}
)
result = db.command(
'rolesInfo',
role,
showPrivileges=True,
**additional_kwargs
)
return creds | ||
|
||
|
||
def check_authentication_restrictions_support(client): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better name: is_authentication_restrictions_supported
try: | ||
with open(config_file) as f: | ||
config.readfp(f) | ||
creds = dict( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can probably move this statement outside of with-block
try: | ||
with open(config_file) as f: | ||
config.readfp(f) | ||
creds = dict( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, please use dict literal instead of constuctor ({'key': 'val'}
instead of dict(key='val')
)
config = configparser.RawConfigParser() | ||
|
||
try: | ||
with open(config_file) as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrap these pieces of code into two try/except statements to be precise with what you're trapping.
- one around with-block (with readfp call) catching
IOError
- the other one around creds dict constuction catching
configparser.NoOptionError
# MongoDB module specific support methods. | ||
# | ||
|
||
def check_compatibility(module, client): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a hanging PR implementing it in a nicer way: https://github.com/ansible/ansible/pull/44110/files#diff-d4702ee9f9f2977f8c82cb5b4e8b0e26R221. You may want to try borrowing some ideas from there.
db = client[db_name] | ||
|
||
if authentication_restrictions_supported: | ||
result = db.command('createRole', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This snippet looks just like one in role_find
. It probably deserves being moved into a reusable function.
) | ||
|
||
if result['ok'] != 1: | ||
module.fail_json(msg='rolesInfo failed', exception=traceback.format_exc()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message is not very informative. Maybe there's some data you'd like to add to the returned result?
roles=roles | ||
) | ||
|
||
if result['ok'] != 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this is also very boilerplate, which you could deduplicate.
and sorted(current['privileges']) == sorted(privileges) \ | ||
and sorted(current_restrictions) == sorted(authentication_restrictions) | ||
else: | ||
return sorted(current['roles']) == sorted(roles) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This thing is also repeated.
|
||
def main(): | ||
module = AnsibleModule( | ||
argument_spec=dict( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rewrite dict creation using literals (with curly braces)
login_user=dict(default=None), | ||
login_password=dict(default=None, no_log=True), | ||
login_host=dict(default='localhost'), | ||
login_port=dict(default='27017'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use type='int'
here and you won't have to do manual conversion or checking manually
@jkramarz Hi, would you be able to take a look at the suggestions above? |
Hello |
@@ -141,6 +141,7 @@ files: | |||
$modules/crypto/: Spredzy | |||
$modules/crypto/acme/: resmo felixfontein | |||
$modules/database/influxdb/: kamsz | |||
$modules/database/mongodb/mongodb_role.py: jkramarz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed as you are listed in the module's author:
list
short_description: Adds or removes a role from a MongoDB database. | ||
description: | ||
- Adds or removes a role from a MongoDB database. | ||
version_added: "2.8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version_added: "2.8" | |
version_added: `2.10` |
@jkramarz Hi, the mongodb modules have moved to https://github.com/ansible-collections/community.mongodb could you please raise the PR there |
@emanuelflp if the PR doesn't exist in the new repo yet, then yes, please proceed. Thanks! |
SUMMARY
mongodb_role
module introduced in this pull request provides management capabilities for user-defined roles in MongoDB, enabling users to define custom privileges.ISSUE TYPE
COMPONENT NAME
mongodb_role
ANSIBLE VERSION