Add ovirt role #54600
Add ovirt role #54600
Conversation
ansibot
added
affects_2.8
|
Please add integration tests, even if they cannot be run in CI, at least it gives confidence that various aspects have been verified (idempotency, check-mode, etc...). It also helps to attest certain use-cases still work when changes are made in the future... |
This comment has been minimized.
This comment has been minimized.
ansibot
added
ci_verified
This comment has been minimized.
This comment has been minimized.
ansibot
removed
the
ci_verified
This comment has been minimized.
This comment has been minimized.
ansibot
removed
the
ci_verified
|
@KKoukiou @derez @machacekondra @mwperina @nasx As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add |
ansibot
removed
the
needs_revision
| - "Should the role be present/absent." | ||
| choices: ['present', 'absent'] | ||
| default: present | ||
| mutable: |
There was a problem hiding this comment.
I think you can't create imutable role, and all roles created by users are mutable, so this parameter is not needed.
| description: | ||
| - "List of permits which role will have" | ||
| - "Permit 'login' is default and all roles will have it." | ||
| - "Dictionary can contain following value." |
There was a problem hiding this comment.
It's not dicitinonary. It's list of permit names.
| name=dict(default=None), | ||
| mutable=dict(default=True, type='bool'), | ||
| administrative=dict(default=False, type='bool'), | ||
| permits=dict(type='list', default=[]), |
There was a problem hiding this comment.
Hmm, I would not use default [], because if you update only description for example of the role it would remove all pertmis I ithink
There was a problem hiding this comment.
Has this comment regarding default been resolved?
| - "List of permits which role will have" | ||
| - "Permit 'login' is default and all roles will have it." | ||
| - "Dictionary can contain following value." | ||
| - "C(name) - Name of permit." |
There was a problem hiding this comment.
You miss description parameter of the role.
| ) | ||
| state = module.params['state'] | ||
| if state == 'present': | ||
| module.params.get('permits').append('login') |
There was a problem hiding this comment.
I just wonder what will happen if user specify login and you append login, maybe it will fail on engine side? Can you please use set and cast it to list?
| administrative: true | ||
| permits: | ||
| - manipulate_permissions | ||
| - create_instance |
There was a problem hiding this comment.
An example of removing role would be nice.
This comment has been minimized.
This comment has been minimized.
ansibot
added
ci_verified
ansibot
removed
the
ci_verified
This comment has been minimized.
This comment has been minimized.
ansibot
added
community_review
|
+1 |
ansibot
added
shipit
|
Thanks all |
SUMMARY
Add ovirt role
Fixes #54199
ISSUE TYPE
COMPONENT NAME
ovirt
ADDITIONAL INFORMATION