Some cleanups, clearer syntax, refactor variables, drop selinux and epel (let other roles do that)

[fpeterschmitt]

No description provided.

[jdauphant]

Hi @leryan ,
I see we share common interest in FreeBSD :)
Thanks for pull request.
My remarks :

• There is good ideas in this PR
• There is lot of modification for one PR
• Compatibility will be broken with old versions
• I personnally don't like using "include_vars", it's break tags filtering "--tags" and "--start-at-tasks" options
• You have probably reason for EPEL, but my role try to be independent as possible: if without EPEL, you cannot install nginx, the role can manage ( but we can do nothing if EPEL is already configurated)

It will take me more time to make a better review and analyze side effect.

Best regards,
Julien

[fpeterschmitt]

Thanks for review :)

Let me ask you some questions:

• What do you mean with include_vars breaking --tags and --start-at-tasks?
• Breaks compatibility with older versions of Ansible, right?

For EPEL, that's a different point of view, mine is that no ansible role should configure package manager instead of another role dedicated to that job.

Of course, if you want, I can rollback changes you don't want to see here and make a proper version fitting your needs & preferences. Just tell me by adding comments in the patches, for example.

[jdauphant]

Include_vars

I talk about this part :

- include_vars: "{{ ansible_os_family }}.yml"

My remarks :

• If you don't tags this task with all tags used in this role, when you use --tags options to filter what you want to do : some tasks will fail because include_vars was not executed
• If you use "--start-at-task" with a task of this role, include_vars (the first task of the role) will not be executed and the role will failed when variables will be missing

I know this is indicated in the best practice but I don't like break these options.

Compatibility

Breaks compatibility with older versions of this role

EPEL

"For EPEL, that's a different point of view, mine is that no ansible role should configure package manager instead of another role dedicated to that job."
I understand that.
I am not familliar with RedHat system like, so I have a question :

• Can I install nginx with a default installation of Centos/RHEL with "yum install nginx" ?
  • If the answer is yes, we should remove this EPEL thing immediatly
  • If the answer is no, this role need to provide a solution (EPEL or not) to install correctly a Centos/RHEL managed version without depends of another role (we can reduce conflit between another role that manage EPEL by checking if EPEL is already configured)

Thanks,
Best regards,
Julien

[jdauphant]

I will comments the code after your answer to be sure to have all elements in hand

[fpeterschmitt]

include_vars

For --tags option, it runs with an extra tag always, but it doesn't runs with --start-at-task :(

Compatibility

Is it really a problem? Things are made to be changed :P

EPEL

It's a Ooops for me, there is no nginx provided in default repositories, at least in CentOS. So here I should rollback some of the code I deleted that manage official vs EPEL repositories.

Personally I use this role to configure EPEL: https://github.com/geerlingguy/ansible-role-repo-epel

Tell me what behavior you want and I'll implement it.

[jdauphant]

include_vars

We will have to always use the "always" tag when we use --tags option :(
I prefer not to.

Compatibility

Yes, we could made a version 2.0 if some broke compatibility :) I have other things that break compatibility to change (it's a more powerfull syntax change :) :

EPEL

Let's keep EPEL. We could improve the role to limit ou remove conflicts with role like : https://github.com/geerlingguy/ansible-role-repo-epel (maybe there is no conflicts with the actual solution)

[fpeterschmitt]

include_vars

No, the special always tag make the task to always run, even if the tags is not specified. I tested it (with ansible 1.9.1).

EPEL

So let's keep it :) We will simply explicitly offer the ability to disable EPEL and so let other roles do it for us. Then it will be up to the guy writing his playbook to manage the whole things.

Let's do a summary:

include_vars: --start-at-task will be broken… and that's somewhat bad.

compatibility: no problem if you want to merge later/much later.

EPEL: keep it, offer the ability to disable it. If i remember well, the current role make already this with nginx_is_el. I just found it to be a bit tricky in the tasks ;-) So it will only need a rollback on this file, almost.

[jdauphant]

Thanks for the "always" tag tips :)

Summary is good.
We can also improve the "nginx_is_el" by rename it to something more clear like "nginx_use_epel"

[kosssi]

add newline ;)