Skip to content

Manage deb822 format and apt-key removal in trixie#1437

Closed
maisim wants to merge 4 commits intopyinfra-dev:3.xfrom
maisim:3.x_Manage_deb822_format_and_apt-key_removal_in_trixie
Closed

Manage deb822 format and apt-key removal in trixie#1437
maisim wants to merge 4 commits intopyinfra-dev:3.xfrom
maisim:3.x_Manage_deb822_format_and_apt-key_removal_in_trixie

Conversation

@maisim
Copy link
Copy Markdown
Contributor

@maisim maisim commented Aug 22, 2025

WIP

  • Pull request is based on the default branch (3.x at this time)
  • Pull request includes tests for any new/updated operations/facts
  • Pull request includes documentation for any new/updated operations/facts
  • Tests pass (see scripts/dev-test.sh)
  • Type checking & code style passes (see scripts/dev-lint.sh)

@maisim
Copy link
Copy Markdown
Contributor Author

maisim commented Aug 22, 2025

Just to let know you I am working on, comments are welcome

@maisim maisim force-pushed the 3.x_Manage_deb822_format_and_apt-key_removal_in_trixie branch 2 times, most recently from 5d70a6b to 5c4cebf Compare August 22, 2025 08:50
DonDebonair
DonDebonair reviewed Sep 7, 2025
View reviewed changes
Comment thread pyinfra/facts/apt.py Outdated
from __future__ import annotations

import re
from typing import List
Copy link
Copy Markdown
Collaborator

@DonDebonair DonDebonair Sep 7, 2025

Choose a reason for hiding this comment

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

You don't actually need to import this, as it's deprecated. You should use list now as type annotation

DonDebonair
DonDebonair reviewed Sep 7, 2025
View reviewed changes
Comment thread pyinfra/facts/apt.py Outdated


class AptSources(FactBase):
def parse_deb822_stanza(lines: list[str]):
Copy link
Copy Markdown
Collaborator

@DonDebonair DonDebonair Sep 7, 2025

Choose a reason for hiding this comment

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

Add return type annotation.

DonDebonair
DonDebonair reviewed Sep 7, 2025
View reviewed changes
Comment thread pyinfra/facts/apt.py Outdated
return repos


def parse_apt_list_file(lines: List[str]):
Copy link
Copy Markdown
Collaborator

@DonDebonair DonDebonair Sep 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Add return type annotation, use list[str] for lines

DonDebonair
DonDebonair reviewed Sep 7, 2025
View reviewed changes
Comment thread pyinfra/facts/apt.py Outdated
return repos


def parse_deb822_sources_file(lines: List[str]):
Copy link
Copy Markdown
Collaborator

@DonDebonair DonDebonair Sep 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Add return type annotation, use list[str] for lines

DonDebonair
DonDebonair reviewed Sep 7, 2025
View reviewed changes
Comment thread pyinfra/facts/apt.py Outdated
stanza. Returns a combined list of repo dicts for all stanzas.
"""
repos = []
stanza: List[str] = []
Copy link
Copy Markdown
Collaborator

@DonDebonair DonDebonair Sep 7, 2025

Choose a reason for hiding this comment

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

Suggested change
stanza: List[str] = []
stanza: list[str] = []

DonDebonair
DonDebonair reviewed Sep 7, 2025
View reviewed changes
Comment thread pyinfra/operations/apt.py Outdated
# then export and dearmor to the APT keyring destination.
yield (
"sh -c 'set -e;"
" install -d -m 0755 /etc/apt/keyrings;"
Copy link
Copy Markdown
Collaborator

@DonDebonair DonDebonair Sep 7, 2025

Choose a reason for hiding this comment

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

Why not use a fact to check if that dir already exists and only emit a files.directory command if it doesn't?

@DonDebonair
Copy link
Copy Markdown
Collaborator

DonDebonair commented Sep 7, 2025

Hey @maisim, I hope you don't mind me leaving some comments. I agreed with @Fizzadar to help him out a bit with reviewing PRs.

This is great work! Support for the new DEB822-style apt sources and a replacement for apt-key are sorely needed. apt-key has in fact been removed in the latest stable Debian release (Trixie).

I have a couple of general suggestions, and I'll leave it to @Fizzadar if he agrees:

  1. I would split this PR into 2: one for supporting DEB822 apt sources and one for replacing apt-key. Makes it easier to review and keeps PRs nicely compact
  2. The work related to DEB822 apt sources could really benefit from some unit tests. There is a lot of parsing going on, which is both error-prone and easy to test. Also AptSources.process(...) is a complex function, trying to process multiple files at once. The algorithm that you wrote (using file markers, very neat!) looks sound to me, but it took me quite some time to truly understand what is going on. Writing one or more tests for that helps with both understanding what's going on and preventing bugs.
  3. parse_deb822_stanza and parse_apt_list_file both return a list of dicts. I think there is an opportunity for type-safety here, because the fields that define an apt source are well defined, even down to the options. So my suggestion would be for both functions to return list[AptRepo] where AptRepo is a dataclass. This dataclass can even be reused across .sources and .list style definitions. I think the only real difference would be that for the latter, the components field would only ever have 1 entry.
    Ideally, I would even say that the AptSources fact should return a list[AptRepo], but as that would break backwards compatibility, we could add an argument to that fact to optionally return list[AptRepo] instead of list[dict] (something like return_type_safe: bool = False)

What do you think @maisim @Fizzadar ?

@maisim
Copy link
Copy Markdown
Contributor Author

maisim commented Sep 12, 2025

Hi @DonDebonair !
Thank you very much for your review! I agree with your comments. Regarding the 2-PR split, I need to get back into it; it may indeed be possible to take care of the new format support in a PR and then the changes in the apt part.

@DonDebonair
Copy link
Copy Markdown
Collaborator

DonDebonair commented Sep 12, 2025

Let me know if you need any help @maisim !

@maisim maisim force-pushed the 3.x_Manage_deb822_format_and_apt-key_removal_in_trixie branch from 5c4cebf to 18070e3 Compare September 13, 2025 08:43
@maisim
Copy link
Copy Markdown
Contributor Author

maisim commented Sep 13, 2025

Hi @DonDebonair ,

Big changes here, this is still a WIP. Don't waste your time and let me double check tomorrow before :)
Thanx again for your feedback, it gave me energy to continue the work 👍

@maisim
Copy link
Copy Markdown
Contributor Author

maisim commented Sep 13, 2025

It's a big all in one push but i will take care to split all this when I will get this all working ;)

@maisim maisim force-pushed the 3.x_Manage_deb822_format_and_apt-key_removal_in_trixie branch from d8a1cbe to 2c1bdf5 Compare September 22, 2025 16:10
@maisim
Copy link
Copy Markdown
Contributor Author

maisim commented Sep 24, 2025

I started separating this into several PRs, the first one is here #1460

@maisim
Copy link
Copy Markdown
Contributor Author

maisim commented Oct 16, 2025

replaced by 3 prs:
#1460
#1465
#1487

@maisim maisim closed this Oct 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DonDebonair DonDebonair DonDebonair left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@maisim @DonDebonair