ikke-t / podman-container-systemd

creates systemd files and creates containers using podman
118 stars 44 forks source link

Use podman_image to pull images and add authn #12

Closed cfelder closed 3 years ago

ikke-t commented 4 years ago

Hi, thanks for this PR, looks nice. Might I ask you additional favor? As now the first image pull is there to update an existing images, if e.g. :latest is used. But it never worked at the time I wrote this, as podman didn't return any info if it actually updated the image or not. Does this new tool now return changed if image gets updated?

And if it does, we should kick restart for the container so that it really updates the running instance as well. This has always been on todo list, but I haven't got around to do it. Are you aware of the fact, and any chance you could include it here?

cfelder commented 4 years ago

@ikke-t that sounds reasonable and if it reports change it's simply one register and changing the conditional. I'll investigate. Meanwhile I just rebased on current master.

Best Christian

cfelder commented 4 years ago

I've just checked the implementation of containers.podman.podman_image.

They just do a basic lookup based on the image name to check whether the image exists or not. To be compatible to the existing implementation of podman-container-systemd I am using force: yes here which will guarantee to force pull for any updates.

In respect to idempotency the approach to explicitly define force pull looks reasonable to me.

ikke-t commented 4 years ago

So there is 1) no way to get info if the image pulled is newer than it was before the pull, using the module? If so, perhaps it should be then 2) done via image inspect, and looking for some id to see if the image changed or remained the same. But that would be outside of this PR then. Tell me if 1 is not possible, and let's just merge this.

Thanks for your work!

cfelder commented 4 years ago

At least it does

self.results['changed'] = digest_before != digest_after

We have to force pull but can register a variable to check whether the image has been changed though.

I'll prepare a patch

cfelder commented 4 years ago

I am uncertain my last commit about adding the handler is correct.

cfelder commented 4 years ago

I've the following issue with the handler (using my last commit here):

RUNNING HANDLER [podman_container_systemd : restart service] ********************************************************************************************************************************************************************************************************************************************************************************
fatal: [localc8f]: FAILED! => {"msg": "The task includes an option with an undefined variable. The error was: 'nginx_container_name' is undefined\n\nThe error appears to be in '/Users/felder/.ansible/roles/podman-container-systemd/handlers/main.yml': line 12, column 3, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n- name: restart service\n  ^ here\n"}

when using your role in a playbook defining container_name by another variable:

- name: create nginx container
  vars:
    container_name: "{{ nginx_container_name }}"
    ...
  include_role:
    name: podman_container_systemd
ikke-t commented 4 years ago

did you find a solution for this? I don't see what's wrong there worth the complain.

cfelder commented 4 years ago

I did not yet investigate thoroughly. I've to dig around a bit more why c1c5bbc leads to this problem not recognising the variable.

cfelder commented 4 years ago

I have to admit that I am not that familiar with handlers in ansible. But as far as I understood handlers in ansible are created early on which can lead to problems when using variables. For some reason the evaluation of service_name fails in the handler, e.g.

- name: restart service
  systemd:
    name: "{{ service_name }}"
    state: restarted

when using a variable for container_name as mentioned in my previous post:

- name: create nginx container
  vars:
    container_name: "{{ nginx_container_name }}"
    ...
  include_role:
    name: podman_container_systemd

whereas the following snippet works fine:

- name: create nginx container
  vars:
    container_name: "nginx_container_name"
    ...
  include_role:
    name: podman_container_systemd
cfelder commented 4 years ago

I am using several nested include_roles. Using public: yes on the first role resolves the issue.

This may be related to https://github.com/ansible/ansible/issues/66553

but is for sure no issue with this PR.

fine to merge for me now