search

mongrelion / ansible-role-docker

Ansible role for installing Docker
MIT License
62 stars 39 forks source link

integration tests #30

Closed paulfantom closed 6 years ago

paulfantom commented 6 years ago

Resolves #13

I've started work towards providing integration tests, currently it works on:

And doesn't work on:

Problems aren't directly related to integration test suite, but to solutions used in this role (like running installation script instead of porting script to ansible tasks).

I'm currently commenting out not working environments.

paulfantom commented 6 years ago

@mongrelion please check travis output.

I can start work towards integrating some solutions from https://github.com/SoInteractive/ansible-docker to make ansible-lint happy and remove installation script.

mongrelion commented 6 years ago

I'm happy to ditch the installation script and replace that with ansible tasks in exchange for a solid test suite.

Keep up the good work!

paulfantom commented 6 years ago

Some questions: 1) Can I replace service module with newer systemd module? 2) This (https://github.com/mongrelion/ansible-role-docker/blob/master/tasks/main.yml#L76) is not used anywhere, remove? 3) This is very static usage of consul: https://github.com/mongrelion/ansible-role-docker/blob/master/tasks/main.yml#L29 probably should be removed or made more dynamic, what do you think?

I think there is a need for a major rework to unify variable naming (variables with role name prefix are easier to read in playbooks) and move from installation script to ansible task. Also I can create some variable mappings for backward compatibility. But before I start I need to know your point of view on this matter.

mongrelion commented 6 years ago
  1. If that doesn't break compatibility between the different distros that we support, then go ahead
  2. It was used for running tests in Vagrant but since you're moving to Molecule, please remove it
  3. This was required back in the days (lol, last year) when support for overlay networks required consul or etcd. This is not required anymore so it can go.

Regarding the unification of variable naming, I agree but I'm afraid it may fall a little out of the scope of this PR so let's keep it nice and tidy and then we can work on the next big thing. This rework is looking promising.

Thanks again for looking into this!

paulfantom commented 6 years ago

Ready to be merged.

Currently it is testing on:

Distros with SELinux aren't tested since installation script uses setsebool command which fails in docker containers due to SELinux beeing in disabled state.

Debian Stretch fails because something fails in the installtion script.

paulfantom commented 6 years ago

I am also creating issues so work can be started on other things I discovered during writing this PR

mongrelion commented 6 years ago

Awesome work, man. Thanks for this.