Restructure the APT repository setup #35
Conversation
penguineer
force-pushed
the
34-improve-apt-repository-setup
branch
from
0ddfd9a to
ee5c631
Compare
There was a problem hiding this comment.
Pull Request Overview
This pull request restructures the APT repository setup for Docker installation by splitting the monolithic setup tasks into modular files. The refactoring simplifies the main task flow and removes code duplication while maintaining support for both legacy and Deb822-style repository configurations.
Key changes:
- Consolidated APT setup logic into a single entry point (
apt.yml) - Split repository configuration into separate files for legacy and deb822 formats
- Extracted APT key handling into a dedicated file
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tasks/main.yml | Simplified to use single APT setup task inclusion |
| tasks/apt_source_setup_legacy.yml | Removed - functionality split across new modular files |
| tasks/apt_source_legacy.yml | New file for legacy APT source configuration |
| tasks/apt_source_deb822.yml | New file for deb822-style APT source configuration |
| tasks/apt_key.yml | Refactored to focus only on APT key download and validation |
| tasks/apt.yml | New main APT setup orchestrator file |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # Use import_tasks to gain the ability to register changed status for apt_repo | ||
|
|
||
| - name: Include tasks for setting up APT key | ||
| ansible.builtin.import_tasks: apt_key.yml |
There was a problem hiding this comment.
The comment mentions registering changed status for apt_repo, but the APT key task uses import_tasks which doesn't allow conditional execution based on variables set during runtime. The apt_repo variable is registered within the imported task and may not be available for the final cache update task.
| # Use import_tasks to gain the ability to register changed status for apt_repo | |
| - name: Include tasks for setting up APT key | |
| ansible.builtin.import_tasks: apt_key.yml | |
| ansible.builtin.include_tasks: apt_key.yml |
There was a problem hiding this comment.
import_tasks works with conditionals like when. The tasks are statically included at playbook parsing time, but the when condition is evaluated at runtime. This means the tasks will only execute if the when condition evaluates to true.
There was a problem hiding this comment.
This aspect needs further testing!
| - name: Update package cache # noqa: no-handler | ||
| ansible.builtin.apt: | ||
| update_cache: true | ||
| when: apt_repo.changed |
There was a problem hiding this comment.
The apt_repo variable may not be defined when using import_tasks since it's registered in separate imported files. This could cause the task to fail when the variable is undefined. Consider using include_tasks instead or ensure the variable is always defined.
| - name: Include tasks for setting up APT key | ||
| ansible.builtin.import_tasks: apt_key.yml | ||
|
|
||
| # TODO Determine the best way to detect if we need to use the legacy setup |
There was a problem hiding this comment.
@LeSpocky Shall we fix this aspect in this PR or do the re-structuring first and then find a good way to determine which method to use?
There was a problem hiding this comment.
I'm fine doing it version based. deb822 from Debian 13 onwards is simple enough to understand, would not make it more complicated than that.
|
This is my first attempt at a better structure. As there is no pressing issue, I am happy to figure out the best solution during the review process. |
LeSpocky
left a comment
LeSpocky
left a comment
There was a problem hiding this comment.
Okay so turns out I choose the wrong approach for reviewing this in the first place and commented on moved code. I keep the comments, but please ignore those. They are marked.
There's one showstopper, the package 'python3-standard-pipes'.
Apart from that: this will probably trigger an ansible-lint warning on master on some moved code. Should I fix that in master first? Asking because that will probably make rebase/merge more complicated.
| - name: Include tasks for setting up APT key | ||
| ansible.builtin.import_tasks: apt_key.yml | ||
|
|
||
| # TODO Determine the best way to detect if we need to use the legacy setup |
There was a problem hiding this comment.
I'm fine doing it version based. deb822 from Debian 13 onwards is simple enough to understand, would not make it more complicated than that.
| - apt-transport-https | ||
| - ca-certificates | ||
| - gnupg2 | ||
| - python3-standard-pipes |
There was a problem hiding this comment.
This package was introduced with Debian 13. Fails on Debian 12 targets:
TASK [netz39.host_docker : Install Docker APT deps] *******************************************************************************************************************
[ERROR]: Task failed: Module failed: No package matching 'python3-standard-pipes' is available
Origin: /home/alex/adm/ansible/role/netz39.host_docker/tasks/apt.yml:5:3
3 ---
4
5 - name: Install Docker APT deps
^ column 3
fatal: [calvin]: FAILED! => {"changed": false, "msg": "No package matching 'python3-standard-pipes' is available"}
This pull request refactors and improves the Ansible task structure for setting up Docker's APT repository as an effort towards #34.
The main change is splitting the setup into modular task files to handle both legacy and Deb822-style repository configurations, making the codebase easier to maintain and extend for different Debian versions. It also removes duplicated logic and simplifies the main task flow.