Skip to content

Conversation

maisim
Copy link
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
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 from 863a9f1 to 5d70a6b Compare August 22, 2025 08:44
@maisim maisim force-pushed the 3.x_Manage_deb822_format_and_apt-key_removal_in_trixie branch from 5d70a6b to 5c4cebf Compare August 22, 2025 08:50
@@ -1,13 +1,13 @@
from __future__ import annotations

import re
from typing import List
Copy link
Contributor

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

@@ -60,44 +60,176 @@ def parse_apt_repo(name):
}


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

Choose a reason for hiding this comment

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

Add return type annotation.

return repos


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

@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, use list[str] for lines

return repos


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

@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, use list[str] for lines

stanza. Returns a combined list of repo dicts for all stanzas.
"""
repos = []
stanza: List[str] = []
Copy link
Contributor

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] = []

# then export and dearmor to the APT keyring destination.
yield (
"sh -c 'set -e;"
" install -d -m 0755 /etc/apt/keyrings;"
Copy link
Contributor

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
Contributor

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants