-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
VMware: Improve module vmware_local_user_facts #47868
Conversation
Hi @ckotte, thank you for submitting this pull-request! |
@ckotte, just so you are aware we have a dedicated Working Group for vmware. |
@@ -50,56 +52,108 @@ | |||
type: dict | |||
sample: [ | |||
{ | |||
"full_name": "Administrator", |
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.
We are changing facts keys here, we should add older values as well as new values. Because, removing old values will break playbooks and roles which are already written by user of this modules.
Ideally we should
- Add a note in notes section stating these values will be deprecated after 4 versions.
- Add a porting guide
Let me know if it makes sense.
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.
Isn't this a bit overkill? You couldn't assign roles to users before #47866. Anyway. How can you combine two lists without messing the whole output?
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.
Yes, I understand it is a bit overkill, but consider from UX point of view. People already using this module will be surprised to see failing PB which were already working.
You can keep -
{
"full_name": "DCUI User",
"principal": "dcui",
"user_group": false,
"role": "admin",
"description": "DCUI User",
"group": false,
"user_id": 100,
"user_shell_access": false
"user_name": "dcui",
"shell_access": false
},
I aware that information is redundant but that will atleast not break playbooks. We will add porting guide note here and a note in documentation stating that these values will be deprecated after 4 release i.e. 2.12
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.
I have added similar note in porting guide for vmware_local_role_facts
here = https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/porting_guides/porting_guide_2.8.rst#id12
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.
ok. I thought this is the PR with the dict to list change. I added the old names and notes.
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.
What do you want to write in the port guide? I think it's sufficient if we add a note later in 2.12 that the properties are deprecated.
13840ab
to
65d1846
Compare
* add check mode support * add assigned role to the output * change output to match vmware_local_user_manager output * change principal to user_name * change full_name to user_description
65d1846
to
201ed45
Compare
@ckotte Thanks for the contribution. |
* add check mode support * add assigned role to the output * change output to match vmware_local_user_manager output * change principal to user_name * change full_name to user_description
* add check mode support * add assigned role to the output * change output to match vmware_local_user_manager output * change principal to user_name * change full_name to user_description
SUMMARY
principal
touser_name
full_name
touser_description
user_group
togroup
user_description
todescription
ISSUE TYPE
COMPONENT NAME
vmware_local_user_facts
ANSIBLE VERSION
ADDITIONAL INFORMATION