-
Notifications
You must be signed in to change notification settings - Fork 741
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
style update #134
style update #134
Conversation
rndmh3ro
commented
Aug 4, 2017
- update task names and documentation to include baseline controls
- use single ticks instead of double ticks
- use native YAML syntax
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.
Very nice work! I agree with almost all changes and appreciate the switch to the regular YAML syntax over the custom Ansible thingy. I just added a few comments.
Keep in mind that I don’t currently use the role so my review is not as thoughtful as if I where using it.
tasks/apt.yml
Outdated
- "{{os_security_packages_list}}" | ||
when: os_security_packages_clean | ||
- '{{os_security_packages_list}}' | ||
when: 'os_security_packages_clean' |
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.
os_security_packages_clean
is supposed to be a boolean. You can ensure this with the bool
filter :)
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.
Fixed that!
tasks/pam.yml
Outdated
@@ -7,55 +7,96 @@ | |||
DEBIAN_FRONTEND: noninteractive | |||
|
|||
- name: remove pam ccreds on Debian systems | |||
apt: name='{{os_packages_pam_ccreds}}' state=absent | |||
apt: | |||
name: '{{os_packages_pam_ccreds}}' |
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 find it easier to read when spaces are added like this:
name: '{{ os_packages_pam_ccreds }}'
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're totally right. Sed did the trick:
sed -i 's/\(\S\)}}/\1 }}/g' templates/*
sed -i 's/{{\(\S\)/{{ \1/g' tasks/*
tasks/user_accounts.yml
Outdated
shell: awk '/^\s*UID_MIN\s*([0-9]*).*?$/ {print $2}' /etc/login.defs removes=/etc/login.defs | ||
shell: awk '/^\s*UID_MIN\s*([0-9]*).*?$/ {print $2}' /etc/login.defs | ||
args: | ||
removes: /etc/login.defs | ||
register: uid_min | ||
check_mode: no |
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 would prefer check_mode: False
. There are good reasons to avoid yes
and no
values in YAML because they look like strings but might be/are evaluated booleans :)
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.
Fixed!
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.
LGTM, thanks :)
force /bin/sh when getting openssh-version
style update
force /bin/sh when getting openssh-version