Add Molecule tests, change handlers

[niluid]

Hi @evrardjp ,
thank you for creating this role. It's really helpful.

My initial purpose was propose the PR for replace the synchronize module here with copy - to make this role do not require passwordless sudo.

But beforehand I make the current PR with proposal to update your role by Molecule tests. AFAIK currently you have the automatic syntax check - and I want to propose use Molecule for additional tests. It may check the syntax, run the role simultaneously in several OS, verify idempotency - and finally ensure that each prepared node able to ping other nodes over VPN.

I would be appreciated if you'll find a time to review this idea and related changes proposed to variables, tasks and handlers. Hope that next explanations will be helpful.

Updated .travis.yml will run molecule command two times and verify:

• default scenario for ring topology
• the star topology as well

Both scenarios described in corresponding subdirectories

We may simply rename default to the ring - however the currently proposed naming convention allow anybody just execute well-known command molecule test in repository root - and this will be enough to run basic test, without necessity additionally check documentation.

star scenario re-use the 2/3 playbooks, so the duplicated code defined as softlinks from molecule/star to molecule/default

Difference in:

• default/molecule.yml
• star/molecule.yml

Files are similar except the variables scenario.name and groups

Current config will spin-up 5 Docker containers, apply the role and execute simple check defined in verify.yml

I'm not going to recommend run the Tinc VPN on different OSs with different package versions in production - but this might be ok for testing purposes I think.

Docker give some additional specific:

• code privileged: true with command: /sbin/init in molecule.yml enable Systemd on CentOS container. Although Ubuntu here doesn't use Systemd (if necessary, this could be changed by using some other specially prepared Ubuntu images, I think)
• Docker images lack some standard software, so converge.yml take care about installing necessary dependencies
• Docker issue modifying /etc/hosts inside container ansible/molecule#959 say that Docker doesn't let us modify /etc/hosts in a container. To workaround this I added molecule-notest tag to this task and instead /etc/hosts will be modified during container creation - as configured by etc_hosts directives in molecule.yml

After failing first tests I found that on CentOS containers Tinc additionally required manual execution of systemctl restart tinc. Maybe I missed something in corresponding conversation but my assumption was that currently these handlers may doesn't work as expected. I think, that on systems with Systemd this code may trigger only systemd daemon-reload and Ansible may not restart the service afterwise. Not sure, maybe it's depends on Ansible version through.

Instead I want propose updated handlers which will be triggered by this code. I have to agree that two-rows call is not ideal and would be great if somebody will propose something more laconic.

As I added enabled: yes to service description in handlers - this PR propose to remove last two tasks from tinc_configure.yml
I tried to imagine scenarios when we the settings in handlers will not be enough - and I can't propose anything. Please correct if I'm wrong.

Updated defaults/main.yml:

• remove the variable service_name (I found it only in handlers and replaced with existing tinc_service_name)
• define defaults for tinc_netname and tinc_control_plane_bind_ip - they required during Molecule execution.

This PR remove the tests folder used by original .travis.yml

This PR update meta/main.yml - after passing the tests I've extended the list of supported Ubuntu/CentOS systems in this file.

Thanks again for your time.
Looking forward for your feedback.

[evrardjp]

OMG this is awesome! :)
I didn't dig into your proposal yet as I am busy with other work today/tomorrow, but will evaluate it as soon as possible, latest Friday.

[niluid]

Thanks for confirmation about taking this request into consideration.
Please take your time - this issue is not a blocker for me.
Glad to see that it might be helpful.

[evrardjp]

I like the idea so far. I have reviewed with my eyes, but not yet tested with molecule. I think this is a stepping stone for this role, as it will allow us to test better, so thank you for that!

If you could propose an update to this PR to separate the commits into functional parts I would be very happy :)

[evrardjp]

Side note for the context: This was created when a bug to the module "service" wouldn't allow to restart the service, enable it, and daemon_reload at the same time.

As you're cleaning that up (I suppose that bug is fixed:) ), I think we can come back to the initial "Restart Service" handler. If we change the handler name, it would basically change the user interface: the handlers is something exposed to the users of the role. In order to not change the user interface, could you keep the words "Restart Service" for the handler name?

Also, I am not sure we need two tasks if we clean this up, the ternary operator to omit daemon_reload should be good enough, what do you think?

Next to all of this, I think it would be nice if it was done in a separate commit. This way we keep the history clean of what does each of those: One is tackling molecule, one is changing the service (in case we ever need to revert, it's easier to have a clean scope)

[evrardjp]

isn't this risky, as not everyone has a nic named eth0?
Instead I would use the following as default: "{{ ansible_default_ipv4.address }}"

Also, can this be done in a separate commit? I would like to see just the minimum required for molecule first :)

[evrardjp]

For me this can be also a separate patch to show new compatibility, that can be done after the 'main' molecule patch.

[evrardjp]

see my comment on the simplification above, and the fact to add it in a different commit. I like where you're heading to, though :)

[niluid]

Hi. Thank you for feedback @evrardjp

I made mistake by summarizing all these changes into one commit. Now I'm not sure if it possible break them back into several parts.

If you don't mind, I would propose close this PR as rejected. Instead I'll propose the other one, which will add only Molecule tests with several vars in defaults/main.

What do you think?

[evrardjp]

That sound good, let's do it step by step. The minimum to include molecule would be the first step, second would be the new features. That's awesome @niluid , thanks for the work.

[evrardjp]

Is there any way I can help? Should I build the molecule support with you?

[niluid]

Thank you @evrardjp
I glad to see tests added in #52

Now we need have these tests return correct 'green' status. As was agreed, this may require some improvements in handlers.

I going to re-check current behavior in Docker and VMs with Ansible 2.9 and 2.10. I'll submit my findings during a week. This probably will be formulated as issue with the list of proposed solutions - to discuss the best approach and then propose PR.

See you later!

[evrardjp]

Thanks! I have a PoC locally, workarounding the issue. I can probably issue a PR with workaround AND ideal state. Would you be okay to review it ?

[niluid]

Sure! I'll review it and response on first opportunity, during a day.
Thank you.