-
-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: warm up package cache if Python is already installed #57
Conversation
Signed-off-by: Philipp Waller <1090452+philippwaller@users.noreply.github.com>
Signed-off-by: Philipp Waller <1090452+philippwaller@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be even (a bit) simpler:
- name: install bootstrap packages using package manager
ansible.builtin.{{ ansible_pkg_mgr }}:
name: "{{ item }}"
update_cache: yes
changed_when: no
loop: "{{ bootstrap_facts_packages.split() }}"
It feels a bit hacky, but may work. It would save some 60 lines or so.
Can you give that a try?
And; most "package" modules can work with a list, so I think it can be simplified further:
- name: install bootstrap packages using package manager
ansible.builtin.{{ ansible_pkg_mgr }}:
name: "{{ bootstrap_facts_packages }}"
update_cache: yes
changed_when: no # I'm not sure why this should no be changed, I think this line can be removed...
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 |
@robertdebock: I came across two issues:
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? |
Ah, you are right, did not think of that. In that case, the PR looks good. About only And one more question; sorry for the lengthiness;
|
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 have made further optimizations and removed the loop instructions. Unfortunately, I have not found a way for a leaner implementation. |
Hey @robertdebock, please let me know if there are any tweaks you want me to make. Cheers! |
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. |
update from source repo
# Conflicts: # tasks/main.yml
@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 😀? |
Thanks, CI is running. When CI passes, I'm ready to merge. |
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.