nickjj / ansible-docker

Install / Configure Docker and Docker Compose using Ansible.
MIT License
750 stars 224 forks source link

replace reload systemd daemon task by a handler #115

Closed coute closed 2 years ago

coute commented 3 years ago

Hi @nickjj , When I checked a playbook using your role with ansible-lint, i had an error :

no-handler: Tasks that run when changed should likely be handlers
../../.cache/ansible-lint/198db2/roles/nickjj.docker/tasks/main.yml:126 Task/Handler: Reload systemd daemon

To fix this message i change the task by a handler.

coute commented 3 years ago

Better way, use only one handler and ask ansible to reload systemd daemon before restarting docker https://docs.ansible.com/ansible/latest/collections/ansible/builtin/systemd_module.html

nickjj commented 3 years ago

Hi,

Thanks for the PR.

I feel like I did this in the past once but something didn't work, only problem was this was years ago and I forgot the details haha.

Did you happen to test this on a real server?

coute commented 3 years ago

I had an env variable in group_vars :

docker__daemon_environment:
  - "ENV_VAR=true"

And run a playbook :

TASK [nickjj.docker : Configure Docker daemon environment variables] ***********
changed: [dev1]

TASK [nickjj.docker : Configure custom systemd unit file override] *************
skipping: [dev1]

TASK [nickjj.docker : Restart Docker now to make sure `docker login` works] ****

RUNNING HANDLER [nickjj.docker : Restart Docker] *******************************
changed: [dev1]

system log :

Sep 23 18:32:27 dev1 systemd[1]: Reloading.
Sep 23 18:32:27 dev1 systemd[1]: Stopping Docker Application Container Engine...

Seems to be ok.

nickjj commented 3 years ago

Would you mind giving it a while while supplying Docker registry credentials, then verify if you can SSH into the box and pull something from a private registry? You don't need to set up any private repos because the error message by Docker will hint that the repo doesn't exist when your login credentials are valid vs Docker saying access denied if your credentials are wrong.

coute commented 3 years ago
coute@dev1:~/$ docker-compose pull stop_detector
Pulling stop_detector ... done

It's fine.

coute commented 3 years ago

But I found an other issue. I removed docker__daemon_environment from my group_vars file and the environment file in systemd remain the same. Template is not updated. No change in playbook.

coute commented 3 years ago

It's due to

   when: docker__daemon_environment | d()

If the variable is not set the task is skipped.

Adding a task that remove the file when it exists and the variable is not set should fix that.

nickjj commented 3 years ago

Oh yeah, I have a feeling that's going to be a problem with any of the systemd config files (all 3 of them). We have 2 options there, either add a delete task for each file while having a when: not docker__daemon_environment | d() or remove that when condition in the existing tasks and let Ansible create an empty config if it's empty.

coute commented 3 years ago

Something like that will work. Need to be test.

nickjj commented 2 years ago

Hi,

Sorry for not getting back to you in about a year.

A variant of some of your ideas were included in https://github.com/nickjj/ansible-docker/commit/327934c9538224cb3234aec692705bf271cecd80 and https://github.com/nickjj/ansible-docker/commit/c2a80feb18e19339f95b2afb759a544bb91ffe94. I gave you credit in the commit. I committed them directly because this PR has been idle for a while and to be fully transparent I wanted to get this release out for Docker Compose v2 support and I wasn't sure how long it would have taken to break out your commits into separate PRs.

These commits are available in v2.2.0 of this role.

Thanks again!