geerlingguy / ansible-role-docker

Ansible Role - Docker
https://galaxy.ansible.com/geerlingguy/docker/
MIT License
1.8k stars 851 forks source link

Remove trusted.gpg.d artifacts. #461

Open danrough opened 2 months ago

danrough commented 2 months ago

Fixes https://github.com/geerlingguy/ansible-role-docker/issues/460

https://github.com/geerlingguy/ansible-role-docker/pull/436 introduced a change to the path where the apt key was placed, and to the associated repository listing in /etc/apt/sources.d/docker.list. We've just run into a problem with this approach, as detailed in the issue I raised, https://github.com/geerlingguy/ansible-role-docker/issues/460.

So that other people who use the role without pinning a specific version don't run into this same problem, I thought it would be wise to add a couple of tasks which remove any artifacts associated with the previous approach to storing repository keys.

I'm sorry to ping you both, @kawadeomkar / @geerlingguy, but I wonder if you would give this some consideration?

danrough commented 2 months ago
failed: [localhost] (item=instance) => {"ansible_job_id": "j73025223298.1850", "ansible_loop_var": "item", "attempts": 2, "changed": false, "finished": 1, "item": {"ansible_job_id": "j73025223298.1850", "ansible_loop_var": "item", "changed": true, "failed": 0, "finished": 0, "item": {"cgroupns_mode": "host", "command": "", "image": "geerlingguy/docker-ubuntu2004-ansible:latest", "name": "instance", "pre_build_image": true, "privileged": true, "volumes": ["/sys/fs/cgroup:/sys/fs/cgroup:rw"]}, "results_file": "/home/runner/.ansible_async/j73025223298.1850", "started": 1}, "msg": "Error connecting: Error while fetching server API version: Not supported URL scheme http+docker", "results_file": "/home/runner/.ansible_async/j73025223298.1850", "started": 1, "stderr": "", "stderr_lines": [], "stdout": "", "stdout_lines": []}

I encountered this the other day in my own environment, I think it's this, https://github.com/ansible-collections/community.docker/issues/860

geerlingguy commented 2 months ago

Yeah, that's an annoying bug that won't be fixed easily until the next release (I don't want to pin a release on all my hundreds of projects!).

If someone else can validate this fix I'm okay merging.

kawadeomkar commented 2 months ago

Good find, I believe this is an issue that some people may come across when upgrading from a previous version to a version after 7.0.0 on the same machine.

I've validated the changes with the following approach: Using Vagrant, run the role on a version <7, then re-run the role after the changes on #436 on a version >=7. This seems to work for me - though I'm not sure if this removes the complete set of vestigial artifacts.

I previously had an issue where I had to remove the following:

/etc/apt/sources.list.d/download_docker_com_linux_ubuntu.list
/etc/apt/sources.list.d/docker.list

that was created on version <7 before running the changes in #436

@danrough Did you have a similar issue? Should we consider removing the files above as well?

cschindlbeck commented 1 month ago

This duplicate key error is annoying af, so I am reviewing this:

1) Can you rebase the branch such that CI passes? 2) The two new tasks that you included must be moved to the top of the file, otherwise i get this error:

TASK [geerlingguy.docker : Ensure old versions of Docker are not installed.] **************************************************************
fatal: [localhost]: FAILED! => {"changed": false, "msg": "E:Conflicting values set for option Signed-By regarding source https://download.docker.com/linux/ubuntu/ jammy: /etc/apt/keyrings/docker.asc != /etc/apt/trusted.gpg.d/docker.asc, E:The list of sources could not be read."}

After moveing the tasks to the top, it works, so thanks @danrough 👍

danrough commented 1 month ago

I'll get to this within the next couple of hours, @cschindlbeck. Thank you for the feedback :)

danrough commented 1 month ago

Updated, @cschindlbeck 😄

danrough commented 1 month ago

@danrough Did you have a similar issue? Should we consider removing the files above as well?

@kawadeomkar I'll be honest and say that I can't remember - I tested this on Debian and I don't think there were any files remaining though. I've just had a look on a couple of our servers and the files aren't there. Whether that's owing to this role or another reason I don't know.

Let me know if there's something that you'd like me to add to this change before you'd merge it.