Worteks / ansible-lemonldapng

LemonLDAP-NG Ansible role
MIT License
6 stars 4 forks source link

using shell module is a bad idea #1

Closed l00ptr closed 5 years ago

l00ptr commented 5 years ago

Using (or almost) exclusively the shell ansible module is a bad idea. We can replace some of them to respect the ansible best practices.

By example instead of using shell module to import a GPG key for debian/apt we can use the apt_key module:

- name: Install LLNG Repository KEY
  copy: src=rpm-gpg-key-ow2 dest=/tmp/gpg-key-ow2 owner=root group=root mode=0644
- name: Check for Missing Trusted Key
  changed_when: "has_apt_key.rc != 0"
  failed_when: false
  register: has_apt_key
  shell: apt-key list | sed 's| ||g' | grep -i {{ lemonldap_gpg_pubkey_id }}
- name: Import LLNG GPG KEY
  shell: apt-key add gpg-key-ow2 chdir=/tmp
when: has_apt_key is defined and has_apt_key.rc is defined and has_apt_key.rc != 0

Can become:

- name: Install LLNG Repository KEY
  copy: 
    src: rpm-gpg-key-ow2 
    dest: /tmp/gpg-key-ow2 
    owner: root 
    group: root 
    mode: 0644

- name: Import LLNG GPG KEY
  apt_key:
    file: /tmp/gpg-key-ow2
    state: present

It's easier to read and more in the ansible spirit :)

coudot commented 5 years ago

Hello @l00ptr,

thanks a lot for reporting this issue! You are right, we can improve this project by using ansible modules like you suggest.

If you want to propose a patch and contribute, feel free to send a pull request.

l00ptr commented 5 years ago

Hello,

I will propose a patch :-) it's still a work in progress but if you want you can check there:

I still have to improve the part related to Apache configuration (replacing example.org and symlinking the lemonldap conf). I don't know what you except from a contribution. PR from master to master or from another branch to the master branch ?! Do you have any guideline ?

J.

l00ptr commented 5 years ago

And I don't know which version of Ansible you want to support, there are a lot improvement into the latest version of ansible. The latest one are not easy to install it by just using the default repo of Debian Stable or RedHat like (CentOS) distribution.

coudot commented 5 years ago

@l00ptr I can't answer myself, I let the other give their opinion

maxbes commented 5 years ago

@l00ptr please request your contribution to be merged into our master branch, and make sure it will run on Ansible 2.5

maxbes commented 5 years ago

Fixed with #3