Andrew Huffman - PR changes #8
Conversation
|
Awesome, thank you for submitting this! I noticed a few things that should be addressed. I'll submit a comment for each of them. |
| @@ -1,6 +1,7 @@ | |||
| The MIT License (MIT) | |||
|
|
|||
| Copyright (c) 2016 Tyler Cross | |||
| Copyright (c) 2016 Andrew Huffman | |||
| > An Ansible role for comprehensive management of a sudoers configuration. | ||
|
|
||
| [](https://travis-ci.org/wtcross/ansible-sudoers) | ||
| # ahuffman.sudoers |
There was a problem hiding this comment.
I'd like to give you contributor access to this repo. That said, I think this'll still need to be changed to wtcross.sudoers.
There was a problem hiding this comment.
Changed back to wtcross.sudoers in latest commit.
| - specifications | ||
|
|
||
| *Tip:* Here's a [great document about sudoers configuration](https://help.ubuntu.com/community/Sudoers) |
There was a problem hiding this comment.
Is there a reason you removed this note? I don't care about the source, but wanted the README in this role to point to some comprehensive sudoers guide. Open to suggestions on an alternative.
There was a problem hiding this comment.
I was assuming users of this role would have experience manipulating /etc/sudoers, but I have no problem adding it back in. A Fedora/CentOS resource might be better in the future ;). Added back in latest commit.
|
|
||
| ## License | ||
| [MIT](LICENSE) | ||
|
|
||
| ## Author Information | ||
| ### Original Author: |
There was a problem hiding this comment.
Rather than specifying original author, how about just listing each of us as an author?
There was a problem hiding this comment.
Changed to include us both in latest commit.
| @@ -1,10 +1,11 @@ | |||
| galaxy_info: | |||
| author: Tyler Cross | |||
| description: | |||
| issue_tracker_url: https://github.com/wtcross/ansible-sudoers/issues | |||
There was a problem hiding this comment.
If the code is going to live in this repo, can we just keep this url?
| @@ -4,20 +4,20 @@ | |||
| name: sudo | |||
| update_cache: yes | |||
| state: present | |||
| when: "{{ ansible_os_family == 'RedHat' }}" | |||
| when: ansible_os_family == 'RedHat' | |||
There was a problem hiding this comment.
In order to make this following best practices can you wrap it with template tags?
There was a problem hiding this comment.
Ex: "{{ ansible_os_family == 'RedHat' }}"
There was a problem hiding this comment.
Otherwise Ansible will throw a warning when running the playbook.
There was a problem hiding this comment.
See the porting guide and look for bare variables: https://docs.ansible.com/ansible/porting_guide_2.0.html
|
|
||
| - name: Ensure sudo is installed for Debian family OSs | ||
| apt: | ||
| name: sudo | ||
| update_cache: yes | ||
| state: present | ||
| when: "{{ ansible_os_family == 'Debian' }}" | ||
| when: ansible_os_family == 'Debian' |
There was a problem hiding this comment.
In order to make this following best practices can you wrap it with template tags?
|
|
||
| - name: Ensure sudo is installed for SUSE family OSs | ||
| zypper: | ||
| name: sudo | ||
| state: present | ||
| when: "{{ ansible_os_family == 'SUSE' }}" | ||
| when: ansible_os_family == 'SUSE' |
There was a problem hiding this comment.
In order to make this following best practices can you wrap it with template tags?
|
Thanks for the quick responses! Merging now. |
Some changes I made a few months back, as we discussed today.