Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove trusted.gpg.d artifacts. #461

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

danrough
Copy link
Contributor

Fixes #460

#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, #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
Copy link
Contributor Author

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, ansible-collections/community.docker#860

@geerlingguy
Copy link
Owner

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
Copy link
Contributor

kawadeomkar commented Jun 8, 2024

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?

state: absent
filename: "{{ docker_apt_filename }}"
update_cache: true
when: docker_add_repo | bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, but do we need a bool type cast here if the var docker_add_repo is defined as a boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following the convention used elsewhere in this particular file, and elsewhere in the role - happy to change it if it would stop you merging this change.

@cschindlbeck
Copy link
Contributor

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
Copy link
Contributor Author

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

@danrough danrough force-pushed the fix/apt-key-duplication branch from 7ab5e28 to f666736 Compare July 24, 2024 10:04
@danrough
Copy link
Contributor Author

Updated, @cschindlbeck 😄

@danrough
Copy link
Contributor Author

@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.

@jkossis
Copy link

jkossis commented Aug 27, 2024

Any update on getting this merged in?

@danrough
Copy link
Contributor Author

🤷‍♂️ I was hoping that it would have been merged in by now too.

@geerlingguy geerlingguy merged commit ae17f06 into geerlingguy:master Aug 27, 2024
7 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
5 participants