ansible-role-bootstrap icon indicating copy to clipboard operation
ansible-role-bootstrap copied to clipboard

fix: warm up package cache if Python is already installed

Open philippwaller opened this issue 3 years ago • 6 comments

Description

This bugfix will preheat the package cache if python is already installed and the native Ansible functions are used to install the bootstrap packages. This pull request will fix the problems described in issue #56.

Testing To reliably test these changes, new/additional containers with python installed are required. However, before implementation, we should consider whether it is possible to simplify this role.

philippwaller avatar Jan 04 '22 19:01 philippwaller

Great idea! This solution is much smarter. We only need an exception for the Portage module as it uses a different API.

The "changed_when" attribute is required in some cases to pass the idempotency test. The apk module for example, always returns "changed" when a package update is performed. We can either keep the changed_when attribute or tag the task with molecule-idempotence-notest.

philippwaller avatar Jan 09 '22 09:01 philippwaller

@robertdebock: I came across two issues:

  1. Ansible complains about the syntax. I have tried several notations but always encountered this error. I think I've seen this notation before, but I haven't found a working example.

ERROR! couldn't resolve module/action 'ansible.builtin.{{ ansible_pkg_mgr }}'. This often indicates a misspelling, missing collection, or incorrect module path.

  1. Not all package manager modules are maintained in the same namespace:
Package Manager Namespace
apt,yum,dnf ansible.builtin
apk,zypper,pacman,portage community.general

If the first problem is solved, the second can be solved using variables. However, we only move the complexity to another file. What do you think about the idea of using only RAW commands in this role?

philippwaller avatar Jan 09 '22 10:01 philippwaller

Ah, you are right, did not think of that. In that case, the PR looks good.

About only raw; I think raw is required in this role, but prefer to use non-raw.

And one more question; sorry for the lengthiness;

  • Why are there all these different tasks, condition bases on the ansible_pkg_mgr, but all of them have the same parameters, like update_cache? Could that update_cache not be passed to then package module?

robertdebock avatar Jan 10 '22 07:01 robertdebock

  • Why are there all these different tasks, condition bases on the ansible_pkg_mgr, but all of them have the same parameters, like update_cache? Could that update_cache not be passed to then package module?

According to the documentation and some stack overflow users, there is unfortunately no generic way to update the package cache. That why I was pushing the "raw" solution. Instead of distinguishing whether or not Python is installed and then going different paths, I would just always run the existing step "install bootstrap packages (raw)". In 90% of the cases, the raw command is executed anyway and also cover the corner cases like mine where python is already installed. I can absolutely understand why you prefer modules and share your opinion in general but in this particular case I see it as a chance to simplify the code. But I am also totally fine with the current solution, I just wanna be a good sparring partner :).

philippwaller avatar Jan 10 '22 09:01 philippwaller

I have made further optimizations and removed the loop instructions. Unfortunately, I have not found a way for a leaner implementation.

philippwaller avatar Jan 29 '22 15:01 philippwaller

Hey @robertdebock, please let me know if there are any tweaks you want me to make. Cheers!

philippwaller avatar Feb 28 '22 21:02 philippwaller

Any decisions on this solution as I have come across the issue with debian 11 missing gnupg remoting on and running apt update solves the problem.

dgibbs64 avatar Oct 06 '22 14:10 dgibbs64

@robertdebock: Two years have passed, and we now have a straightforward fix for this issue. Would you be open to merging this simple one-liner 😀?

philippwaller avatar Dec 02 '23 18:12 philippwaller

Thanks, CI is running. When CI passes, I'm ready to merge.

robertdebock avatar Dec 04 '23 11:12 robertdebock