From dd3464ffc319e6890652a43da00a1c11797bcc82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Mon, 24 Aug 2026 17:36:35 +0200 Subject: [PATCH 01/11] fix(libvirt): une sonde qui n'a pas pu regarder ne conclut plus au vert (0.1.75) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Le contrôle du pool rendait ok quand sa sonde échouait : ne pas savoir y valait « tout va bien ». Aggravant, la sonde invoquait virsh en direct, sans la détection de préfixe sudo -n que infra/libvirt.py a précisément construite pour ce cas. Sur une machine où l'URI système exige des droits, elle échouait donc toujours, et le contrôle était toujours vert. L'hypothèse de l'issue sur _check_kvm est confirmée : il sondait virsh version sans --connect, alors que libvirt.py documente que virsh sans --connect peut viser l'URI session selon la distribution. Un utilisateur hors du groupe libvirt avait donc KVM au vert pendant que provision, qui vise l'URI système, mourait sur « Pool not found ». Les deux sondes passent désormais par run_virsh, qui porte le --connect et la détection du préfixe. Une sonde impossible rend maintenant unknown, le jeton livré en 0.1.73 dont la docstring citait déjà ce pool comme le défaut à corriger. Cas ajouté : première sonde réussie mais seconde impossible vaut unknown aussi, parce que proposer « créer » ou « démarrer » au hasard ferait échouer la remédiation. Trois sudo virsh sans -n passent par run_virsh. Le -n n'est pas un détail : la sortie est capturée, donc un prompt sudo n'a pas de terminal où s'afficher, et l'appel pend. Un bail DHCP refusé remonte désormais à l'écran plutôt que dans un journal que personne ne lit — c'est ce qui faisait échouer un hôte en « injoignable » sans cause visible. Le garde-fou vise les sudo qui CAPTURENT leur sortie sans -n, et non tout sudo : le sudo -v de pré-authentification de doctor --fix est délibérément interactif, sa sortie n'est pas capturée, et son prompt a un terminal. C'est la capture sans -n qui produit la pendaison, pas sudo lui-même. Au passage, tout OSError de run_command devient un CommandError : un binaire qui disparaît ne fait plus planter un diagnostic. Vérifié : 778 tests dont 13 neufs, chacun éprouvé par mutation, 18 e2e, ruff, mypy strict, et doctor joué sur les deux dépôts fournisseurs. Closes #172 Closes #173 Co-Authored-By: Claude Opus 5 --- CHANGELOG.fr.md | 34 +++ CHANGELOG.md | 32 +++ pyproject.toml | 2 +- src/dsoxlab/cli/infrastructure.py | 6 + src/dsoxlab/i18n/strings/en.py | 7 +- src/dsoxlab/i18n/strings/fr.py | 8 +- src/dsoxlab/infra/inventory.py | 18 +- src/dsoxlab/infra/libvirt.py | 17 +- src/dsoxlab/infra/terraform.py | 76 +++-- src/dsoxlab/services/doctor.py | 67 ++++- src/dsoxlab/utils/shell.py | 7 + tests/test_doctor_installation.py | 23 +- tests/test_sondes_virsh.py | 446 ++++++++++++++++++++++++++++++ uv.lock | 2 +- 14 files changed, 692 insertions(+), 53 deletions(-) create mode 100644 tests/test_sondes_virsh.py diff --git a/CHANGELOG.fr.md b/CHANGELOG.fr.md index f8c5314..b6773fa 100644 --- a/CHANGELOG.fr.md +++ b/CHANGELOG.fr.md @@ -9,6 +9,40 @@ et le projet suit le [versionnage sémantique](https://semver.org/lang/fr/). ## [Non publié] +## [0.1.75] - 2026-08-24 + +### Corrigé + +- **Le contrôle du pool libvirt concluait « vert » quand il n'avait pas pu + regarder** (issue #172). La sonde invoquait `virsh -c qemu:///system` en + direct, sans la détection du préfixe `sudo -n` que `infra/libvirt.py` a + construite pour ce cas : sur toute machine où l'URI système exige des + droits, la sonde échouait à chaque fois, et cet échec était rendu `ok`, un + contrôle vert en permanence précisément là où `provision` allait mourir sur + « Pool Not Found ». La sonde passe désormais par `run_virsh`, et une sonde + qui ne peut pas mesurer rend `state: unknown` (introduit en 0.1.73), qui ne + peint le verdict ni en vert ni en rouge. `_check_kvm` interroge aussi + explicitement l'**URI système** : un `virsh version` nu peut viser l'URI + session selon la distribution, et répondre parfaitement à un utilisateur que + l'URI système refuse. + +- **Trois `sudo virsh` bruts sans `-n` pouvaient pendre ou échouer en + silence** (issue #173). `_ensure_kvm_dhcp_leases` (deux occurrences) et + `_reset_kvm_domain` capturaient leur sortie : un prompt de mot de passe sudo + n'avait aucun terminal où s'afficher et l'appel restait pendu ; sur une + machine configurée par le groupe `libvirt` sans droits sudo, le bail DHCP + n'était jamais posé et l'échec partait dans un journal que personne ne lit, + l'hôte mourant plus tard en « injoignable » sans cause visible. Les trois + appels passent désormais par `run_virsh` (chemin détecté, URI système, + jamais de prompt), et un bail refusé s'affiche **à l'écran** pendant + `provision`, pas seulement au journal. Un test garde-fou refuse désormais + tout `subprocess.run(["sudo", …])` qui capture sa sortie sans `-n` dans + `src/dsoxlab/`, sur le modèle du garde-fou anti-`shell=True` de 0.1.70. + +- `run_command` convertit désormais tout `OSError` en `CommandError` au lieu + de laisser un binaire qui disparaît en cours de route faire planter + l'appelant : un diagnostic ne doit pas mourir en diagnostiquant. + ## [0.1.74] - 2026-08-24 ### Corrigé diff --git a/CHANGELOG.md b/CHANGELOG.md index d3a0ead..bac14d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,38 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.1.75] - 2026-08-24 + +### Fixed + +- **The libvirt pool check concluded "green" when it could not look** (issue + #172). The probe invoked `virsh -c qemu:///system` directly, without the + `sudo -n` prefix detection that `infra/libvirt.py` was built for: on any + machine where the system URI requires privileges, the probe failed every + time, and that failure was reported as `ok` — a permanently green check + right where `provision` was about to die on "Pool Not Found". The probe now + goes through `run_virsh`, and a probe that cannot measure yields + `state: unknown` (introduced in 0.1.73), which never paints the verdict + green nor red. `_check_kvm` also queries the **system URI** explicitly: a + bare `virsh version` may target the session URI depending on the + distribution, and answer perfectly for a user the system URI refuses. + +- **Three raw `sudo virsh` calls without `-n` could hang or fail silently** + (issue #173). `_ensure_kvm_dhcp_leases` (twice) and `_reset_kvm_domain` + captured their output, so a sudo password prompt had no terminal to show on + and the call hung; on a machine configured through the `libvirt` group + without sudo rights, the DHCP lease was never planted and the failure went + to a log nobody reads, the host later dying as "unreachable" with no visible + cause. All three calls now go through `run_virsh` (detected path, system + URI, never a prompt), and a refused lease is printed **on screen** by + `provision`, not just logged. A guard test now rejects any + `subprocess.run(["sudo", …])` that captures its output without `-n` in + `src/dsoxlab/`, on the model of the anti-`shell=True` guard from 0.1.70. + +- `run_command` now converts every `OSError` into a `CommandError` instead of + letting a binary that vanishes mid-run crash the caller — a diagnosis must + not die while diagnosing. + ## [0.1.74] - 2026-08-24 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 2c790d9..26d2a2b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "dsoxlab" -version = "0.1.74" +version = "0.1.75" description = "Turn declarative exercises into reproducible, runnable and verifiable lab environments" readme = "README.md" requires-python = ">=3.11" diff --git a/src/dsoxlab/cli/infrastructure.py b/src/dsoxlab/cli/infrastructure.py index ce525df..21706e0 100644 --- a/src/dsoxlab/cli/infrastructure.py +++ b/src/dsoxlab/cli/infrastructure.py @@ -181,6 +181,12 @@ def provision( info(f" {commande}") raise typer.Exit(4) from None + # Un bail DHCP refusé pendant l'apply se dit ici, à l'écran : confiné au + # journal, l'hôte échouerait plus tard en « injoignable » sans que rien ne + # relie l'échec à sa cause. + for message in result.warnings: + warn(message) + # Étape 3 : attendre que les VMs soient réellement joignables (sshd + # compte student + cloud-init terminé). Sans ça, le premier `dsoxlab run` # échoue en « unreachable » car la VM boote encore. diff --git a/src/dsoxlab/i18n/strings/en.py b/src/dsoxlab/i18n/strings/en.py index 85b7d5f..084263b 100644 --- a/src/dsoxlab/i18n/strings/en.py +++ b/src/dsoxlab/i18n/strings/en.py @@ -287,6 +287,10 @@ "provision_no_ssh_key": "Lab SSH key missing: {path}\nWithout it, cloud keypair would be empty and VMs unreachable.\nRun first: dsoxlab instructor bootstrap", "provision_done": "Provisioning complete — {count} host(s) ready.", "provision_failed": "Provisioning failed: {error}", + "provision_lease_refused": + "DHCP lease refused for {host} ({mac}): {error}\n" + "Without it, the host will get no IP and will later show up as " + "unreachable.", "provision_provider_conflict": "Cannot provision on '{current}': provider '{others}' still has active lab infrastructure.\nincus and KVM share the lab's network name and subnet, so they can't run at the same time.\nFinish or tear down the other one first:\n DSOXLAB_PROVIDER={other} dsoxlab destroy", "provision_waiting_ssh": "Waiting for hosts to become reachable (SSH + cloud-init)…", "provision_waiting_ssh_host": "Waiting for {host} (SSH + cloud-init), attempt {attempt}…", @@ -936,7 +940,8 @@ "detail_pool_inactive": "the `{pool}` pool is defined but never started: `provision` will fail " "with \"storage pool is not active\"", - "detail_pool_unknown": "cannot be checked without virsh", + "detail_pool_unknown": + "virsh did not answer: not verified — nothing is proven either way", "explain_apparmor_denied": "Known cause: AppArmor denies the VM disks. virt-aa-helper cannot " "resolve a disk declared by pool reference, so none of them enters the " diff --git a/src/dsoxlab/i18n/strings/fr.py b/src/dsoxlab/i18n/strings/fr.py index 69679a7..c6bdfcf 100644 --- a/src/dsoxlab/i18n/strings/fr.py +++ b/src/dsoxlab/i18n/strings/fr.py @@ -288,6 +288,10 @@ "provision_no_ssh_key": "Clé SSH du lab manquante : {path}\nSans elle, le keypair cloud serait vide et les VMs inaccessibles.\nLance d'abord : dsoxlab instructor bootstrap", "provision_done": "Provisionnement terminé — {count} hôte(s) prêt(s).", "provision_failed": "Provisionnement échoué : {error}", + "provision_lease_refused": + "Bail DHCP refusé pour {host} ({mac}) : {error}\n" + "Sans lui, l'hôte n'obtiendra pas son IP et paraîtra plus tard " + "injoignable.", "provision_provider_conflict": "Impossible de provisionner sur « {current} » : le provider « {others} » a encore une infra de lab active.\nincus et KVM partagent le nom de réseau et le subnet du lab, ils ne peuvent pas tourner en même temps.\nTermine ou détruis l'autre d'abord :\n DSOXLAB_PROVIDER={other} dsoxlab destroy", "provision_waiting_ssh": "Attente que les hôtes soient joignables (SSH + cloud-init)…", "provision_waiting_ssh_host": "Attente de {host} (SSH + cloud-init), tentative {attempt}…", @@ -947,7 +951,9 @@ "detail_pool_inactive": "le pool « {pool} » est défini mais jamais démarré : « provision » " "échouera sur « storage pool is not active »", - "detail_pool_unknown": "non vérifiable sans virsh", + "detail_pool_unknown": + "virsh n'a pas répondu : non vérifié, rien n'est prouvé ni dans un sens " + "ni dans l'autre", "explain_apparmor_denied": "Cause connue : AppArmor refuse les disques des VM. virt-aa-helper ne " "sait pas résoudre un disque déclaré par référence de pool, donc aucun " diff --git a/src/dsoxlab/infra/inventory.py b/src/dsoxlab/infra/inventory.py index b5f1f2d..f0224d6 100644 --- a/src/dsoxlab/infra/inventory.py +++ b/src/dsoxlab/infra/inventory.py @@ -32,6 +32,8 @@ from ..i18n import _ from ..models.repo import RepoMetadata from ..utils.fichiers import ecrire_atomiquement +from ..utils.shell import CommandError +from . import libvirt logger = logging.getLogger(__name__) @@ -428,12 +430,16 @@ def _reset_kvm_domain(repo_meta: RepoMetadata, fqdn: str) -> bool: if infra is None or getattr(infra, "provider", None) != "kvm": return False # check=False : la fonction rend un booléen « tenté ou non » et son - # appelant enchaîne. Un reset refusé (sudo absent, domaine déjà éteint) ne - # doit pas casser l'attente : c'est un dépannage opportuniste, pas une étape. - res = subprocess.run( - ["sudo", "virsh", "reset", fqdn], capture_output=True, text=True, check=False - ) - if res.returncode == 0: + # appelant enchaîne. Un reset refusé (droits absents, domaine déjà éteint) + # ne doit pas casser l'attente : c'est un dépannage opportuniste, pas une + # étape. `run_virsh` porte la détection du préfixe `sudo -n` : un `sudo` + # brut pendait sur un prompt sans terminal, et échouait toujours chez qui + # joint libvirt par le groupe, sans droits sudo. + try: + res = libvirt.run_virsh(["reset", fqdn], check=False) + except CommandError: + return False + if res.ok: logger.info("reset envoyé à %s (déblocage du premier boot)", fqdn) return True return False diff --git a/src/dsoxlab/infra/libvirt.py b/src/dsoxlab/infra/libvirt.py index bcd6367..16f6e91 100644 --- a/src/dsoxlab/infra/libvirt.py +++ b/src/dsoxlab/infra/libvirt.py @@ -94,20 +94,20 @@ def _uri() -> str: return os.environ.get("LIBVIRT_DEFAULT_URI") or _URI_DEFAUT -def _sonder(prefixe: list[str]) -> bool: +def _sonder(prefixe: list[str], *, timeout: int = _TIMEOUT) -> bool: """Ce préfixe permet-il de joindre l'hyperviseur ?""" try: run_command( [*prefixe, "virsh", "--connect", _uri(), "list", "--name"], check=True, - timeout=_TIMEOUT, + timeout=timeout, ) except CommandError: return False return True -def _prefixe() -> list[str]: +def _prefixe(*, timeout: int = _TIMEOUT) -> list[str]: """Rend le préfixe qui joint l'hyperviseur, ``[]`` ou ``["sudo", "-n"]``. L'ordre n'est pas indifférent. La configuration que recommande libvirt est @@ -130,7 +130,7 @@ def _prefixe() -> list[str]: return _prefixe_retenu for candidat in ([], ["sudo", "-n"]): - if _sonder(candidat): + if _sonder(candidat, timeout=timeout): logger.debug( "virsh joignable avec le préfixe %r sur %s", candidat, _uri() ) @@ -151,9 +151,14 @@ def _oublier_prefixe() -> None: def _virsh( args: list[str], *, check: bool = True, timeout: int = _TIMEOUT ) -> CommandResult: - """Invoque ``virsh`` sur l'URI système, sans jamais bloquer sur un mot de passe.""" + """Invoque ``virsh`` sur l'URI système, sans jamais bloquer sur un mot de passe. + + Le ``timeout`` borne aussi la détection du préfixe : un appelant pressé + (``doctor``, qui sonde en quelques secondes) ne doit pas attendre deux + fois trente secondes qu'un démon muet refuse de répondre aux sondes. + """ return run_command( - [*_prefixe(), "virsh", "--connect", _uri(), *args], + [*_prefixe(timeout=min(timeout, _TIMEOUT)), "virsh", "--connect", _uri(), *args], check=check, timeout=timeout, ) diff --git a/src/dsoxlab/infra/terraform.py b/src/dsoxlab/infra/terraform.py index cef908e..a36dc58 100644 --- a/src/dsoxlab/infra/terraform.py +++ b/src/dsoxlab/infra/terraform.py @@ -86,6 +86,13 @@ class ProvisionResult: hosts: dict[str, str] """Map FQDN → IP extraite de l'output ``hosts``.""" + warnings: list[str] = field(default_factory=list) + """Messages déjà traduits que l'appelant doit **afficher**. + + Un bail DHCP refusé n'empêche pas l'apply, mais condamne l'hôte à un + « injoignable » ultérieur sans cause visible : le dire seulement au + journal, c'est ne le dire à personne.""" + def is_available() -> bool: """Retourne True si la CLI ``terraform`` est dans le PATH.""" @@ -442,7 +449,7 @@ def other_active_providers(repo_meta: RepoMetadata) -> list[str]: def _ensure_kvm_dhcp_leases( repo_meta: RepoMetadata, target_hosts: list[str] | None = None -) -> None: +) -> list[str]: """Pose à chaud les baux DHCP statiques manquants d'un réseau KVM existant. Le provider ``dmacvicar/libvirt`` ne sait pas mettre à jour un réseau : @@ -460,34 +467,46 @@ def _ensure_kvm_dhcp_leases( présente ou un ``virsh`` qui échoue ne bloque pas le provision (Terraform reste la source de vérité pour la création initiale). + Les appels passent par :func:`~dsoxlab.infra.libvirt.run_virsh` : un + ``sudo virsh`` brut sans ``-n`` pendait sur les machines où sudo demande un + mot de passe (la sortie est capturée, le prompt n'a aucun terminal), et + échouait toujours sur celles configurées par groupe ``libvirt`` sans droits + sudo — le bail n'était jamais posé, et l'hôte mourait plus tard en + « injoignable » sans cause visible. + Le calcul MAC/IP DOIT rester aligné sur le template kvm (``main.tf`` : préfixe MAC ``52:54::

:00`` dérivé de ``sha256(repo_id)``, dernier octet ``idx+16`` ; ``ip = cidrhost(cidr, idx+11)``), idx étant la position dans ``infra.hosts``. + + Returns: + Les avertissements **déjà traduits** à afficher à l'utilisateur : un + par bail refusé. Best-effort ne veut pas dire muet. """ infra = repo_meta.infra if infra is None or getattr(infra, "provider", None) != "kvm" or not infra.network: - return + return [] # check=False : « le réseau n'existe pas encore » est un cas NORMAL, et # virsh le dit par un code retour. C'est une branche, pas une panne. - dump = subprocess.run( - ["sudo", "virsh", "net-dumpxml", infra.network], - capture_output=True, - text=True, - check=False, - ) - if dump.returncode != 0: - return # réseau pas encore créé : Terraform le posera avec ses baux + try: + dump = libvirt.run_virsh(["net-dumpxml", infra.network], check=False) + except CommandError: + # virsh absent ou muet : rien à poser à chaud, Terraform dira lui-même + # ce qui l'empêche de créer le réseau. + return [] + if not dump.ok: + return [] # réseau pas encore créé : Terraform le posera avec ses baux existing = {m.lower() for m in re.findall(r"mac='([^']+)'", dump.stdout)} try: network = ipaddress.ip_network(infra.cidr or "10.10.10.0/24", strict=False) except ValueError: - return + return [] # Même dérivation que le template kvm (locals.mac_prefix) : deux octets du # hash du repo.id isolent les MAC entre dépôts. Doit rester synchronisé. digest = hashlib.sha256(repo_meta.id.encode("utf-8")).hexdigest() mac_prefix = f"52:54:{digest[0:2]}:{digest[2:4]}:00" wanted = set(target_hosts) if target_hosts else None + avertissements: list[str] = [] for idx, host in enumerate(infra.hosts): if wanted is not None and host.name not in wanted: continue @@ -498,23 +517,30 @@ def _ensure_kvm_dhcp_leases( entry = f"" # check=False : best-effort assumé (voir la docstring), Terraform reste # la source de vérité pour la création initiale du réseau. - res = subprocess.run( - ["sudo", "virsh", "net-update", infra.network, - "add-last", "ip-dhcp-host", entry, "--live", "--config"], - capture_output=True, - text=True, - check=False, - ) - if res.returncode == 0: + try: + res = libvirt.run_virsh( + ["net-update", infra.network, + "add-last", "ip-dhcp-host", entry, "--live", "--config"], + check=False, + ) + except CommandError as exc: + res = exc.result + if res.ok: logger.info("bail DHCP ajouté à chaud: %s -> %s (%s)", host.name, ip, mac) else: # Best-effort ne veut pas dire muet. Sans ce bail, l'hôte n'obtiendra # pas son IP et l'attente échouera plus tard sur un « injoignable » - # qui ne dira jamais pourquoi. On continue, mais on le dit. + # qui ne dira jamais pourquoi. On continue, mais on le dit — à + # l'écran, via l'appelant : un logger.warning que personne ne lit + # est un silence. + erreur = (res.stderr or res.stdout).strip() logger.warning( - "bail DHCP refusé pour %s (%s) : %s", - host.name, mac, (res.stderr or res.stdout).strip(), + "bail DHCP refusé pour %s (%s) : %s", host.name, mac, erreur, + ) + avertissements.append( + _("provision_lease_refused", host=host.name, mac=mac, error=erreur) ) + return avertissements def apply( @@ -552,7 +578,7 @@ def apply( # DHCP des hosts ajoutés après sa création, à chaud, sinon leur VM ne # recevrait pas son IP statique. No-op au premier provision (réseau absent) # et hors kvm. Voir _ensure_kvm_dhcp_leases. - _ensure_kvm_dhcp_leases(repo_meta, target_hosts) + avertissements = _ensure_kvm_dhcp_leases(repo_meta, target_hosts) # Note : ``init`` n'est PAS appelé automatiquement ici. La CLI # ``dsoxlab provision`` l'appelle séparément (avec spinner) pour @@ -602,7 +628,9 @@ def apply( env=_provider_env(repo_meta), ) - return _read_outputs(tf_dir, env=_provider_env(repo_meta)) + result = _read_outputs(tf_dir, env=_provider_env(repo_meta)) + result.warnings.extend(avertissements) + return result #: Délai laissé à Terraform pour mourir sur ``SIGTERM`` avant le ``SIGKILL``. diff --git a/src/dsoxlab/services/doctor.py b/src/dsoxlab/services/doctor.py index 9b97740..093b999 100644 --- a/src/dsoxlab/services/doctor.py +++ b/src/dsoxlab/services/doctor.py @@ -41,9 +41,11 @@ from ..discovery.scanner import compter_fichiers_labs from ..i18n import _ from ..infra import ansible as ansible_infra +from ..infra import libvirt as libvirt_infra from ..models import LabDefinition, RepoMetadata from ..models.repo import InfraDefinition from ..models.runtime import RuntimeType +from ..utils.shell import CommandError, CommandResult from .lab_service import get_all_labs, resolve_pytest_cmd #: Providers packagés qui reposent sur un hyperviseur **local**, donc @@ -281,6 +283,22 @@ def _sonder( return None +def _sonde_virsh(args: list[str], *, delai: int = 5) -> CommandResult | None: + """Joue une sonde ``virsh`` sur l'URI système, ou rend ``None`` si impossible. + + Le chemin est celui de :func:`~dsoxlab.infra.libvirt.run_virsh` — même URI, + même détection du préfixe ``sudo -n`` — parce qu'un diagnostic qui + n'emprunte pas le chemin des commandes qu'il couvre mesure autre chose + qu'elles. ``check=False`` : un code retour non nul EST une réponse ; seul + l'appel qui ne peut pas aboutir (binaire absent, timeout) vaut ``None``. + """ + try: + result = libvirt_infra.run_virsh(args, check=False, timeout=delai) + except CommandError: + return None + return result + + def _current_user() -> str: """L'utilisateur à qui s'adresse un correctif de groupe. @@ -366,13 +384,20 @@ def _check_kvm() -> Check: ) # Un virsh qui sort en erreur est justement ce que ce contrôle cherche à # rapporter ; un virsh qui ne répond pas dit la même chose plus fort. - result = _sonder(["virsh", "version"]) - if result is None or result.returncode != 0: + # + # L'interrogation vise **l'URI système**, celle où ``provision`` crée ses + # domaines : ``virsh version`` nu peut viser l'URI session selon la + # distribution, et répondre parfaitement à un utilisateur que l'URI système + # refuse — deux lignes vertes au-dessus d'un provisionnement mort. + # ``run_virsh`` porte le ``--connect qemu:///system`` et la détection du + # préfixe ``sudo -n``, exactement comme les commandes qu'il couvre. + probe = _sonde_virsh(["version"]) + if probe is None or not probe.ok: return _check( "kvm", False, _("detail_kvm_daemon_err"), fix=_fix(["sudo", "systemctl", "start", "libvirtd"]), ) - first_line = result.stdout.splitlines()[0] if result.stdout else "ok" + first_line = probe.stdout.splitlines()[0] if probe.stdout else "ok" return _check("kvm", True, first_line) @@ -644,14 +669,20 @@ def _pools_libvirt(*, definis: bool) -> list[str] | None: Rendre ``None`` quand la sonde n'aboutit pas. ``virsh`` absent ou muet est l'affaire du contrôle KVM ; le redire ici empilerait deux rouges pour une - seule cause. + seule cause. Mais ``None`` n'est pas un vert : le contrôle appelant doit le + traduire en :data:`STATE_UNKNOWN`, jamais en « tout va bien ». + + La sonde passe par :func:`~dsoxlab.infra.libvirt.run_virsh` : invoquer + ``virsh`` en direct échouait systématiquement sur les machines où l'URI + système exige ``sudo``, et l'échec permanent de la sonde peignait le + contrôle en vert permanent. """ - cmd = ["virsh", "-c", "qemu:///system", "pool-list"] + args = ["pool-list"] if definis: - cmd.append("--all") - cmd.append("--name") - probe = _sonder(cmd) - if probe is None or probe.returncode != 0: + args.append("--all") + args.append("--name") + probe = _sonde_virsh(args) + if probe is None or not probe.ok: return None return probe.stdout.split() @@ -677,12 +708,26 @@ def _check_libvirt_pool(pool: str) -> Check: """ actifs = _pools_libvirt(definis=False) if actifs is None: - return _check("libvirt_pool", True, _("detail_pool_unknown")) + # La sonde n'a pas pu regarder : ni vert, ni rouge. L'ancien code + # rendait ``ok=True`` ici — le vert affiché faute d'avoir mesuré, + # exactement le mensonge que STATE_UNKNOWN existe pour interdire. + return _check( + "libvirt_pool", False, _("detail_pool_unknown"), + forced_state=STATE_UNKNOWN, + ) if pool in actifs: return _check("libvirt_pool", True, pool) definis = _pools_libvirt(definis=True) - if definis is not None and pool in definis: + if definis is None: + # Le pool n'est pas actif, mais sans la liste des pools définis on ne + # sait pas si le geste est « créer » ou « démarrer » : proposer l'un + # des deux au hasard ferait échouer la remédiation sur l'autre cas. + return _check( + "libvirt_pool", False, _("detail_pool_unknown"), + forced_state=STATE_UNKNOWN, + ) + if pool in definis: return _check( "libvirt_pool", False, _("detail_pool_inactive", pool=pool), fix=demarrer_pool_fix(pool), diff --git a/src/dsoxlab/utils/shell.py b/src/dsoxlab/utils/shell.py index 40d41eb..df11b28 100644 --- a/src/dsoxlab/utils/shell.py +++ b/src/dsoxlab/utils/shell.py @@ -79,6 +79,13 @@ def run_command( raise CommandError( cmd, CommandResult(returncode=-1, stdout="", stderr=f"Commande introuvable: {cmd[0]}") ) from exc + except OSError as exc: + # Un binaire qui disparaît entre deux appels, un exec refusé : l'échec + # appartient à la commande, pas à l'appelant. Le laisser remonter en + # OSError nu ferait planter un diagnostic en train de diagnostiquer. + raise CommandError( + cmd, CommandResult(returncode=-1, stdout="", stderr=str(exc)) + ) from exc result = CommandResult( returncode=proc.returncode, diff --git a/tests/test_doctor_installation.py b/tests/test_doctor_installation.py index cb4cac8..f37268b 100644 --- a/tests/test_doctor_installation.py +++ b/tests/test_doctor_installation.py @@ -16,12 +16,21 @@ import pytest from dsoxlab.i18n import _ +from dsoxlab.infra import libvirt from dsoxlab.models.lab import LabDefinition, ValidationConfig from dsoxlab.models.repo import InfraDefinition, RepoMetadata from dsoxlab.models.runtime import RuntimeConfig, RuntimeType, Target from dsoxlab.services import doctor +@pytest.fixture(autouse=True) +def sans_cache_de_prefixe() -> None: + """Les sondes du pool passent par ``run_virsh``, dont le chemin détecté + est mémorisé par processus : un test ne doit pas hériter de la détection + d'un autre.""" + libvirt._oublier_prefixe() + + def _lab(lab_id: str, runtime_type: RuntimeType) -> LabDefinition: targets = ( [] @@ -533,7 +542,12 @@ def test_un_virsh_muet_ne_conclut_pas_a_l_absence( monkeypatch: pytest.MonkeyPatch, ) -> None: """Sonder n'est pas deviner : un virsh injoignable ne prouve pas qu'un pool - manque, et le rouge appartient alors au contrôle KVM, pas à celui-ci.""" + manque, et le rouge appartient alors au contrôle KVM, pas à celui-ci. + + Mais ne pas savoir n'est pas savoir que tout va bien : l'ancien contrôle + rendait ``ok=True`` ici, le vert affiché faute d'avoir mesuré (issue #172). + L'état est désormais ``unknown`` — hors de ``failing()``, et hors du vert. + """ def _run(cmd, *args, **kwargs): # type: ignore[no-untyped-def] raise OSError("virsh a disparu entre deux appels") @@ -541,8 +555,13 @@ def _run(cmd, *args, **kwargs): # type: ignore[no-untyped-def] monkeypatch.setattr(doctor.subprocess, "run", _run) check = doctor._check_libvirt_pool("default") - assert check.ok + assert not check.ok + assert check.state == doctor.STATE_UNKNOWN assert check.detail == _("detail_pool_unknown") + assert check.fix is None + + rapport = doctor.DoctorReport(required=[check]) + assert rapport.failing() == [], "unknown ne prouve aucune panne" # ── la remédiation nomme le pool que Terraform a nommé ──────────────────────── diff --git a/tests/test_sondes_virsh.py b/tests/test_sondes_virsh.py new file mode 100644 index 0000000..f10312b --- /dev/null +++ b/tests/test_sondes_virsh.py @@ -0,0 +1,446 @@ +"""Issues #172 et #173 : toute sonde ``virsh`` emprunte le chemin détecté. + +Deux défauts, une même cause : des appels ``virsh`` qui recomposaient leur +propre ligne de commande au lieu de passer par ``run_virsh``. + +* ``doctor`` sondait le pool par un ``virsh -c qemu:///system`` direct, sans la + détection du préfixe ``sudo -n``. Sur une machine où l'URI système exige des + droits, la sonde échouait **toujours** — et l'échec rendait ``ok=True`` : + le contrôle était vert en permanence, précisément là où ``provision`` + allait mourir sur « Pool Not Found ». Et ``virsh version`` nu, sans + ``--connect``, pouvait viser l'URI session : deux lignes vertes au-dessus + d'un provisionnement mort. +* Trois ``subprocess.run(["sudo", "virsh", …], capture_output=True)`` sans + ``-n`` : un prompt sudo sans terminal où s'afficher pend, et sur une machine + configurée par groupe ``libvirt`` sans droits sudo, le bail DHCP n'était + jamais posé — l'échec partait dans un ``logger.warning`` que personne ne lit. + +Comme dans ``test_etat_libvirt.py``, ``virsh`` est simulé au niveau du wrapper +``run_command`` du module ``libvirt`` : c'est le seul niveau qui prouve que +l'appel passe bien par la détection de préfixe et par l'URI système. +""" + +from __future__ import annotations + +import re +from pathlib import Path +from typing import Any + +import pytest +from typer.testing import CliRunner + +from dsoxlab import i18n +from dsoxlab.cli import app +from dsoxlab.i18n import _ +from dsoxlab.infra import inventory, libvirt, terraform +from dsoxlab.models.repo import HostDefinition, InfraDefinition, RepoMetadata +from dsoxlab.services import doctor +from dsoxlab.utils.shell import CommandError, CommandResult + +runner = CliRunner() + + +@pytest.fixture(autouse=True) +def langue_en(monkeypatch: pytest.MonkeyPatch) -> None: + """Les assertions portent sur le texte anglais, pas sur la LANG du poste.""" + monkeypatch.setattr(i18n, "_strings", i18n._load("en")) + + +@pytest.fixture(autouse=True) +def sans_cache_de_prefixe() -> None: + """Le chemin retenu est mémorisé par processus : un test ne doit pas + hériter de la détection d'un autre.""" + libvirt._oublier_prefixe() + + +class VirshScripte: + """Un ``run_command`` de substitution, réglé sous-commande par sous-commande. + + Chaque commande reçue est enregistrée **entière**, préfixe compris : c'est + sur elle que portent les assertions « par quel chemin » et « vers quelle + URI », qui sont tout le sujet de ces deux issues. + + ``reponses`` associe une sous-commande virsh à son :class:`CommandResult`. + Une sous-commande absente répond ``rc=0`` sans sortie. ``sudo_requis`` + simule la machine où l'URI système n'est joignable que par ``sudo -n``. + """ + + def __init__( + self, + reponses: dict[str, CommandResult] | None = None, + *, + sudo_requis: bool = False, + ) -> None: + self.reponses = reponses or {} + self.sudo_requis = sudo_requis + self.commandes: list[list[str]] = [] + + @staticmethod + def _sous_commande(cmd: list[str]) -> str: + if "virsh" not in cmd: + return "" + reste = cmd[cmd.index("virsh") + 1 :] + if reste[:1] == ["--connect"]: + reste = reste[2:] + return reste[0] if reste else "" + + def __call__(self, cmd: list[str], **kwargs: Any) -> CommandResult: + self.commandes.append(cmd) + if self.sudo_requis and cmd[:2] != ["sudo", "-n"]: + return self._rendre( + cmd, kwargs, + CommandResult(returncode=1, stdout="", stderr="permission denied"), + ) + reponse = self.reponses.get( + self._sous_commande(cmd), CommandResult(returncode=0, stdout="", stderr="") + ) + return self._rendre(cmd, kwargs, reponse) + + @staticmethod + def _rendre( + cmd: list[str], kwargs: dict[str, Any], reponse: CommandResult + ) -> CommandResult: + if not reponse.ok and kwargs.get("check", True): + raise CommandError(cmd, reponse) + return reponse + + def celles_de(self, sous_commande: str) -> list[list[str]]: + return [c for c in self.commandes if self._sous_commande(c) == sous_commande] + + +def _meta(*hosts: str, provider: str = "kvm") -> RepoMetadata: + return RepoMetadata( + id="demo", + category="demo", + infra=InfraDefinition( + provider=provider, + network="lab-demo", + cidr="10.10.30.0/24", + hosts=[HostDefinition(name=h) for h in hosts], + ), + ) + + +# ── #172 : la sonde du pool passe par run_virsh ────────────────────────────── + +def test_le_pool_present_est_vu_meme_derriere_sudo( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Le cœur du défaut : la sonde directe échouait toujours sur une machine + où l'URI système exige des droits, et l'échec valait « vert ». Par + ``run_virsh``, la sonde emprunte ``sudo -n`` et **mesure** enfin.""" + virsh = VirshScripte( + { + "list": CommandResult(returncode=0, stdout="", stderr=""), + "pool-list": CommandResult(returncode=0, stdout="default\n", stderr=""), + }, + sudo_requis=True, + ) + monkeypatch.setattr(libvirt, "run_command", virsh) + + check = doctor._check_libvirt_pool("default") + + assert check.ok + assert check.detail == "default" + sondes = virsh.celles_de("pool-list") + assert sondes, "aucune sonde pool-list jouée" + for cmd in sondes: + assert cmd[:2] == ["sudo", "-n"], "la sonde doit emprunter le chemin détecté" + assert "--connect" in cmd and "qemu:///system" in cmd + + +def test_le_pool_absent_reste_un_constat_rouge( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Mesurer et ne rien trouver n'est pas « ne pas pouvoir mesurer » : le + pool manquant garde son rouge et sa remédiation de création.""" + virsh = VirshScripte( + {"pool-list": CommandResult(returncode=0, stdout="", stderr="")} + ) + monkeypatch.setattr(libvirt, "run_command", virsh) + + check = doctor._check_libvirt_pool("default") + + assert not check.ok + assert check.state == doctor.STATE_FAILED + assert check.fix is not None + assert "pool-define-as" in check.fix.display + + +def test_la_sonde_impossible_rend_unknown( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Ne pas savoir n'est ni vert ni rouge. L'ancien contrôle rendait + ``ok=True`` ici : le vert affiché faute d'avoir mesuré, l'occurrence la + plus littérale du motif que ce dépôt combat (issue #172).""" + + def _echec(cmd: list[str], **kwargs: Any) -> CommandResult: + raise CommandError( + cmd, CommandResult(returncode=1, stdout="", stderr="daemon muet") + ) + + monkeypatch.setattr(libvirt, "run_command", _echec) + + check = doctor._check_libvirt_pool("default") + + assert not check.ok + assert check.state == doctor.STATE_UNKNOWN + assert check.detail == _("detail_pool_unknown") + assert check.fix is None + assert doctor.DoctorReport(required=[check]).failing() == [] + + +def test_check_kvm_interroge_l_uri_systeme( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """``virsh version`` nu peut viser l'URI session selon la distribution, et + répondre parfaitement à un utilisateur que l'URI système refuse : deux + lignes vertes au-dessus d'un ``provision`` mort. Le contrôle doit viser + l'URI que ``provision`` utilise.""" + virsh = VirshScripte( + {"version": CommandResult(returncode=0, stdout="Compiled against 10.0\n", stderr="")} + ) + monkeypatch.setattr(libvirt, "run_command", virsh) + monkeypatch.setattr(doctor.shutil, "which", lambda name: f"/usr/bin/{name}") + + check = doctor._check_kvm() + + assert check.ok + versions = virsh.celles_de("version") + assert versions, "aucun virsh version joué" + for cmd in versions: + assert "--connect" in cmd and "qemu:///system" in cmd + + +def test_kvm_rouge_quand_l_uri_systeme_refuse( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Machine fraîche, utilisateur hors du groupe ``libvirt``, pas de sudo : + l'URI système ne répond ni en direct ni par ``sudo -n``. Le contrôle doit + le dire, au lieu d'un vert lu sur l'URI session.""" + + def _echec(cmd: list[str], **kwargs: Any) -> CommandResult: + raise CommandError( + cmd, + CommandResult(returncode=1, stdout="", stderr="authentication failed"), + ) + + monkeypatch.setattr(libvirt, "run_command", _echec) + monkeypatch.setattr(doctor.shutil, "which", lambda name: f"/usr/bin/{name}") + + check = doctor._check_kvm() + + assert not check.ok + + +# ── #173 : les trois sudo virsh passent par run_virsh ──────────────────────── + +def test_le_reset_kvm_emprunte_le_chemin_detecte( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Le ``sudo virsh reset`` brut pendait sur un prompt sans terminal, et + échouait toujours chez qui joint libvirt par le groupe, sans sudo.""" + virsh = VirshScripte() + monkeypatch.setattr(libvirt, "run_command", virsh) + + assert inventory._reset_kvm_domain(_meta("web1.lab"), "web1.lab") + + resets = virsh.celles_de("reset") + assert len(resets) == 1 + assert "sudo" not in resets[0], "le chemin direct répond : pas de sudo" + assert "--connect" in resets[0] and "qemu:///system" in resets[0] + assert resets[0][-1] == "web1.lab" + + +def test_un_reset_refuse_reste_un_faux_sans_lever( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """C'est un dépannage opportuniste : un hyperviseur injoignable ne doit + pas casser la boucle d'attente qui l'entoure.""" + + def _echec(cmd: list[str], **kwargs: Any) -> CommandResult: + raise CommandError( + cmd, CommandResult(returncode=1, stdout="", stderr="unreachable") + ) + + monkeypatch.setattr(libvirt, "run_command", _echec) + + assert not inventory._reset_kvm_domain(_meta("web1.lab"), "web1.lab") + + +def test_un_bail_refuse_est_rendu_a_l_appelant( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Best-effort ne veut pas dire muet : le refus doit remonter jusqu'à + l'écran, pas seulement au journal (issue #173).""" + virsh = VirshScripte( + { + "net-dumpxml": CommandResult( + returncode=0, stdout="lab-demo", + stderr="", + ), + "net-update": CommandResult( + returncode=1, stdout="", stderr="error: permission denied" + ), + } + ) + monkeypatch.setattr(libvirt, "run_command", virsh) + + avertissements = terraform._ensure_kvm_dhcp_leases(_meta("web1.lab")) + + assert len(avertissements) == 1 + assert "web1.lab" in avertissements[0] + assert "permission denied" in avertissements[0] + updates = virsh.celles_de("net-update") + assert updates, "aucun net-update joué" + assert "--connect" in updates[0] and "qemu:///system" in updates[0] + + +def test_un_bail_pose_ne_produit_aucun_avertissement( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Le contre-cas, sans lequel le précédent passerait sur un code qui + avertit toujours.""" + virsh = VirshScripte( + { + "net-dumpxml": CommandResult( + returncode=0, stdout="lab-demo", + stderr="", + ), + } + ) + monkeypatch.setattr(libvirt, "run_command", virsh) + + assert terraform._ensure_kvm_dhcp_leases(_meta("web1.lab")) == [] + + +def test_apply_porte_les_avertissements_de_bail( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, +) -> None: + """``apply`` est le seul relais entre le refus et l'appelant CLI : s'il + jette la liste, l'écran ne saura jamais rien.""" + monkeypatch.setattr(terraform, "is_available", lambda: True) + monkeypatch.setattr(terraform, "workdir", lambda meta: tmp_path) + monkeypatch.setattr(terraform, "write_tfvars", lambda meta: None) + monkeypatch.setattr( + terraform, "_ensure_kvm_dhcp_leases", lambda meta, hosts=None: ["refusé"] + ) + monkeypatch.setattr( + terraform, "run_command", + lambda *a, **k: CommandResult(returncode=0, stdout="", stderr=""), + ) + monkeypatch.setattr( + terraform, "_read_outputs", + lambda tf_dir, env=None: terraform.ProvisionResult(outputs={}, hosts={}), + ) + + result = terraform.apply(_meta("web1.lab")) + + assert result.warnings == ["refusé"] + + +def test_provision_affiche_le_refus_de_bail( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, +) -> None: + """Critère de l'issue #173 : l'échec se dit **à l'écran**. Un + ``logger.warning`` que personne ne lit est un silence.""" + monkeypatch.setenv("HOME", str(tmp_path / "home")) + monkeypatch.setenv("XDG_STATE_HOME", str(tmp_path / "state")) + monkeypatch.setenv("XDG_CACHE_HOME", str(tmp_path / "cache")) + monkeypatch.setenv("DSOXLAB_NO_UPDATE_CHECK", "1") + monkeypatch.setenv("DSOXLAB_LANG", "en") + monkeypatch.delenv("DSOXLAB_PROVIDER", raising=False) + (tmp_path / "home" / ".ssh" / "config.d").mkdir(parents=True) + + depot = tmp_path / "repo" + (depot / "ssh").mkdir(parents=True) + (depot / "ssh" / "id_ed25519").write_text("clé factice", encoding="utf-8") + (depot / "meta.yml").write_text( + "repo:\n id: demo\n category: demo\n" + "infra:\n provider: kvm\n network: lab-demo\n cidr: 10.10.30.0/24\n" + " hosts:\n - name: web1.lab\n", + encoding="utf-8", + ) + + refus = _( + "provision_lease_refused", + host="web1.lab", mac="52:54:00:00:00:10", error="permission denied", + ) + monkeypatch.setattr(terraform, "other_active_providers", lambda meta: []) + monkeypatch.setattr( + terraform, "find_orphan_domains", lambda meta: terraform.OrphanScan() + ) + monkeypatch.setattr(terraform, "init", lambda *a, **k: None) + monkeypatch.setattr( + terraform, "apply", + lambda *a, **k: terraform.ProvisionResult( + outputs={}, hosts={}, warnings=[refus] + ), + ) + + resultat = runner.invoke(app, ["provision", "--lab-home", str(depot)]) + + assert resultat.exit_code == 0, resultat.stdout + assert "web1.lab" in resultat.stdout + assert "DHCP lease refused" in resultat.stdout + + +# ── le garde-fou : plus un seul sudo capturé sans -n ───────────────────────── + +def _appels_sudo_captures_sans_n(texte: str) -> list[int]: + """Les lignes où un ``subprocess.run(["sudo", …])`` capture sa sortie + sans ``-n``. La capture est le critère : c'est elle qui prive le prompt + de terminal et fait pendre l'appel. Un sudo **interactif** délibéré (le + ``sudo -v`` de pré-authentification de ``doctor --fix``, gardé par un + ``isatty``) laisse le prompt s'afficher, et reste légitime.""" + lignes: list[int] = [] + for depart in re.finditer(r"subprocess\.run\(", texte): + profondeur, fin = 0, None + for i in range(depart.end() - 1, len(texte)): + if texte[i] == "(": + profondeur += 1 + elif texte[i] == ")": + profondeur -= 1 + if profondeur == 0: + fin = i + break + if fin is None: + continue + appel = texte[depart.start() : fin + 1] + capture = "capture_output" in appel or "stdout=" in appel + if capture and re.search(r'\[\s*"sudo"\s*,\s*"(?!-n")', appel): + lignes.append(texte.count("\n", 0, depart.start()) + 1) + return lignes + + +def test_plus_aucun_sudo_capture_sans_n_en_source() -> None: + """Le contrat de l'issue #173, sur le modèle du garde-fou + anti-``shell=True`` : la règle que ``libvirt.py`` s'est donnée vaut pour + tout ``src/dsoxlab/``, pas pour la moitié des appels.""" + racine = Path(__file__).resolve().parent.parent / "src" / "dsoxlab" + coupables = [ + f"{chemin}:{ligne}" + for chemin in sorted(racine.rglob("*.py")) + for ligne in _appels_sudo_captures_sans_n( + chemin.read_text(encoding="utf-8") + ) + ] + assert coupables == [] + + +def test_le_garde_fou_detecte_bien_le_motif() -> None: + """Un garde-fou qui ne détecte rien ne garde rien : on le prouve sur le + motif exact que l'issue #173 a retiré des sources.""" + fautif = ( + 'res = subprocess.run(\n' + ' ["sudo", "virsh", "reset", fqdn],' + ' capture_output=True, text=True, check=False\n' + ')\n' + ) + assert _appels_sudo_captures_sans_n(fautif) == [1] + + corrige = 'res = subprocess.run(["sudo", "-n", "virsh", "reset"], capture_output=True)' + assert _appels_sudo_captures_sans_n(corrige) == [] + + interactif = 'preauth = subprocess.run(["sudo", "-v"], check=False)' + assert _appels_sudo_captures_sans_n(interactif) == [] diff --git a/uv.lock b/uv.lock index 1f20282..9485f44 100644 --- a/uv.lock +++ b/uv.lock @@ -313,7 +313,7 @@ wheels = [ [[package]] name = "dsoxlab" -version = "0.1.74" +version = "0.1.75" source = { editable = "." } dependencies = [ { name = "ansible-core", version = "2.19.12", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.12'" }, From 50b3490215dc12ec978a1430f8faa4289300cf38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Mon, 24 Aug 2026 17:51:44 +0200 Subject: [PATCH 02/11] =?UTF-8?q?fix(runtimes/shell):=20une=20fixture=20qu?= =?UTF-8?q?i=20ne=20peut=20pas=20=C3=AAtre=20copi=C3=A9e=20ne=20laisse=20p?= =?UTF-8?q?lus=20un=20workdir=20vide=20(0.1.76)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ShellRuntime itère sur runtime.fixtures, pas sur le contenu de fixtures/. Les deux écarts possibles produisaient donc le même dégât, sans un mot : une fixture déclarée mais absente partait en logger.warning, une fixture présente mais non déclarée n'était jamais lue. Dans les deux cas run créait un challenge/work vide, sortait en 0, et l'apprenant n'avait rien à faire. C'est le défaut qui a rendu 7 labs de terraform-training injouables le 2026-07-28, tous marqués faits. Il se cachait d'autant mieux que les outils de vérification des corrigés copient, eux, le répertoire entier : la solution passait au vert pendant que le parcours apprenant était cassé. Mesuré avant de corriger : les 7 labs ont été réparés depuis, et les trois catalogues sont aujourd'hui indemnes. Le contrôle est donc préventif, et il ne casse aucune CI de catalogue tout en portant sur 70 labs shell. Deux niveaux, parce qu'ils servent deux moments. validate-structure prévient l'auteur en CI, dans les deux sens et sur le chemin qui s'évade ; le runtime protège l'apprenant qui joue un lab déjà publié. Les fichiers cachés sont exemptés : un .gitkeep versionne un répertoire vide, et le signaler serait un faux positif que chaque auteur apprendrait à ignorer — un contrôle qu'on apprend à ignorer ne contrôle plus rien. Renversement assumé, et il est motivé. Un test existant affirmait l'inverse : « a typo in one entry must not deprive the learner of the whole workdir ». C'est le fichier manquant qui l'en prive. Une erreur d'auteur n'est pas quelque chose que l'apprenant peut réparer, et un exercice amputé le fait échouer au check pour des raisons qu'il cherchera dans son propre travail. Le test est réécrit, pas supprimé, et il porte le motif du renversement. Ce que son intention demandait est gardé là où il fallait : le message nomme toutes les fixtures fautives d'un coup. La validation précède toute copie, donc c'est tout ou rien : un workdir à moitié rempli a l'air de marcher, ce qui est pire qu'un refus. Vérifié sur un lab réel copié dans un catalogue jetable : validate-structure nomme chaque fixture et sort en 1, run refuse et sort en 2, le workdir reste vide malgré 6 fixtures valides. 790 tests dont 12 neufs, chacun éprouvé par mutation dans les deux sens, 18 e2e, ruff, mypy strict, et les trois catalogues réels validés sans un faux positif. Closes #177 Co-Authored-By: Claude Opus 5 --- CHANGELOG.fr.md | 41 +++++++ CHANGELOG.md | 36 ++++++ pyproject.toml | 2 +- src/dsoxlab/cli/auteur.py | 2 + src/dsoxlab/i18n/strings/en.py | 22 ++++ src/dsoxlab/i18n/strings/fr.py | 24 ++++ src/dsoxlab/runtimes/shell.py | 41 +++++-- src/dsoxlab/validators/content.py | 65 ++++++++++ tests/test_fixtures_declarees.py | 196 ++++++++++++++++++++++++++++++ tests/test_shell_fixtures.py | 42 +++++-- uv.lock | 2 +- 11 files changed, 451 insertions(+), 22 deletions(-) create mode 100644 tests/test_fixtures_declarees.py diff --git a/CHANGELOG.fr.md b/CHANGELOG.fr.md index b6773fa..678444a 100644 --- a/CHANGELOG.fr.md +++ b/CHANGELOG.fr.md @@ -9,6 +9,47 @@ et le projet suit le [versionnage sémantique](https://semver.org/lang/fr/). ## [Non publié] +## [0.1.76] - 2026-08-24 + +### Corrigé + +- **Une fixture qui ne peut pas être copiée ne laisse plus un répertoire de + travail vide en silence** (issue #177). `ShellRuntime` itère sur + `runtime.fixtures`, pas sur le contenu de `fixtures/` : les deux écarts + possibles produisaient donc le même dégât, sans un mot. Une fixture *déclarée + mais absente du disque* partait en `logger.warning`, une fixture *présente + mais non déclarée* n'était jamais lue. Dans les deux cas `dsoxlab run` créait + un `challenge/work` vide, sortait en **0**, et l'apprenant n'avait rien à + faire. C'est le défaut qui a rendu **7 labs de `terraform-training` + injouables le 2026-07-28**, tous marqués faits — et il se cachait d'autant + mieux que les outils de vérification des corrigés copient, eux, le répertoire + entier : la solution passait au vert pendant que le parcours apprenant était + cassé. + +### Ajouté + +- **`validate-structure` compare désormais `fixtures/` à la déclaration**, dans + les deux sens : déclarée mais absente, présente mais non déclarée, et chemin + qui sort du workdir. Le contrôle est dans le lot par défaut (hors ligne), si + bien que la CI d'un catalogue attrape la faute avant un apprenant. Les + fichiers cachés en sont exemptés : un `.gitkeep` sert à versionner un + répertoire vide, et le signaler serait un faux positif que chaque auteur + apprendrait à ignorer. + +### Modifié + +- **Une fixture qui ne peut pas être copiée fait maintenant échouer `run`, au + lieu d'être ignorée.** C'est le renversement d'une décision antérieure — + qu'une faute de frappe dans une entrée ne devait pas priver l'apprenant de + tout son workdir. C'est le fichier manquant qui l'en prive : une erreur + d'auteur n'est pas quelque chose qu'il peut réparer, et un exercice amputé + échoue au `check` pour des raisons qu'il cherchera dans son propre travail. + La validation précède toute copie, donc c'est tout ou rien : un workdir à + moitié rempli a l'air de marcher. Toutes les fixtures fautives sont nommées + d'un coup, pour que l'auteur corrige en une passe plutôt qu'en autant de + `run` qu'il a de fixtures. + + ## [0.1.75] - 2026-08-24 ### Corrigé diff --git a/CHANGELOG.md b/CHANGELOG.md index bac14d8..1656000 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,42 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.1.76] - 2026-08-24 + +### Fixed + +- **A fixture that cannot be copied no longer leaves an empty work directory in + silence** (issue #177). `ShellRuntime` iterates over `runtime.fixtures`, not + over the contents of `fixtures/`, so both possible gaps caused the same + damage without a word: a fixture *declared but missing from disk* went to a + `logger.warning`, and a fixture *present but undeclared* was never read. In + both cases `dsoxlab run` created an empty `challenge/work`, exited **0**, and + the learner had nothing to do. This is the defect that made **7 labs of + `terraform-training` unplayable on 2026-07-28**, all marked done — and it hid + all the better because the tooling that checks the solutions copies the whole + directory, so the answer key went green while the learner's path was broken. + +### Added + +- **`validate-structure` now compares `fixtures/` against the declaration**, + both ways: declared-but-missing, present-but-undeclared, and a path escaping + the workdir. The check runs in the default (offline) set, so a catalogue's CI + catches the mistake before a learner does. Hidden files are exempt: a + `.gitkeep` versions an empty directory, and flagging it would be a false + positive every author would learn to ignore. + +### Changed + +- **A fixture that cannot be copied now fails `run` instead of being skipped.** + This reverses an earlier decision — that a typo in one entry should not + deprive the learner of the whole workdir. It is the missing file that + deprives them: an authoring mistake is not something they can fix, and an + amputated exercise fails `check` for reasons they will look for in their own + work. Validation happens before any copy, so it is all or nothing: a + half-filled workdir looks like it works. Every offending fixture is named at + once, so an author fixes them in one pass rather than one `run` per fixture. + + ## [0.1.75] - 2026-08-24 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 26d2a2b..3cdca87 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "dsoxlab" -version = "0.1.75" +version = "0.1.76" description = "Turn declarative exercises into reproducible, runnable and verifiable lab environments" readme = "README.md" requires-python = ">=3.11" diff --git a/src/dsoxlab/cli/auteur.py b/src/dsoxlab/cli/auteur.py index ddf479a..c22794d 100644 --- a/src/dsoxlab/cli/auteur.py +++ b/src/dsoxlab/cli/auteur.py @@ -64,6 +64,7 @@ def validate_structure_cmd( from ..validators.content import ( ContentIssue, check_doc_url, + validate_fixtures, validate_internal_links, validate_language_parity, validate_scoring, @@ -165,6 +166,7 @@ def _rendre(entete: str, lignes: list[str]) -> None: for lab in labs: rapports = [ validate_internal_links(lab), + validate_fixtures(lab), validate_scoring(lab), validate_language_parity(lab), validate_targets(lab, host_names), diff --git a/src/dsoxlab/i18n/strings/en.py b/src/dsoxlab/i18n/strings/en.py index 084263b..802d7d6 100644 --- a/src/dsoxlab/i18n/strings/en.py +++ b/src/dsoxlab/i18n/strings/en.py @@ -825,6 +825,17 @@ "target '{target}' aims at host '{host}', missing from infra.hosts in meta.yml", "content_role_host_unknown": "role '{role}' aims at '{host}', missing from infra.hosts in meta.yml", + # ── fixtures: the directory and the declaration must agree ────────────── + "content_fixture_missing": + "fixture declared but missing from fixtures/: '{fixture}' will never " + "be copied", + "content_fixture_undeclared": + "'{fixture}' sits in fixtures/ but is not declared in runtime.fixtures: " + "it will not be copied, and the work directory will stay empty", + "content_fixture_escapes": + "'{fixture}' escapes the workdir: a fixture path is relative to " + "fixtures/, with no '..' and no absolute path", + "content_doc_url_no_scheme": "no URL scheme", "content_doc_url_scheme": "unexpected scheme: {scheme}", "content_doc_url_unreachable": "unreachable: {error}", @@ -1338,4 +1349,15 @@ "0.3.0. Use `dsoxlab completion install`, which does the same thing " "under a name that says it. The wrapper in ~/.local/bin is no longer " "written: `uv tool install` and `pipx` already put theirs there.", + + # ── fixtures refused at run time (runtimes/shell.py) ──────────────────── + "fixture_lab_injouable": + "Lab '{lab}' cannot start: {count} declared fixture(s) cannot be " + "copied. Without them the work directory would be empty, leaving " + "nothing to do.", + "fixture_introuvable": + "'{fixture}' is declared in runtime.fixtures but missing from fixtures/", + "fixture_hors_workdir": + "'{fixture}' escapes the workdir: a fixture path is relative to " + "fixtures/, with no '..' and no absolute path", } diff --git a/src/dsoxlab/i18n/strings/fr.py b/src/dsoxlab/i18n/strings/fr.py index c6bdfcf..c8e9689 100644 --- a/src/dsoxlab/i18n/strings/fr.py +++ b/src/dsoxlab/i18n/strings/fr.py @@ -831,6 +831,18 @@ "du meta.yml", "content_role_host_unknown": "le rôle « {role} » vise « {host} », absent de infra.hosts du meta.yml", + # ── fixtures : le répertoire et la déclaration doivent coïncider ──────── + "content_fixture_missing": + "fixture déclarée mais absente de fixtures/ : « {fixture} » ne sera " + "jamais copiée", + "content_fixture_undeclared": + "« {fixture} » est dans fixtures/ mais n'est pas déclarée dans " + "runtime.fixtures : elle ne sera pas copiée, et le répertoire de " + "travail restera vide", + "content_fixture_escapes": + "« {fixture} » sort du workdir : un chemin de fixture est relatif à " + "fixtures/, sans « .. » ni chemin absolu", + "content_doc_url_no_scheme": "aucun schéma d'URL", "content_doc_url_scheme": "schéma inattendu : {scheme}", "content_doc_url_unreachable": "injoignable : {error}", @@ -1361,4 +1373,16 @@ "Utilise « dsoxlab completion install », qui fait la même chose sous " "un nom qui le dit. Le wrapper de ~/.local/bin n'est plus écrit : " "uv tool install et pipx y posent déjà le leur.", + + # ── fixtures refusées à l'exécution (runtimes/shell.py) ───────────────── + "fixture_lab_injouable": + "Le lab « {lab} » ne peut pas démarrer : {count} fixture(s) déclarée(s) " + "ne peuvent pas être copiées. Sans elles, le répertoire de travail " + "serait vide et il n'y aurait rien à faire.", + "fixture_introuvable": + "« {fixture} » est déclarée dans runtime.fixtures mais absente de " + "fixtures/", + "fixture_hors_workdir": + "« {fixture} » sort du workdir : un chemin de fixture est relatif à " + "fixtures/, sans « .. » ni chemin absolu", } diff --git a/src/dsoxlab/runtimes/shell.py b/src/dsoxlab/runtimes/shell.py index a475f3d..d92ee2a 100644 --- a/src/dsoxlab/runtimes/shell.py +++ b/src/dsoxlab/runtimes/shell.py @@ -32,12 +32,21 @@ import shutil from pathlib import Path +from ..i18n import _ from ..models.lab import LabDefinition from .base import BaseRuntime, EventCallback, SessionSpec logger = logging.getLogger(__name__) +class FixtureError(RuntimeError): + """Une fixture déclarée n'a pas pu être copiée : le lab serait injouable. + + Hérite de ``RuntimeError`` parce que la CLI en attrape déjà un autour de + ``run`` : le message, déjà traduit, s'affiche et la commande sort en 2. + """ + + class ShellRuntime(BaseRuntime): """Runtime local 100 % déclaratif (workdir + fixtures). @@ -68,31 +77,39 @@ def start( workdir.mkdir(parents=True, exist_ok=True) fixtures_root = lab.path / "fixtures" + # Deux passes. La première refuse, la seconde copie : un `workdir` + # à moitié rempli est pire qu'un refus, parce qu'il a l'air de marcher. + # Les fautes sont TOUTES collectées avant de lever — un auteur corrige + # en une passe plutôt qu'en autant de `run` qu'il a de fixtures. + a_copier: list[tuple[Path, Path]] = [] + fautes: list[str] = [] for rel in lab.runtime.fixtures: chemin = Path(rel) if chemin.is_absolute() or ".." in chemin.parts: - logger.warning( - "Fixture ignorée : %s sort du workdir. Un chemin de fixture " - "est toujours relatif à fixtures/, sans « .. ».", - rel, - ) + fautes.append(_("fixture_hors_workdir", fixture=rel)) continue src = fixtures_root / chemin if not src.is_file(): - logger.warning( - "Fixture déclarée mais introuvable : %s " - "(le lab devrait livrer ce fichier dans fixtures/)", - src, - ) + fautes.append(_("fixture_introuvable", fixture=rel)) continue + a_copier.append((src, workdir / chemin)) + + if fautes: + # Un `logger.warning` laissait `run` sortir en 0 sur un workdir vide, + # et l'apprenant sans rien à faire. Le lab est cassé : on le dit. + raise FixtureError( + _("fixture_lab_injouable", lab=lab.id, count=len(fautes)) + + "\n - " + "\n - ".join(fautes) + ) + + for src, dst in a_copier: # Le chemin déclaré est PRÉSERVÉ : `modules/stockage/main.tf` arrive # en `/modules/stockage/main.tf`. Aplatir sur le nom de # base rendait impossible tout lab à modules (deux `main.tf` dans # l'arborescence s'écrasaient l'un l'autre). - dst = workdir / chemin dst.parent.mkdir(parents=True, exist_ok=True) shutil.copy2(src, dst) - logger.info("fixture %s → %s", rel, dst) + logger.info("fixture %s → %s", src.name, dst) def session_spec(self, lab: LabDefinition) -> SessionSpec: """Un sous-shell dans ``/``. diff --git a/src/dsoxlab/validators/content.py b/src/dsoxlab/validators/content.py index cab32e9..e9993e1 100644 --- a/src/dsoxlab/validators/content.py +++ b/src/dsoxlab/validators/content.py @@ -327,3 +327,68 @@ def validate_targets(lab: LabDefinition, host_names: set[str]) -> ContentReport: params={"role": role, "host": hote}, )) return report + + +#: Un fichier caché de `fixtures/` n'est pas une fixture pédagogique : un +#: `.gitkeep` y sert à versionner un répertoire vide, et le signaler comme non +#: déclaré serait un faux positif que chaque auteur devrait apprendre à ignorer. +def _fixtures_sur_disque(racine: Path) -> set[str]: + if not racine.is_dir(): + return set() + return { + str(chemin.relative_to(racine)) + for chemin in racine.rglob("*") + if chemin.is_file() and not any(p.startswith(".") for p in + chemin.relative_to(racine).parts) + } + + +def validate_fixtures(lab: LabDefinition) -> ContentReport: + """Le répertoire ``fixtures/`` et la déclaration doivent dire la même chose. + + ``ShellRuntime`` itère sur ``runtime.fixtures``, **pas** sur le contenu du + répertoire. Les deux écarts possibles sont donc des défauts, et aucun ne se + voyait : + + - **déclarée mais absente du disque** : le fichier ne sera jamais copié ; + - **présente mais non déclarée** : l'auteur l'a livrée en croyant qu'elle + suffisait, et ``run`` crée un ``challenge/work`` vide. + + Le second est celui qui a rendu 7 labs injouables le 2026-07-28, tous + marqués faits — et il se cache d'autant mieux que les outils de vérification + des corrigés copient, eux, le répertoire entier : la solution passe au vert + pendant que le parcours apprenant est cassé. + """ + report = ContentReport(lab_id=lab.id) + if lab.runtime.type.value != "shell": + return report + + racine = lab.path / "fixtures" + sur_disque = _fixtures_sur_disque(racine) + declarees: set[str] = set() + + for rel in lab.runtime.fixtures: + chemin = Path(rel) + if chemin.is_absolute() or ".." in chemin.parts: + # Refusé à l'exécution : une fixture ne sort jamais du workdir. + report.issues.append(ContentIssue( + path=lab.path / "lab.yaml", + key="content_fixture_escapes", + params={"fixture": rel}, + )) + continue + declarees.add(str(chemin)) + if not (racine / chemin).is_file(): + report.issues.append(ContentIssue( + path=lab.path / "lab.yaml", + key="content_fixture_missing", + params={"fixture": rel}, + )) + + for orpheline in sorted(sur_disque - declarees): + report.issues.append(ContentIssue( + path=racine / orpheline, + key="content_fixture_undeclared", + params={"fixture": orpheline}, + )) + return report diff --git a/tests/test_fixtures_declarees.py b/tests/test_fixtures_declarees.py new file mode 100644 index 0000000..1156be5 --- /dev/null +++ b/tests/test_fixtures_declarees.py @@ -0,0 +1,196 @@ +"""Le répertoire ``fixtures/`` et la déclaration doivent dire la même chose (#177). + +``ShellRuntime`` itère sur ``runtime.fixtures``, **pas** sur le contenu du +répertoire. Les deux écarts possibles produisaient le même dégât, en silence : + +- une fixture **déclarée mais absente** partait en ``logger.warning`` ; +- une fixture **présente mais non déclarée** n'était jamais lue. + +Dans les deux cas ``dsoxlab run`` créait un ``challenge/work`` vide, sortait en +**0**, et l'apprenant n'avait rien à faire. Le défaut a rendu **7 labs de +`terraform-training` injouables le 2026-07-28**, tous marqués faits — et il se +cachait d'autant mieux que les outils de vérification des corrigés copient, eux, +le répertoire entier : la solution passait au vert pendant que le parcours +apprenant était cassé. + +Ce module éprouve les **deux** niveaux, parce qu'ils servent deux moments +différents : le validator prévient l'auteur en CI, le runtime protège +l'apprenant qui joue un lab déjà publié. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from dsoxlab.models.lab import LabDefinition +from dsoxlab.runtimes.shell import FixtureError, ShellRuntime +from dsoxlab.validators.content import validate_fixtures + +_ENTETE = """\ +id: l1-demo +title: Demo lab +level: beginner +skills: [demo] +distros: [any] +doc_url: https://example.org/docs/demo/ +""" + + +def _lab(tmp_path: Path, *, declarees: str, sur_disque: dict[str, str], + runtime: str = "shell") -> LabDefinition: + """Écrit un lab dont la déclaration et le disque peuvent diverger.""" + base = tmp_path / "labs" / "l1-demo" + base.mkdir(parents=True, exist_ok=True) + if runtime == "shell": + corps = f"runtime:\n type: shell\n workdir: challenge/work\n{declarees}" + else: + corps = ("runtime:\n type: vm\n targets:\n - name: cible\n" + " host: hote.lab\n") + (base / "lab.yaml").write_text(_ENTETE + corps, encoding="utf-8") + + for chemin_relatif, contenu in sur_disque.items(): + fichier = base / "fixtures" / chemin_relatif + fichier.parent.mkdir(parents=True, exist_ok=True) + fichier.write_text(contenu, encoding="utf-8") + return LabDefinition.from_yaml(base / "lab.yaml") + + +def _cles(lab: LabDefinition) -> list[str]: + return [souci.key for souci in validate_fixtures(lab).issues] + + +# ── Le validator : prévenir l'auteur, en CI ───────────────────────────────── + +def test_une_fixture_livree_mais_muette_est_signalee(tmp_path: Path) -> None: + """Le cas exact des 7 labs : `fixtures/` porte des fichiers, rien ne les déclare. + + C'est le plus traître des deux, parce que l'auteur voit ses fichiers et + conclut que le travail est fait. + """ + lab = _lab(tmp_path, declarees=" fixtures: []\n", + sur_disque={"acces.log": "…"}) + + assert _cles(lab) == ["content_fixture_undeclared"] + + +def test_une_fixture_declaree_sans_fichier_est_vue(tmp_path: Path) -> None: + lab = _lab(tmp_path, declarees=" fixtures:\n - absente.log\n", + sur_disque={}) + + assert _cles(lab) == ["content_fixture_missing"] + + +def test_une_declaration_conforme_ne_dit_rien(tmp_path: Path) -> None: + """Le garde-fou du garde-fou : un lab correct ne doit produire aucun bruit. + + Sans lui, un contrôle trop zélé se ferait désactiver plutôt que corriger. + """ + lab = _lab(tmp_path, declarees=" fixtures:\n - acces.log\n", + sur_disque={"acces.log": "…"}) + + assert _cles(lab) == [] + + +def test_un_chemin_qui_remonte_l_arborescence_est_refuse(tmp_path: Path) -> None: + lab = _lab(tmp_path, declarees=" fixtures:\n - ../../etc/passwd\n", + sur_disque={}) + + assert _cles(lab) == ["content_fixture_escapes"] + + +def test_un_sous_repertoire_declare_est_accepte(tmp_path: Path) -> None: + """Le chemin déclaré est préservé : un lab à modules doit rester possible.""" + lab = _lab(tmp_path, + declarees=" fixtures:\n - modules/stockage/main.tf\n", + sur_disque={"modules/stockage/main.tf": "resource {}"}) + + assert _cles(lab) == [] + + +def test_un_fichier_cache_n_est_pas_une_fixture(tmp_path: Path) -> None: + """Un `.gitkeep` sert à versionner un répertoire vide, pas à être copié. + + Le signaler serait un faux positif que chaque auteur apprendrait à ignorer, + et un contrôle qu'on apprend à ignorer ne contrôle plus rien. + """ + lab = _lab(tmp_path, declarees=" fixtures: []\n", + sur_disque={".gitkeep": ""}) + + assert _cles(lab) == [] + + +def test_un_lab_vm_n_est_pas_concerne(tmp_path: Path) -> None: + """`fixtures` n'a de sens que pour le runtime shell.""" + lab = _lab(tmp_path, declarees="", sur_disque={"trace.log": "…"}, + runtime="vm") + + assert _cles(lab) == [] + + +def test_les_deux_ecarts_se_signalent_ensemble(tmp_path: Path) -> None: + """Un rapport par lab, pas par exécution : l'auteur corrige en une passe.""" + lab = _lab(tmp_path, declarees=" fixtures:\n - absente.log\n", + sur_disque={"presente.log": "…"}) + + assert sorted(_cles(lab)) == ["content_fixture_missing", + "content_fixture_undeclared"] + + +# ── Le runtime : protéger l'apprenant, sur un lab déjà publié ─────────────── + +def test_run_refuse_de_demarrer_sur_une_fixture_absente(tmp_path: Path) -> None: + """Le cœur du défaut : `run` sortait en 0 sur un répertoire de travail vide.""" + lab = _lab(tmp_path, declarees=" fixtures:\n - absente.log\n", + sur_disque={}) + + with pytest.raises(FixtureError) as exc: + ShellRuntime().start(lab) + + assert "absente.log" in str(exc.value) + + +def test_aucune_fixture_n_est_copiee_si_l_une_manque(tmp_path: Path) -> None: + """Tout ou rien : un workdir à moitié rempli a l'air de marcher. + + C'est pire qu'un refus, parce que l'apprenant cherche l'erreur chez lui. + """ + lab = _lab(tmp_path, + declarees=" fixtures:\n - presente.log\n - absente.log\n", + sur_disque={"presente.log": "…"}) + + with pytest.raises(FixtureError): + ShellRuntime().start(lab) + + workdir = lab.path / "challenge" / "work" + assert not (workdir / "presente.log").exists(), ( + "la fixture valide ne doit pas être copiée si une autre manque" + ) + + +def test_le_refus_nomme_toutes_les_fautes(tmp_path: Path) -> None: + """Sinon l'auteur corrige une fixture par `run`, autant de fois qu'il en a.""" + lab = _lab(tmp_path, + declarees=" fixtures:\n - une.log\n - deux.log\n", + sur_disque={}) + + with pytest.raises(FixtureError) as exc: + ShellRuntime().start(lab) + + message = str(exc.value) + assert "une.log" in message and "deux.log" in message + + +def test_une_fixture_conforme_arrive_bien_dans_le_workdir(tmp_path: Path) -> None: + """L'autre bout : le comportement nominal ne doit pas se perdre.""" + lab = _lab(tmp_path, + declarees=" fixtures:\n - modules/stockage/main.tf\n", + sur_disque={"modules/stockage/main.tf": "resource {}"}) + + ShellRuntime().start(lab) + + copie = lab.path / "challenge" / "work" / "modules" / "stockage" / "main.tf" + assert copie.read_text(encoding="utf-8") == "resource {}", ( + "le chemin déclaré est préservé, il n'est pas aplati sur le nom de base" + ) diff --git a/tests/test_shell_fixtures.py b/tests/test_shell_fixtures.py index 4e959a9..8086f2e 100644 --- a/tests/test_shell_fixtures.py +++ b/tests/test_shell_fixtures.py @@ -9,15 +9,23 @@ These tests pin both directions: the tree is kept, and a path escaping the workdir is refused rather than followed. + +Since #177 a fixture that cannot be copied **fails the run** instead of being +skipped with a log line nobody reads. The reversal is deliberate: a partially +filled workdir looks like it works, so the learner hunts for a mistake that is +not theirs, while `run` exits 0. A refusal naming the missing file tells them +in one line that the lab itself is broken. """ from __future__ import annotations from pathlib import Path +import pytest + from dsoxlab.models.lab import LabDefinition, ValidationConfig from dsoxlab.models.runtime import RuntimeConfig, RuntimeType -from dsoxlab.runtimes.shell import ShellRuntime +from dsoxlab.runtimes.shell import FixtureError, ShellRuntime def _lab(tmp_path: Path, fixtures: list[str]) -> LabDefinition: @@ -78,11 +86,16 @@ def test_a_flat_fixture_still_lands_at_the_root(tmp_path: Path) -> None: def test_a_fixture_escaping_the_workdir_is_refused(tmp_path: Path) -> None: - """`../` in a declared path must not write outside the workdir.""" + """`../` in a declared path must not write outside the workdir. + + The safety invariant is unchanged and now enforced more strongly: nothing + is copied, *and* the lab refuses to start. + """ _ecrire(tmp_path, "dehors.txt", "vole\n") lab = _lab(tmp_path, ["../dehors.txt"]) - ShellRuntime().start(lab) + with pytest.raises(FixtureError): + ShellRuntime().start(lab) work = tmp_path / "challenge" / "work" assert work.is_dir(), "the workdir is still created" @@ -92,16 +105,29 @@ def test_a_fixture_escaping_the_workdir_is_refused(tmp_path: Path) -> None: def test_an_absolute_fixture_path_is_always_refused(tmp_path: Path) -> None: lab = _lab(tmp_path, ["/etc/hostname"]) - ShellRuntime().start(lab) + with pytest.raises(FixtureError): + ShellRuntime().start(lab) assert list((tmp_path / "challenge" / "work").iterdir()) == [] -def test_a_missing_fixture_does_not_stop_the_others(tmp_path: Path) -> None: - """A typo in one entry must not deprive the learner of the whole workdir.""" +def test_a_typo_in_one_entry_now_fails_the_whole_run(tmp_path: Path) -> None: + """The reversal of #177, kept explicit rather than quietly dropped. + + This test used to assert the opposite — that the other fixtures were still + copied — on the grounds that a typo should not deprive the learner of the + whole workdir. It is the missing file that deprives them: they cannot fix + an authoring mistake, and an amputated exercise fails `check` for reasons + they will look for in their own work. What the old intent asked for is kept + where it belongs: the message names every file that is missing. + """ _ecrire(tmp_path / "fixtures", "present.tf", "ok\n") lab = _lab(tmp_path, ["absent.tf", "present.tf"]) - ShellRuntime().start(lab) + with pytest.raises(FixtureError) as exc: + ShellRuntime().start(lab) - assert (tmp_path / "challenge" / "work" / "present.tf").is_file() + assert "absent.tf" in str(exc.value) + assert not (tmp_path / "challenge" / "work" / "present.tf").exists(), ( + "all or nothing: a half-filled workdir looks like it works" + ) diff --git a/uv.lock b/uv.lock index 9485f44..0b6e648 100644 --- a/uv.lock +++ b/uv.lock @@ -313,7 +313,7 @@ wheels = [ [[package]] name = "dsoxlab" -version = "0.1.75" +version = "0.1.76" source = { editable = "." } dependencies = [ { name = "ansible-core", version = "2.19.12", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.12'" }, From 4661211642efcbd0edf0f62d8e30b85b5ca6a735 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Mon, 24 Aug 2026 18:19:41 +0200 Subject: [PATCH 03/11] =?UTF-8?q?fix(utils/shell):=20check=3DFalse=20tient?= =?UTF-8?q?=20sa=20promesse,=20et=20git=20comme=20docker=20sont=20d=C3=A9c?= =?UTF-8?q?lar=C3=A9s=20(0.1.77)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit run_command transformait TimeoutExpired, FileNotFoundError et toute autre OSError en CommandError même avec check=False. Tout appelant qui croyait recevoir un CommandResult en toutes circonstances se trompait, et le nom du paramètre l'encourageait à le croire. Le docstring l'affirmait d'ailleurs : « Raises: si check=True et le code de retour est != 0 », ce qui était faux. Deux victimes, reproduites par exécution avant d'être corrigées. Sur un PATH sans git, catalog add sortait en trace Python : c'est la deuxième commande du parcours d'accueil, donc le plus mauvais moment pour montrer une trace. Et une sonde de disponibilité qui expirait faisait sauter la boucle de réessai entière, au lieu d'être comptée comme un échec à réessayer — or c'est exactement ce qu'une sonde doit tolérer, un service pas encore prêt pouvant ne pas répondre du tout. CommandResult porte maintenant failure, qui nomme pourquoi la commande n'a pas tourné, séparément d'un code de retour non nul. Les deux appellent des gestes opposés : lire stderr d'un côté, installer un paquet ou réessayer de l'autre. Corriger la racine a suffi à réparer la sonde et le catalogue, qui utilisaient déjà check=False et testaient res.ok. git et docker deviennent des contrôles de doctor. Aucun des deux n'est une dépendance Python et aucun n'était déclaré nulle part. git est requis partout puisque catalog add clone ; docker suit ce que le catalogue déclare — requis dès qu'un lab déclare runtime.services, informatif sinon. Vérifié sur les trois dépôts : requis sur terraform-training (5 labs) et ansible-training (2), informatif sur linux-dsoxlab-training (aucun). Le tirage d'image est séparé du démarrage. Le premier docker run tirait l'image dans son budget de 180 s : au-delà la commande échouait sur un message de démarrage qui ne parlait pas du réseau, en deçà run pendait plusieurs minutes sans dire pourquoi. Le tirage a son propre délai, s'annonce, et est sauté si l'image est déjà locale. Un défaut trouvé en vérifiant celui-là, et qui valait d'être corrigé à la racine : j'avais posé catalog_git_absent dans le code sans la poser dans les dictionnaires. _() rend alors la clé elle-même, et l'utilisateur voyait « catalog_git_absent » s'afficher. Mon propre test passait au vert, puisqu'il vérifiait que le message contenait « git » — que cette clé contient. Le garde-fou i18n existant s'arrêtait à validators/ et models/ ; il couvre désormais tout appel _("…") littéral du paquet, dans les deux langues, et il n'a trouvé que cette clé-là. Le test fautif est durci plutôt que réécrit. Vérifié : 809 tests dont 19 neufs, éprouvés par trois mutations distinctes, 18 e2e, ruff, mypy strict, et le symptôme d'origine rejoué sur un PATH sans git dans les deux langues. Closes #174 Co-Authored-By: Claude Opus 5 --- CHANGELOG.fr.md | 42 ++++ CHANGELOG.md | 40 ++++ pyproject.toml | 2 +- src/dsoxlab/cli/_commun.py | 9 +- src/dsoxlab/i18n/strings/en.py | 25 +++ src/dsoxlab/i18n/strings/fr.py | 27 +++ src/dsoxlab/runtimes/services.py | 35 ++- src/dsoxlab/services/catalog.py | 32 +-- src/dsoxlab/services/doctor.py | 73 ++++++ src/dsoxlab/utils/shell.py | 61 ++++- tests/test_cles_i18n_existantes.py | 81 +++++++ tests/test_commande_absente.py | 271 +++++++++++++++++++++++ tests/test_doctor.py | 8 +- tests/test_json_output.py | 3 +- tests/test_provision_et_json_services.py | 8 +- uv.lock | 2 +- 16 files changed, 684 insertions(+), 35 deletions(-) create mode 100644 tests/test_cles_i18n_existantes.py create mode 100644 tests/test_commande_absente.py diff --git a/CHANGELOG.fr.md b/CHANGELOG.fr.md index 678444a..ebd3552 100644 --- a/CHANGELOG.fr.md +++ b/CHANGELOG.fr.md @@ -9,6 +9,48 @@ et le projet suit le [versionnage sémantique](https://semver.org/lang/fr/). ## [Non publié] +## [0.1.77] - 2026-08-24 + +### Corrigé + +- **`run_command(check=False)` tient enfin sa promesse** (issue #174). Un + binaire absent, un délai dépassé ou toute autre `OSError` levait une + `CommandError` *quelles que soient les options* : tout appelant qui croyait + recevoir un `CommandResult` en toutes circonstances se trompait — et le nom du + paramètre l'encourageait à le croire. Deux victimes, mesurées par exécution + avant d'être corrigées : `dsoxlab catalog add` sortait en trace Python sur un + poste sans git, ce qui est la deuxième commande du parcours d'accueil ; et une + sonde qui expirait faisait sauter la boucle de réessai **entière** au lieu + d'être comptée comme un échec à réessayer. +- **Une clé i18n manquante ne parvient plus brute à l'utilisateur.** `_()` rend + la clé elle-même quand elle n'est pas définie : une clé posée dans le code + mais pas dans les dictionnaires s'affichait donc `catalog_git_absent`. Un + garde-fou existait, mais s'arrêtait à `validators/` et `models/` ; il couvre + désormais tout appel `_("…")` littéral du paquet, dans les deux langues. + +### Ajouté + +- **`git` et `docker` sont des contrôles de `doctor`.** Aucun des deux n'est une + dépendance Python — `uv tool install` n'apporte ni l'un ni l'autre — et aucun + n'était déclaré nulle part. `git` est requis partout, puisque `catalog add` + clone. `docker`, lui, suit ce que le catalogue déclare : requis dès qu'un lab + déclare `runtime.services`, informatif sinon, pour qu'un dépôt qui ne s'en + sert pas n'en voie jamais de rouge. +- **`CommandResult.failure`** nomme *pourquoi* une commande n'a pas pu tourner + (`not_found`, `timeout`, `os_error`), séparément d'un code de retour non nul. + Les deux appellent des gestes opposés — lire stderr, ou installer un paquet et + réessayer — et un appelant qui ne regardait que `returncode` les confondait. + +### Modifié + +- **Le tirage d'une image est désormais distinct du démarrage d'un conteneur.** + Le premier `docker run` tirait l'image dans son propre budget de 180 secondes : + au-delà, la commande échouait sur un message de démarrage qui ne parlait pas du + réseau ; en deçà, `run` pendait plusieurs minutes sans dire pourquoi. Le tirage + a maintenant son propre délai et s'annonce, et il est sauté si l'image est déjà + locale. + + ## [0.1.76] - 2026-08-24 ### Corrigé diff --git a/CHANGELOG.md b/CHANGELOG.md index 1656000..a9c0823 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,46 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.1.77] - 2026-08-24 + +### Fixed + +- **`run_command(check=False)` now keeps its promise** (issue #174). A missing + binary, an expired timeout or any other `OSError` raised a `CommandError` + *whatever the options*, so any caller expecting a `CommandResult` in all + circumstances was wrong — and the parameter name encouraged them to expect + it. Two victims, measured by running them before being fixed: `dsoxlab + catalog add` exited with a Python traceback on a machine without git, which + is the second command of the onboarding path; and a probe that timed out + aborted the **whole** retry loop instead of counting as one failure to retry. +- **A missing i18n key no longer reaches the user as a raw key.** `_()` returns + the key itself when it is undefined, so a key added to the code but not to + the dictionaries printed as `catalog_git_absent`. A guard existed but stopped + at `validators/` and `models/`; it now covers every literal `_("…")` in the + package, in both languages. + +### Added + +- **`git` and `docker` are `doctor` checks.** Neither is a Python dependency — + `uv tool install` brings neither — and neither was declared anywhere. `git` + is required everywhere, since `catalog add` clones. `docker` follows what the + catalogue declares: required as soon as one lab declares `runtime.services`, + informational otherwise, so a repository that does not use it never sees red + for it. +- **`CommandResult.failure`** names *why* a command could not run + (`not_found`, `timeout`, `os_error`), separately from a non-zero return code. + The two call for opposite gestures — read stderr, versus install a package or + retry — and a caller reading only `returncode` conflated them. + +### Changed + +- **Pulling an image is now separate from starting a container.** The first + `docker run` pulled the image within its own 180-second budget: beyond that + the command failed on a startup message that never mentioned the network, and + below it `run` hung for minutes without saying why. The pull now has its own + budget and announces itself, and is skipped when the image is already local. + + ## [0.1.76] - 2026-08-24 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 3cdca87..369aad3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "dsoxlab" -version = "0.1.76" +version = "0.1.77" description = "Turn declarative exercises into reproducible, runnable and verifiable lab environments" readme = "README.md" requires-python = ">=3.11" diff --git a/src/dsoxlab/cli/_commun.py b/src/dsoxlab/cli/_commun.py index 76ff8cd..4e2d8ce 100644 --- a/src/dsoxlab/cli/_commun.py +++ b/src/dsoxlab/cli/_commun.py @@ -287,7 +287,14 @@ def _ensure_services(lab: LabDefinition, root: Path, *, quiet: bool = False) -> # celui où le Ctrl-C tombe. with interruptible(Stage.SERVICES): try: - svc.start(service, repo_id) + svc.start( + service, repo_id, + # Un tirage d'image est long et silencieux : sans ce mot, + # `run` a l'air figé. `quiet` le tait comme le reste du + # progrès, pour ne pas polluer le document JSON. + notifier=None if quiet else + (lambda image: info(_("service_pulling", image=image))), + ) except svc.ServiceError as exc: error(_("service_failed", name=service.name, detail=str(exc))) raise typer.Exit(2) from None diff --git a/src/dsoxlab/i18n/strings/en.py b/src/dsoxlab/i18n/strings/en.py index 802d7d6..d9fb059 100644 --- a/src/dsoxlab/i18n/strings/en.py +++ b/src/dsoxlab/i18n/strings/en.py @@ -236,6 +236,9 @@ "Catalogue '{name}' is already installed in {path}. " "Update it (dsoxlab catalog update {name}), or reinstall with --force, " "which loses the progress and the work it holds.", + "catalog_git_absent": + "git is missing: 'catalog add' clones a repository and cannot work " + "without it. Install it, then run the command again.", "catalog_clone_echec": "Cloning {url} failed:\n{detail}", "catalog_sans_meta": @@ -696,6 +699,8 @@ # ── run ─────────────────────────────────────────────────────────────────── "services_docker_absent": "This lab needs a containerised service, but Docker is not reachable. Start Docker, then run the command again.", + "service_pulling": "Pulling image {image}… (first time, this may take a while)", + "service_pull_echec": "pulling image {image} failed: {detail}", "service_starting": "Starting service [bold]{name}[/bold] ({image})…", "service_ready": "Service [bold]{name}[/bold] is ready.", "service_failed": "Service [bold]{name}[/bold] could not start: {detail}", @@ -914,6 +919,26 @@ "detail_pytest_bundled": "bundled with dsoxlab (used by 'check')", "detail_pytest_via": "via {cmd}", "detail_provider_unresolved": "declared candidates: {candidates} — none selected", + # ── git and docker: two tools nothing declared ────────────────────────── + "check_git": "git", + "check_docker": "Docker", + "detail_git_missing": + "git is missing: 'dsoxlab catalog add' clones a repository and cannot " + "work without it", + "detail_git_muet": + "git is present but does not answer: cannot tell whether it works", + "detail_docker_missing": + "docker is missing: labs declaring runtime.services cannot start", + "detail_docker_muet": + "docker is present but does not answer: cannot tell whether the engine " + "works", + "detail_docker_daemon": + "the docker client answers but the daemon does not: check that it is " + "started and that your account may talk to it", + "reason_docker_no_services": + "docker is informational: no lab in this repository declares " + "runtime.services.", + "detail_terraform_missing": "not found: `provision` cannot create the machines", "detail_terraform_broken": diff --git a/src/dsoxlab/i18n/strings/fr.py b/src/dsoxlab/i18n/strings/fr.py index c8e9689..ce1edff 100644 --- a/src/dsoxlab/i18n/strings/fr.py +++ b/src/dsoxlab/i18n/strings/fr.py @@ -237,6 +237,9 @@ "Le catalogue « {name} » est déjà installé dans {path}. " "Mets-le à jour (dsoxlab catalog update {name}), ou réinstalle-le " "avec --force, ce qui perd la progression et le travail qui s'y trouvent.", + "catalog_git_absent": + "git est absent : « catalog add » clone un dépôt et ne peut pas " + "fonctionner sans lui. Installe-le, puis relance la commande.", "catalog_clone_echec": "Le clone de {url} a échoué :\n{detail}", "catalog_sans_meta": @@ -700,6 +703,8 @@ # ── run ─────────────────────────────────────────────────────────────────── "services_docker_absent": "Ce lab a besoin d'un service conteneurisé, mais Docker est injoignable. Démarrez Docker, puis relancez la commande.", + "service_pulling": "Tirage de l'image {image}… (première fois, cela peut être long)", + "service_pull_echec": "le tirage de l'image {image} a échoué : {detail}", "service_starting": "Démarrage du service [bold]{name}[/bold] ({image})…", "service_ready": "Service [bold]{name}[/bold] prêt.", "service_failed": "Le service [bold]{name}[/bold] n'a pas pu démarrer : {detail}", @@ -923,6 +928,28 @@ "detail_pytest_bundled": "embarqué avec dsoxlab (celui qu'utilise « check »)", "detail_pytest_via": "via {cmd}", "detail_provider_unresolved": "candidats déclarés : {candidates} — aucun choisi", + # ── git et docker : deux outils que rien ne déclarait ─────────────────── + "check_git": "git", + "check_docker": "Docker", + "detail_git_missing": + "git est absent : « dsoxlab catalog add » clone un dépôt et ne peut " + "pas fonctionner sans lui", + "detail_git_muet": + "git est présent mais ne répond pas : impossible de dire s'il " + "fonctionne", + "detail_docker_missing": + "docker est absent : les labs qui déclarent runtime.services ne " + "peuvent pas démarrer", + "detail_docker_muet": + "docker est présent mais ne répond pas : impossible de dire si le " + "moteur fonctionne", + "detail_docker_daemon": + "le client docker répond mais pas le démon : vérifie qu'il est démarré " + "et que ton compte peut lui parler", + "reason_docker_no_services": + "docker est informatif : aucun lab de ce dépôt ne déclare " + "runtime.services.", + "detail_terraform_missing": "introuvable : « provision » ne peut pas créer les machines", "detail_terraform_broken": diff --git a/src/dsoxlab/runtimes/services.py b/src/dsoxlab/runtimes/services.py index eaee541..e4ff319 100644 --- a/src/dsoxlab/runtimes/services.py +++ b/src/dsoxlab/runtimes/services.py @@ -26,6 +26,7 @@ import re import socket import time +from collections.abc import Callable from dataclasses import dataclass from ..i18n import _ @@ -251,7 +252,38 @@ def _run_post_start(service: Service, name: str) -> None: )) -def start(service: Service, repo_id: str) -> str: +#: Le tirage d'une image n'a rien à voir avec le démarrage d'un conteneur : +#: il traverse le réseau, et une image de plusieurs gigaoctets sur le réseau +#: partagé d'une salle de formation dépasse largement le délai d'un `run`. +_DELAI_TIRAGE = 1800 + + +def _image_locale(image: str) -> bool: + return run_command(["docker", "image", "inspect", image], + check=False, timeout=15).ok + + +def _tirer(image: str, notifier: Callable[[str], None] | None) -> None: + """Tire l'image si elle n'est pas déjà là, en le disant. + + Le premier ``docker run`` tirait l'image dans son propre délai. Deux + conséquences : au-delà, la commande échouait sur un message de démarrage + qui ne parlait pas du réseau ; et en deçà, l'apprenant voyait `run` pendre + plusieurs minutes sans savoir que quelque chose se téléchargeait. + """ + if _image_locale(image): + return + if notifier is not None: + notifier(image) + res = run_command(["docker", "pull", image], check=False, + timeout=_DELAI_TIRAGE) + if not res.ok: + raise ServiceError(_("service_pull_echec", image=image, + detail=(res.stderr or res.stdout).strip())) + + +def start(service: Service, repo_id: str, *, + notifier: Callable[[str], None] | None = None) -> str: """Démarre (ou réutilise) le conteneur d'un service et attend sa disponibilité. Idempotent : si le conteneur tourne déjà **avec la configuration déclarée**, @@ -301,6 +333,7 @@ def start(service: Service, repo_id: str) -> str: cmd += list(service.run_args) cmd.append(service.image) + _tirer(service.image, notifier) res = run_command(cmd, check=False, timeout=180) if not res.ok: raise ServiceError(_( diff --git a/src/dsoxlab/services/catalog.py b/src/dsoxlab/services/catalog.py index b3dc413..833a27d 100644 --- a/src/dsoxlab/services/catalog.py +++ b/src/dsoxlab/services/catalog.py @@ -32,7 +32,7 @@ from ..config import xdg_data_home, xdg_state_home from ..i18n import _ from ..templates import catalogues_manifeste -from ..utils.shell import run_command +from ..utils.shell import FAILURE_NOT_FOUND, CommandResult, run_command #: Un identifiant de catalogue sert de nom de répertoire : on le borne pour #: qu'il ne puisse ni remonter l'arborescence, ni porter d'espace. @@ -126,12 +126,24 @@ def _est_un_catalogue(racine: Path) -> bool: return (racine / "meta.yml").is_file() +def _git(args: list[str], *, timeout: int) -> CommandResult: + """Appelle git, et refuse tôt s'il n'est pas là. + + `git` n'est pas une dépendance Python : il n'est ni installé par + ``uv tool install``, ni déclaré nulle part. Sans ce point de passage, son + absence remontait en « Commande introuvable », qui dit ce qui s'est passé + mais pas quoi faire — sur la deuxième commande que tape un nouvel + utilisateur. + """ + res = run_command(["git", *args], check=False, timeout=timeout) + if res.failure == FAILURE_NOT_FOUND: + raise CatalogueError(_("catalog_git_absent")) + return res + + def _origine(racine: Path) -> str | None: """L'URL d'origine d'un catalogue cloné, si git la connaît.""" - res = run_command( - ["git", "-C", str(racine), "remote", "get-url", "origin"], - check=False, timeout=15, - ) + res = _git(["-C", str(racine), "remote", "get-url", "origin"], timeout=15) return res.stdout.strip() if res.ok and res.stdout.strip() else None @@ -239,10 +251,7 @@ def ajouter(reference: str, *, force: bool = False) -> CatalogueInstalle: shutil.rmtree(destination) destination.parent.mkdir(parents=True, exist_ok=True) - res = run_command( - ["git", "clone", "--depth", "1", url, str(destination)], - check=False, timeout=900, - ) + res = _git(["clone", "--depth", "1", url, str(destination)], timeout=900) if not res.ok: # Un clone à moitié fait laisserait un répertoire qui n'est pas un # catalogue, que `list` montrerait comme installé. @@ -265,10 +274,7 @@ def mettre_a_jour(identifiant: str) -> str: if not _ID_VALIDE.match(identifiant) or not _est_un_catalogue(racine): raise CatalogueError(_("catalog_absent", name=identifiant)) - res = run_command( - ["git", "-C", str(racine), "pull", "--ff-only"], - check=False, timeout=600, - ) + res = _git(["-C", str(racine), "pull", "--ff-only"], timeout=600) if not res.ok: raise CatalogueError(_("catalog_update_echec", name=identifiant, diff --git a/src/dsoxlab/services/doctor.py b/src/dsoxlab/services/doctor.py index 093b999..5ba3bc6 100644 --- a/src/dsoxlab/services/doctor.py +++ b/src/dsoxlab/services/doctor.py @@ -908,6 +908,67 @@ def uses_vm(labs: list[LabDefinition]) -> bool: return any(lab.runtime.type in _VM_RUNTIMES for lab in labs) +def _check_git() -> Check: + """``catalog add`` clone : sans git, la deuxième commande du parcours échoue. + + git n'est pas une dépendance Python — ``uv tool install`` ne l'apporte pas — + et il ne figurait dans aucun contrôle. Son absence remontait en trace + Python sur la commande d'accueil, ce qui est le plus mauvais moment pour + montrer une trace à quelqu'un. + """ + if shutil.which("git") is None: + return _check( + "git", False, _("detail_git_missing"), + hint="https://git-scm.com/downloads", + ) + result = _sonder(["git", "--version"]) + if result is None: + # Présent dans le PATH mais muet : ni vert ni rouge, on ne sait pas. + return _check("git", False, _("detail_git_muet"), forced_state=STATE_UNKNOWN) + if result.returncode != 0: + detail = (result.stderr or result.stdout).strip().splitlines() + return _check("git", False, detail[-1] if detail else _("detail_git_muet"), + hint="https://git-scm.com/downloads") + return _check("git", True, result.stdout.strip() or "ok") + + +def uses_services(labs: list[LabDefinition]) -> bool: + """Ce dépôt a-t-il au moins un lab qui déclare des conteneurs ? + + Lit le contrat, et rien d'autre : le moteur ne sait pas quelles images un + catalogue déclare, et n'a pas à le savoir. + """ + return any(lab.runtime.services for lab in labs) + + +def _check_docker() -> Check: + """Un lab qui déclare ``runtime.services`` ne démarre pas sans moteur. + + Trois situations, et elles appellent trois gestes différents : le binaire + n'est pas là (installer), il est là mais le démon ne répond pas (démarrer + le service, ou se mettre dans le groupe), ou la sonde elle-même n'aboutit + pas — auquel cas on ne sait pas, et on le dit plutôt que de trancher. + """ + if shutil.which("docker") is None: + return _check( + "docker", False, _("detail_docker_missing"), + hint="https://docs.docker.com/engine/install/", + ) + result = _sonder(["docker", "version", "--format", "{{.Server.Version}}"], delai=15) + if result is None: + return _check("docker", False, _("detail_docker_muet"), forced_state=STATE_UNKNOWN) + if result.returncode != 0: + # Le cas courant : le client répond, le démon non. Le message doit + # nommer le démon, sinon on cherche un paquet déjà installé. + detail = (result.stderr or result.stdout).strip().splitlines() + return _check( + "docker", False, + detail[-1] if detail else _("detail_docker_daemon"), + hint="https://docs.docker.com/engine/install/linux-postinstall/", + ) + return _check("docker", True, result.stdout.strip() or "ok") + + def _hypervisor_checks() -> dict[str, Check]: return {"kvm": _check_kvm(), "incus": _check_incus()} @@ -927,6 +988,18 @@ def collect_checks(root: Path, repo_meta: RepoMetadata | None) -> DoctorReport: labs = get_all_labs(root) report = DoctorReport() report.required.extend([_check_python(), _check_pytest(root), _check_shell()]) + # git sert au parcours d'accueil (`catalog add` clone), quel que soit le + # domaine du dépôt : il est requis partout. + report.required.append(_check_git()) + + # docker, lui, dépend de ce que le catalogue déclare — jamais de son + # domaine. Un dépôt sans `runtime.services` n'a aucune raison de voir du + # rouge pour un moteur qu'il n'utilise pas. + if uses_services(labs): + report.required.append(_check_docker()) + else: + report.optional.append(_check_docker()) + report.notes.append(_("reason_docker_no_services")) needs_vm = uses_vm(labs) active = repo_meta.infra.provider if repo_meta else "" diff --git a/src/dsoxlab/utils/shell.py b/src/dsoxlab/utils/shell.py index df11b28..1463005 100644 --- a/src/dsoxlab/utils/shell.py +++ b/src/dsoxlab/utils/shell.py @@ -10,15 +10,38 @@ logger = logging.getLogger(__name__) +#: Causes d'échec qui ne viennent pas du code de retour de la commande, mais +#: du fait qu'elle n'a pas pu s'exécuter du tout. Jetons **stables** : un +#: appelant les compare, il ne les affiche pas — le texte lisible est dans +#: ``stderr``, et la traduction dans la couche qui rend le message. +FAILURE_NOT_FOUND = "not_found" +"""Le binaire n'est pas dans le PATH.""" + +FAILURE_TIMEOUT = "timeout" +"""La commande a dépassé son délai. Elle tournait peut-être encore.""" + +FAILURE_OS_ERROR = "os_error" +"""Exec refusé, descripteur épuisé, binaire disparu entre-temps.""" + + @dataclass class CommandResult: returncode: int stdout: str stderr: str + failure: str | None = None + """Pourquoi la commande n'a pas pu s'exécuter, ou ``None`` si elle a tourné. + + Distingue « la commande a répondu, mal » de « la commande n'a pas répondu ». + Un appelant qui ne regarde que ``returncode`` traiterait un binaire absent + comme un échec métier, et un délai dépassé comme un refus — deux causes qui + appellent des gestes opposés (installer un paquet, ou réessayer). + """ + @property def ok(self) -> bool: - return self.returncode == 0 + return self.returncode == 0 and self.failure is None class CommandError(RuntimeError): @@ -33,6 +56,21 @@ def __init__(self, cmd: list[str], result: CommandResult) -> None: ) +def _echec(cmd: list[str], cause: str, detail: str, *, check: bool) -> CommandResult: + """Traduit un échec d'exécution en résultat, ou en exception si ``check``. + + C'est ici que ``check=False`` tient sa promesse. Les trois causes levaient + auparavant **quelles que soient** les options, si bien qu'un appelant qui + croyait recevoir un ``CommandResult`` en toutes circonstances se trompait — + et le nom du paramètre l'encourageait à le croire. Un `git` absent sortait + donc en traceback Python sur la deuxième commande du parcours d'accueil. + """ + resultat = CommandResult(returncode=-1, stdout="", stderr=detail, failure=cause) + if check: + raise CommandError(cmd, resultat) + return resultat + + def run_command( cmd: list[str], *, @@ -47,14 +85,18 @@ def run_command( cmd: Liste de tokens de la commande. cwd: Répertoire de travail (optionnel). timeout: Timeout en secondes (défaut 120). - check: Lève CommandError si le code de retour est != 0. + check: Lève CommandError si la commande échoue — code de retour non + nul, binaire absent, délai dépassé ou erreur système. Avec + ``check=False``, **aucune** de ces situations ne lève : le résultat + porte ``returncode = -1`` et un ``failure`` qui dit laquelle. env: Variables d'environnement supplémentaires. Returns: CommandResult avec returncode, stdout et stderr. Raises: - CommandError: Si check=True et le code de retour est != 0. + CommandError: Si ``check=True`` et que la commande a échoué, pour + n'importe laquelle des quatre causes ci-dessus. """ logger.debug("run: %s (cwd=%s)", " ".join(cmd), cwd) @@ -74,18 +116,15 @@ def run_command( check=False, ) except subprocess.TimeoutExpired as exc: - raise CommandError(cmd, CommandResult(returncode=-1, stdout="", stderr=str(exc))) from exc - except FileNotFoundError as exc: - raise CommandError( - cmd, CommandResult(returncode=-1, stdout="", stderr=f"Commande introuvable: {cmd[0]}") - ) from exc + return _echec(cmd, FAILURE_TIMEOUT, str(exc), check=check) + except FileNotFoundError: + return _echec(cmd, FAILURE_NOT_FOUND, f"{cmd[0]}: No such file or directory", + check=check) except OSError as exc: # Un binaire qui disparaît entre deux appels, un exec refusé : l'échec # appartient à la commande, pas à l'appelant. Le laisser remonter en # OSError nu ferait planter un diagnostic en train de diagnostiquer. - raise CommandError( - cmd, CommandResult(returncode=-1, stdout="", stderr=str(exc)) - ) from exc + return _echec(cmd, FAILURE_OS_ERROR, str(exc), check=check) result = CommandResult( returncode=proc.returncode, diff --git a/tests/test_cles_i18n_existantes.py b/tests/test_cles_i18n_existantes.py new file mode 100644 index 0000000..8c91072 --- /dev/null +++ b/tests/test_cles_i18n_existantes.py @@ -0,0 +1,81 @@ +"""Toute clé passée à ``_()`` doit exister dans les deux langues. + +Ce garde-fou est né d'une clé posée dans le code et jamais dans les +dictionnaires : ``_()`` rend alors **la clé elle-même**, si bien que +l'utilisateur voyait `catalog_git_absent` s'afficher là où une phrase était +attendue. Rien ne l'attrapait, et le test qui aurait dû le faire passait au +vert : il vérifiait que le message contenait « git », or la chaîne +`catalog_git_absent` en contient. + +Un garde-fou existait déjà (`test_i18n_validators.py`), mais son périmètre +s'arrêtait à `validators/` et `models/`, et aux clés passées en `key=`. Les +appels `_("…")` du reste du paquet — la CLI, les services, les runtimes — +n'étaient vérifiés nulle part. + +Les clés construites dynamiquement (`_(f"check_{key}")`) sont hors de portée +d'une lecture statique et sont ignorées : elles sont couvertes par les tests +qui exercent réellement les commandes. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import dsoxlab +from dsoxlab.i18n.strings.en import STRINGS as EN +from dsoxlab.i18n.strings.fr import STRINGS as FR + +RACINE = Path(dsoxlab.__file__).parent + + +def _cles_litterales() -> dict[str, list[str]]: + """Chaque clé littérale passée à ``_()``, avec les fichiers qui l'emploient.""" + trouvees: dict[str, list[str]] = {} + for chemin in sorted(RACINE.rglob("*.py")): + if chemin.is_relative_to(RACINE / "i18n"): + continue + arbre = ast.parse(chemin.read_text(encoding="utf-8")) + for noeud in ast.walk(arbre): + if not isinstance(noeud, ast.Call): + continue + if getattr(noeud.func, "id", "") != "_" or not noeud.args: + continue + premier = noeud.args[0] + if isinstance(premier, ast.Constant) and isinstance(premier.value, str): + trouvees.setdefault(premier.value, []).append( + str(chemin.relative_to(RACINE)) + ) + return trouvees + + +def test_la_lecture_des_sources_est_representative() -> None: + """Sans ce contrôle, une lecture cassée rendrait le test suivant toujours vert. + + C'est le défaut que ce module corrige : un contrôle qui, faute de pouvoir + mesurer, conclut que tout va bien. + """ + assert len(_cles_litterales()) > 200 + + +def test_chaque_cle_du_paquet_existe_en_anglais() -> None: + manquantes = { + cle: fichiers for cle, fichiers in _cles_litterales().items() if cle not in EN + } + assert manquantes == {}, f"clés absentes de strings/en.py : {manquantes}" + + +def test_chaque_cle_du_paquet_existe_en_francais() -> None: + manquantes = { + cle: fichiers for cle, fichiers in _cles_litterales().items() if cle not in FR + } + assert manquantes == {}, f"clés absentes de strings/fr.py : {manquantes}" + + +def test_les_deux_dictionnaires_portent_les_memes_cles() -> None: + """Une clé traduite d'un seul côté s'affiche en anglais dans une session FR. + + Écrire un libellé en anglais n'est pas plus neutre que l'écrire en français. + """ + assert sorted(set(EN) - set(FR)) == [], "présentes en EN, absentes en FR" + assert sorted(set(FR) - set(EN)) == [], "présentes en FR, absentes en EN" diff --git a/tests/test_commande_absente.py b/tests/test_commande_absente.py new file mode 100644 index 0000000..03573f8 --- /dev/null +++ b/tests/test_commande_absente.py @@ -0,0 +1,271 @@ +"""`check=False` tient sa promesse, et git comme docker sont déclarés (#174). + +`run_command` transformait `TimeoutExpired`, `FileNotFoundError` et toute autre +`OSError` en `CommandError` **même avec `check=False`**. Tout appelant qui +croyait recevoir un `CommandResult` en toutes circonstances se trompait — et le +nom du paramètre l'encourageait à le croire. + +Deux victimes, mesurées par exécution avant d'être corrigées : + +- **git absent** : `dsoxlab catalog add` sortait en trace Python. C'est la + deuxième commande du parcours d'accueil, donc le plus mauvais moment possible + pour montrer une trace à quelqu'un. Or git n'est ni une dépendance déclarée, + ni un contrôle de `doctor`. +- **une sonde qui expire** : la `CommandError` faisait sauter la boucle de + réessai *entière*, au lieu d'être comptée comme un échec à réessayer. + +Ce module éprouve la racine et les deux bords, parce que corriger la racine sans +vérifier les bords laisserait croire le travail fait. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path +from typing import Any + +import pytest + +from dsoxlab.utils.shell import ( + FAILURE_NOT_FOUND, + FAILURE_OS_ERROR, + FAILURE_TIMEOUT, + CommandError, + run_command, +) + +# ── La racine : la promesse de check=False ────────────────────────────────── + +def test_un_binaire_absent_ne_leve_plus_sans_check(tmp_path: Path) -> None: + resultat = run_command(["binaire-qui-n-existe-pas-du-tout"], check=False) + + assert resultat.returncode == -1 + assert resultat.failure == FAILURE_NOT_FOUND + assert not resultat.ok + + +def test_un_binaire_absent_leve_toujours_avec_check() -> None: + """L'autre bout : `check=True` garde exactement le comportement d'avant.""" + with pytest.raises(CommandError): + run_command(["binaire-qui-n-existe-pas-du-tout"], check=True) + + +def test_un_delai_depasse_ne_leve_plus_sans_check() -> None: + resultat = run_command(["sleep", "5"], check=False, timeout=1) + + assert resultat.failure == FAILURE_TIMEOUT + assert not resultat.ok + + +def test_un_delai_depasse_leve_toujours_avec_check() -> None: + with pytest.raises(CommandError): + run_command(["sleep", "5"], check=True, timeout=1) + + +def test_une_erreur_systeme_est_nommee(monkeypatch: pytest.MonkeyPatch) -> None: + """Un exec refusé, un descripteur épuisé : la cause se distingue des autres.""" + def _refuse(*args: Any, **kwargs: Any) -> None: + raise PermissionError(13, "Permission denied") + + monkeypatch.setattr(subprocess, "run", _refuse) + resultat = run_command(["peu-importe"], check=False) + + assert resultat.failure == FAILURE_OS_ERROR + + +def test_une_commande_qui_repond_mal_n_est_pas_un_echec_d_execution() -> None: + """La distinction qui justifie le champ : répondre mal ≠ ne pas répondre. + + Les deux gestes sont opposés — lire stderr d'un côté, installer un paquet + ou réessayer de l'autre. Un appelant qui ne regarde que `returncode` les + confondrait. + """ + resultat = run_command(["sh", "-c", "exit 3"], check=False) + + assert resultat.returncode == 3 + assert resultat.failure is None, "la commande a bien tourné, elle a mal fini" + assert not resultat.ok + + +def test_une_commande_qui_reussit_ne_porte_aucune_cause() -> None: + resultat = run_command(["sh", "-c", "exit 0"], check=False) + + assert resultat.ok and resultat.failure is None + + +def test_le_message_d_absence_ne_depend_pas_de_la_langue() -> None: + """`utils/` est sous la couche i18n : il n'y compose aucune phrase traduite. + + Une phrase française en dur y serait sortie telle quelle sous + `DSOXLAB_LANG=en`, et la traduction appartient à la couche qui affiche. + """ + resultat = run_command(["binaire-qui-n-existe-pas-du-tout"], check=False) + + assert "introuvable" not in resultat.stderr.lower() + + +# ── Le bord : git absent devient une phrase, pas une trace ────────────────── + +def test_git_absent_donne_une_erreur_de_catalogue( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, +) -> None: + """`catalog add` sortait en trace Python sur un poste sans git.""" + from dsoxlab.services import catalog + + monkeypatch.setattr(catalog, "run_command", lambda *a, **k: _absent()) + + with pytest.raises(catalog.CatalogueError) as exc: + catalog._git(["clone", "https://example.org/x", str(tmp_path)], timeout=5) + + message = str(exc.value) + # Assertion durcie : la version d'origine se contentait de « git », que la + # clé i18n `catalog_git_absent` contient elle-même. Elle passait donc au + # vert alors que la clé n'existait dans aucun dictionnaire et s'affichait + # brute à l'utilisateur — le faux positif que ce test devait exclure. + assert message != "catalog_git_absent", "la clé i18n n'est pas traduite" + assert "git" in message.lower() + + +def _absent() -> Any: + from dsoxlab.utils.shell import CommandResult + + return CommandResult(returncode=-1, stdout="", stderr="…", + failure=FAILURE_NOT_FOUND) + + +# ── Le bord : doctor déclare enfin les deux outils ────────────────────────── + +def test_git_est_un_controle_requis(tmp_path: Path) -> None: + """git sert au parcours d'accueil, quel que soit le domaine du dépôt.""" + from dsoxlab.services.doctor import collect_checks + + (tmp_path / "labs").mkdir() + (tmp_path / "meta.yml").write_text("repo:\n id: essai\n category: essai\n", + encoding="utf-8") + rapport = collect_checks(tmp_path, None) + + assert "git" in [c.key for c in rapport.required] + + +def test_docker_suit_ce_que_le_catalogue_declare(tmp_path: Path) -> None: + """Requis si un lab déclare des services, informatif sinon. + + Le classement lit le contrat et rien d'autre : un dépôt qui n'utilise pas + docker n'a aucune raison d'en voir du rouge, et le moteur n'a pas à savoir + quelles images un catalogue déclare. + """ + from dsoxlab.services.doctor import collect_checks + + (tmp_path / "meta.yml").write_text("repo:\n id: essai\n category: essai\n", + encoding="utf-8") + base = tmp_path / "labs" / "l1" + base.mkdir(parents=True) + (base / "lab.yaml").write_text( + "id: l1\ntitle: T\nlevel: l1\nskills: [s]\ndistros: [any]\n" + "doc_url: https://example.org/\n" + "runtime:\n type: shell\n workdir: challenge/work\n", + encoding="utf-8") + + sans = collect_checks(tmp_path, None) + assert "docker" in [c.key for c in sans.optional], ( + "sans service déclaré, docker ne doit pas bloquer" + ) + + (base / "lab.yaml").write_text( + (base / "lab.yaml").read_text(encoding="utf-8") + + " services:\n - name: db\n image: postgres:16\n", + encoding="utf-8") + + avec = collect_checks(tmp_path, None) + assert "docker" in [c.key for c in avec.required], ( + "dès qu'un lab déclare des services, docker devient requis" + ) + + +# ── Le bord : le tirage d'image, distinct du démarrage ────────────────────── + +def test_une_image_absente_est_tiree_avant_le_run( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Le premier `docker run` tirait l'image dans SON délai. + + Au-delà, la commande échouait sur un message de démarrage qui ne parlait pas + du réseau ; en deçà, `run` pendait plusieurs minutes sans rien dire. + """ + from dsoxlab.runtimes import services as svc + + appels: list[list[str]] = [] + + def _faux(cmd: list[str], **kwargs: Any) -> Any: + from dsoxlab.utils.shell import CommandResult + appels.append(cmd) + # L'image n'est pas locale, le pull réussit. + if cmd[:3] == ["docker", "image", "inspect"]: + return CommandResult(returncode=1, stdout="", stderr="") + return CommandResult(returncode=0, stdout="", stderr="") + + monkeypatch.setattr(svc, "run_command", _faux) + vus: list[str] = [] + svc._tirer("postgres:16", vus.append) + + assert ["docker", "pull", "postgres:16"] in appels + assert vus == ["postgres:16"], "le tirage doit se dire, sinon run a l'air figé" + + +def test_une_image_deja_locale_n_est_pas_retiree( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Sinon chaque `run` repasserait par le réseau.""" + from dsoxlab.runtimes import services as svc + + appels: list[list[str]] = [] + + def _faux(cmd: list[str], **kwargs: Any) -> Any: + from dsoxlab.utils.shell import CommandResult + appels.append(cmd) + return CommandResult(returncode=0, stdout="", stderr="") + + monkeypatch.setattr(svc, "run_command", _faux) + vus: list[str] = [] + svc._tirer("postgres:16", vus.append) + + assert not any(c[:2] == ["docker", "pull"] for c in appels) + assert vus == [], "rien à annoncer quand rien n'est téléchargé" + + +def test_le_delai_de_tirage_depasse_celui_du_demarrage() -> None: + """Une image de plusieurs gigaoctets sur un réseau partagé de formation. + + C'est le contexte réel du défaut : le délai d'un démarrage de conteneur n'a + aucune raison de borner un téléchargement. + """ + from dsoxlab.runtimes.services import _DELAI_TIRAGE + + assert _DELAI_TIRAGE >= 900 + + +# ── Le bord : une sonde qui expire se réessaie ────────────────────────────── + +def test_une_sonde_qui_expire_est_reessayee(monkeypatch: pytest.MonkeyPatch) -> None: + """La `CommandError` faisait sauter la boucle de réessai entière. + + Or c'est exactement ce qu'une sonde doit tolérer : un service qui n'est pas + encore prêt peut ne pas répondre du tout, pas seulement répondre mal. + """ + from dsoxlab.runtimes import services as svc + from dsoxlab.utils.shell import CommandResult + + essais = {"n": 0} + + def _faux(cmd: list[str], **kwargs: Any) -> CommandResult: + essais["n"] += 1 + if essais["n"] < 3: + return CommandResult(returncode=-1, stdout="", stderr="…", + failure=FAILURE_TIMEOUT) + return CommandResult(returncode=0, stdout="", stderr="") + + monkeypatch.setattr(svc, "run_command", _faux) + monkeypatch.setattr(svc.time, "sleep", lambda _: None) + + assert svc._wait_exec("c", ["vrai"], timeout=30) is True + assert essais["n"] == 3, "les deux délais dépassés devaient être réessayés" diff --git a/tests/test_doctor.py b/tests/test_doctor.py index 1bcfa73..93c2d1d 100644 --- a/tests/test_doctor.py +++ b/tests/test_doctor.py @@ -169,7 +169,8 @@ def test_shell_only_repo_never_shows_a_red_hypervisor( _labs(monkeypatch, [_lab("a", RuntimeType.SHELL)]) report = doctor.collect_checks(tmp_path, _repo()) - assert _labels(report.optional) == {_("check_kvm"), _("check_incus")} + assert _labels(report.optional) == {_("check_kvm"), _("check_incus"), + _("check_docker")} assert not report.failing() assert report.notes @@ -181,7 +182,7 @@ def test_active_provider_is_required_and_the_others_are_not( report = doctor.collect_checks(tmp_path, _repo(provider="kvm")) assert _("check_kvm") in _labels(report.required) - assert _labels(report.optional) == {_("check_incus")} + assert _labels(report.optional) == {_("check_incus"), _("check_docker")} assert [c.label for c in report.failing()] == [_("check_kvm")] @@ -192,7 +193,8 @@ def test_remote_provider_requires_no_local_hypervisor( _labs(monkeypatch, [_lab("a", RuntimeType.VM)]) report = doctor.collect_checks(tmp_path, _repo(provider="outscale")) - assert _labels(report.optional) == {_("check_kvm"), _("check_incus")} + assert _labels(report.optional) == {_("check_kvm"), _("check_incus"), + _("check_docker")} assert not report.failing() diff --git a/tests/test_json_output.py b/tests/test_json_output.py index e0d5a61..022bd71 100644 --- a/tests/test_json_output.py +++ b/tests/test_json_output.py @@ -319,7 +319,8 @@ def test_doctor_rend_des_cles_stables(catalogue: Path, sans_hyperviseur: None) - assert document["ok"] is True # Un catalogue 100 % shell : les hyperviseurs sont informatifs, et le kvm # en échec ne doit donc pas peindre le verdict en rouge. - assert [c["key"] for c in document["informational"]] == ["kvm", "incus"] + assert [c["key"] for c in document["informational"]] == [ + "docker", "kvm", "incus"] def test_un_correctif_expose_sa_categorie( diff --git a/tests/test_provision_et_json_services.py b/tests/test_provision_et_json_services.py index ffbe05e..e190a42 100644 --- a/tests/test_provision_et_json_services.py +++ b/tests/test_provision_et_json_services.py @@ -58,7 +58,8 @@ def test_le_demarrage_des_services_se_tait_en_mode_machine( # `_ensure_services` importe le module dans son corps : c'est donc le module # source qu'il faut patcher, pas un attribut de l'appelant. monkeypatch.setattr(svc, "docker_available", lambda: True) - monkeypatch.setattr(svc, "start", lambda service, repo: "conteneur") + monkeypatch.setattr(svc, "start", + lambda service, repo, **kw: "conteneur") _commun._ensure_services(lab, tmp_path, quiet=True) @@ -81,7 +82,8 @@ def test_le_demarrage_des_services_se_dit_en_mode_normal( # `_ensure_services` importe le module dans son corps : c'est donc le module # source qu'il faut patcher, pas un attribut de l'appelant. monkeypatch.setattr(svc, "docker_available", lambda: True) - monkeypatch.setattr(svc, "start", lambda service, repo: "conteneur") + monkeypatch.setattr(svc, "start", + lambda service, repo, **kw: "conteneur") _commun._ensure_services(lab, tmp_path, quiet=False) @@ -103,7 +105,7 @@ def test_un_service_en_echec_se_dit_meme_en_mode_machine( lab = _lab_avec_service(tmp_path) monkeypatch.setattr(svc, "docker_available", lambda: True) - def _echoue(service: object, repo: str) -> str: + def _echoue(service: object, repo: str, **kw: object) -> str: raise svc.ServiceError("le conteneur ne démarre pas") monkeypatch.setattr(svc, "start", _echoue) diff --git a/uv.lock b/uv.lock index 0b6e648..501dd3b 100644 --- a/uv.lock +++ b/uv.lock @@ -313,7 +313,7 @@ wheels = [ [[package]] name = "dsoxlab" -version = "0.1.76" +version = "0.1.77" source = { editable = "." } dependencies = [ { name = "ansible-core", version = "2.19.12", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.12'" }, From c65176e9d8dc90418f376af6d8519e08a53bd393 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Mon, 24 Aug 2026 18:32:19 +0200 Subject: [PATCH 04/11] =?UTF-8?q?fix(lab=5Fstate):=20un=20lab=20dont=20le?= =?UTF-8?q?=20moteur=20est=20injoignable=20ne=20s'affiche=20plus=20pr?= =?UTF-8?q?=C3=AAt=20(0.1.78)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _services_degrades rendait une liste vide quand docker_available() était faux. L'intention était juste — une machine sans Docker n'est pas un lab cassé — mais elle couvrait un cas de trop : « Docker était là et son démon est tombé » devenait indistinguable de « Docker n'a jamais été installé ». Dans le premier cas le lab est bel et bien injouable, et status annonçait pourtant ready, voire validated quand une note existait d'une session précédente. Le moteur est maintenant interrogé en amont, par calculer, et les deux causes se distinguent : docker absent du PATH, ou client présent avec un moteur muet. Elles appellent des gestes opposés — installer un paquet, ou démarrer un démon et vérifier que le compte peut lui parler — et confondre les deux envoie l'utilisateur chercher un paquet déjà présent. Renversement assumé, comme pour #177. Un test affirmait l'inverse : « une machine sans Docker n'est pas un lab cassé : on ne sait simplement rien ». Un lab qui DÉCLARE des services et dont le moteur ne répond pas est injouable, et run y échoue déjà explicitement en services_docker_absent, code 2 : annoncer ready contredisait la commande suivante. Le test est réécrit, porte le motif du renversement, et la moitié juste de la décision d'origine est gardée dans un second test — un lab sans service ignore le moteur, sans même payer une sonde. Les deux détails passent par des appels _() littéraux plutôt qu'une clé calculée : c'est ce qui permet au garde-fou i18n ajouté en 0.1.77 de vérifier qu'ils existent des deux côtés. Il l'a fait. Vérifié sur un lab réel de terraform-training : le même lab passe de validated à degraded avec la cause nommée, en français et en anglais, et sa note reste lisible. 816 tests dont 8 neufs, éprouvés par deux mutations, 18 e2e, ruff, mypy strict. Closes #179 Co-Authored-By: Claude Opus 5 --- CHANGELOG.fr.md | 18 +++ CHANGELOG.md | 18 +++ pyproject.toml | 2 +- src/dsoxlab/i18n/strings/en.py | 6 + src/dsoxlab/i18n/strings/fr.py | 6 + src/dsoxlab/services/lab_state.py | 46 ++++++- tests/test_lab_state.py | 41 ++++++- tests/test_moteur_injoignable.py | 198 ++++++++++++++++++++++++++++++ uv.lock | 2 +- 9 files changed, 331 insertions(+), 6 deletions(-) create mode 100644 tests/test_moteur_injoignable.py diff --git a/CHANGELOG.fr.md b/CHANGELOG.fr.md index ebd3552..ebfee6a 100644 --- a/CHANGELOG.fr.md +++ b/CHANGELOG.fr.md @@ -9,6 +9,24 @@ et le projet suit le [versionnage sémantique](https://semver.org/lang/fr/). ## [Non publié] +## [0.1.78] - 2026-08-24 + +### Corrigé + +- **Un lab dont le moteur de conteneurs est injoignable ne s'affiche plus + « prêt »** (issue #179). `_services_degrades` rendait une liste vide quand + `docker_available()` était faux : « Docker **était** là et son démon est + tombé » devenait donc indistinguable de « Docker n'a jamais été installé ». + Dans le premier cas le lab est bel et bien injouable — `run` y échoue déjà + explicitement, code 2 — et pourtant `status` annonçait `ready`, voire + `validated` lorsqu'une note existait d'une session précédente. +- Les deux causes se distinguent désormais, parce qu'elles appellent des gestes + opposés : installer un paquet, ou démarrer un démon et vérifier que le compte + peut lui parler. Un lab qui ne déclare **aucun** service continue d'ignorer le + moteur, sans même payer une sonde — c'était la moitié juste de la décision + d'origine, elle est gardée et testée pour elle-même. + + ## [0.1.77] - 2026-08-24 ### Corrigé diff --git a/CHANGELOG.md b/CHANGELOG.md index a9c0823..ce197ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.1.78] - 2026-08-24 + +### Fixed + +- **A lab whose container engine is unreachable no longer shows as ready** + (issue #179). `_services_degrades` returned an empty list when + `docker_available()` was false, so "Docker **was** there and its daemon went + down" was indistinguishable from "Docker was never installed". In the first + case the lab really is unplayable — `run` already fails on it explicitly with + exit code 2 — yet `status` announced `ready`, or even `validated` when a score + existed from an earlier session. +- The two causes are now told apart, because they call for opposite gestures: + install a package, versus start a daemon and check the account may talk to it. + A lab that declares **no** service still ignores the engine entirely, without + even paying for a probe — that was the sound half of the original decision, + and it is kept and tested on its own. + + ## [0.1.77] - 2026-08-24 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 369aad3..69af205 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "dsoxlab" -version = "0.1.77" +version = "0.1.78" description = "Turn declarative exercises into reproducible, runnable and verifiable lab environments" readme = "README.md" requires-python = ">=3.11" diff --git a/src/dsoxlab/i18n/strings/en.py b/src/dsoxlab/i18n/strings/en.py index d9fb059..edc7242 100644 --- a/src/dsoxlab/i18n/strings/en.py +++ b/src/dsoxlab/i18n/strings/en.py @@ -129,6 +129,12 @@ "lab_state_validated": "validated", "lab_state_validated_detail": "Score obtained: {score} / {max}", "lab_state_degraded": "degraded", + "lab_state_docker_absent": + "docker is not installed: this lab declares services and cannot be " + "played as is", + "lab_state_docker_muet": + "the Docker engine does not answer: this lab declares services and " + "cannot be played until it is back", "lab_state_degraded_detail": "A declared service is no longer running: {services}. " "Restart it with: dsoxlab run ", diff --git a/src/dsoxlab/i18n/strings/fr.py b/src/dsoxlab/i18n/strings/fr.py index ce1edff..c062d54 100644 --- a/src/dsoxlab/i18n/strings/fr.py +++ b/src/dsoxlab/i18n/strings/fr.py @@ -130,6 +130,12 @@ "lab_state_validated": "validé", "lab_state_validated_detail": "Note obtenue : {score} / {max}", "lab_state_degraded": "dégradé", + "lab_state_docker_absent": + "docker n'est pas installé : ce lab déclare des services et ne peut " + "pas être joué en l'état", + "lab_state_docker_muet": + "le moteur Docker ne répond pas : ce lab déclare des services et ne " + "peut pas être joué tant qu'il ne redémarre pas", "lab_state_degraded_detail": "Un service déclaré ne tourne plus : {services}. " "Relance-le : dsoxlab run ", diff --git a/src/dsoxlab/services/lab_state.py b/src/dsoxlab/services/lab_state.py index 8012429..77a80ae 100644 --- a/src/dsoxlab/services/lab_state.py +++ b/src/dsoxlab/services/lab_state.py @@ -28,6 +28,7 @@ from __future__ import annotations import hashlib +import shutil from dataclasses import dataclass from pathlib import Path @@ -141,19 +142,44 @@ def oublier_depart(root: Path, lab_id: str) -> None: return +#: Docker n'est pas installé sur cette machine. +MOTEUR_ABSENT = "absent" +#: Le client docker est là, mais le moteur ne répond pas. +MOTEUR_MUET = "muet" + + +def _moteur_indisponible() -> str | None: + """Pourquoi les services ne peuvent pas être observés, ou ``None`` s'ils le peuvent. + + Les deux causes appellent des gestes différents — installer Docker, ou + démarrer son démon et vérifier que le compte peut lui parler — donc elles se + distinguent. Elles ne se distinguaient pas : un ``docker_available()`` faux + valait « rien à signaler », si bien qu'un lab dont le démon venait de tomber + s'affichait ``ready``. + """ + from ..runtimes import services as svc + + if shutil.which("docker") is None: + return MOTEUR_ABSENT + if not svc.docker_available(): + return MOTEUR_MUET + return None + + def _services_degrades(lab: LabDefinition, repo_id: str) -> list[str]: """Les services déclarés qui ne tournent plus. On n'interroge Docker que si le lab en déclare : un catalogue sans service ne doit pas payer un appel, ni virer au rouge parce que Docker est absent d'une machine qui n'en a pas besoin. + + Le moteur injoignable est traité **en amont**, par ``calculer`` : ici, on + sait déjà qu'il répond, et la liste ne parle que des conteneurs. """ if not lab.runtime.services: return [] from ..runtimes import services as svc - if not svc.docker_available(): - return [] tombes: list[str] = [] for service in lab.runtime.services: etat = svc.status(service, repo_id) @@ -175,6 +201,22 @@ def calculer(root: Path, lab: LabDefinition, repo_id: str) -> LabState: scores = store.get_best_scores(root, [lab.id]) meilleur, maximum = scores.get(lab.id, (None, None)) + # Un lab qui DÉCLARE des services et dont le moteur ne répond pas est + # injouable, quelle que soit la note déjà obtenue : le dire avant de lire + # le score évite d'annoncer « validé » sur un lab qu'on ne peut plus jouer. + if lab.runtime.services: + cause = _moteur_indisponible() + if cause is not None: + # Deux appels littéraux plutôt qu'une clé calculée : c'est ce qui + # permet au garde-fou i18n de vérifier que les deux existent. + detail = (_("lab_state_docker_absent") if cause == MOTEUR_ABSENT + else _("lab_state_docker_muet")) + return LabState( + lab_id=lab.id, state=DEGRADED, + label=_("lab_state_degraded"), detail=detail, + best_score=meilleur, max_score=maximum, + ) + tombes = _services_degrades(lab, repo_id) if tombes: return LabState( diff --git a/tests/test_lab_state.py b/tests/test_lab_state.py index 5b8de86..2242969 100644 --- a/tests/test_lab_state.py +++ b/tests/test_lab_state.py @@ -207,10 +207,22 @@ def test_un_service_jamais_demarre_n_est_pas_une_degradation( assert lab_state.calculer(_depot(tmp_path), lab, "essai").state != lab_state.DEGRADED -def test_docker_absent_ne_degrade_aucun_lab( +def test_docker_absent_degrade_un_lab_qui_declare_des_services( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, ) -> None: - """Une machine sans Docker n'est pas un lab cassé : on ne sait simplement rien.""" + """Le renversement de #179, gardé explicite plutôt que discrètement effacé. + + Ce test affirmait l'inverse : « une machine sans Docker n'est pas un lab + cassé : on ne sait simplement rien. » L'intention était juste, mais elle + couvrait un cas de trop. Un lab qui **déclare** des services et dont le + moteur est injoignable est bel et bien injouable — `dsoxlab run` y échoue + déjà explicitement en `services_docker_absent`, code 2. Annoncer `ready` + contredisait donc la commande suivante. + + Ce que l'intention d'origine protégeait est gardé, et testé juste en + dessous : un lab qui ne déclare **aucun** service ignore le moteur, sans + même payer une sonde. + """ racine = _depot(tmp_path) / "labs" / "l1-demo" racine.mkdir(parents=True, exist_ok=True) (racine / "lab.yaml").write_text( @@ -225,6 +237,31 @@ def test_docker_absent_ne_degrade_aucun_lab( monkeypatch.setattr(svc, "docker_available", lambda: False) + etat = lab_state.calculer(_depot(tmp_path), lab, "essai") + assert etat.state == lab_state.DEGRADED + assert etat.detail, "l'état doit dire pourquoi, sinon il ne sert à rien" + + +def test_docker_absent_ne_degrade_pas_un_lab_sans_service( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """L'intention d'origine, à l'endroit exact où elle vaut. + + Un catalogue entièrement `shell` sans conteneur ne doit pas virer au rouge + pour un moteur qu'il n'utilise pas. + """ + racine = _depot(tmp_path) / "labs" / "l1-demo" + racine.mkdir(parents=True, exist_ok=True) + (racine / "lab.yaml").write_text( + _BASE + "runtime:\n type: shell\n workdir: challenge/work\n", + encoding="utf-8", + ) + lab = LabDefinition.from_yaml(racine / "lab.yaml") + + from dsoxlab.runtimes import services as svc + + monkeypatch.setattr(svc, "docker_available", lambda: False) + assert lab_state.calculer(_depot(tmp_path), lab, "essai").state != lab_state.DEGRADED diff --git a/tests/test_moteur_injoignable.py b/tests/test_moteur_injoignable.py new file mode 100644 index 0000000..f23ec24 --- /dev/null +++ b/tests/test_moteur_injoignable.py @@ -0,0 +1,198 @@ +"""Un lab dont le moteur est tombé ne s'affiche plus « prêt » (#179). + +`_services_degrades` rendait une liste vide quand `docker_available()` était +faux. L'intention était juste — une machine sans Docker n'est pas un lab cassé, +et c'était un choix documenté et testé — mais elle rendait « Docker **était** là +et son démon est tombé » indistinguable de « Docker n'a jamais été installé ». +Dans le premier cas, le lab est bel et bien injouable, et `dsoxlab status` +annonçait pourtant `ready` ou `validated`. + +Les deux causes se distinguent maintenant, parce qu'elles appellent des gestes +opposés : installer un paquet, ou démarrer un démon et vérifier que le compte +peut lui parler. + +Un lab qui ne déclare **aucun** service n'est pas concerné, et ce point compte +autant que les autres : c'est lui qui garantit qu'un catalogue entièrement +`shell` sans conteneur ne paie ni un appel, ni un faux rouge. +""" + +from __future__ import annotations + +import shutil +from pathlib import Path + +import pytest + +from dsoxlab.models.lab import LabDefinition +from dsoxlab.services import lab_state + +_ENTETE = """\ +id: l1-demo +title: Demo lab +level: beginner +skills: [demo] +distros: [any] +doc_url: https://example.org/docs/demo/ +runtime: + type: shell + workdir: challenge/work +""" + +_SERVICE = """\ + services: + - name: db + image: postgres:16 +""" + + +@pytest.fixture(autouse=True) +def xdg_jetable(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.setenv("XDG_STATE_HOME", str(tmp_path / "state")) + monkeypatch.setenv("XDG_DATA_HOME", str(tmp_path / "data")) + + +def _depot(tmp_path: Path) -> Path: + racine = tmp_path / "catalogue" + (racine / "labs").mkdir(parents=True, exist_ok=True) + (racine / "meta.yml").write_text("repo:\n id: essai\n category: essai\n", + encoding="utf-8") + return racine + + +def _lab(tmp_path: Path, *, avec_service: bool) -> LabDefinition: + base = _depot(tmp_path) / "labs" / "l1-demo" + base.mkdir(parents=True, exist_ok=True) + (base / "lab.yaml").write_text( + _ENTETE + (_SERVICE if avec_service else ""), encoding="utf-8") + return LabDefinition.from_yaml(base / "lab.yaml") + + +def _moteur(monkeypatch: pytest.MonkeyPatch, *, installe: bool, repond: bool) -> None: + """Simule l'état du moteur, sans dépendre de la machine qui joue les tests.""" + from dsoxlab.runtimes import services as svc + + monkeypatch.setattr(shutil, "which", + lambda nom: "/usr/bin/docker" if installe else None) + monkeypatch.setattr(svc, "docker_available", lambda: repond) + + +# ── Le défaut : un moteur tombé passait pour « rien à signaler » ──────────── + +def test_un_demon_tombe_rend_le_lab_degrade( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Le cas de l'issue : docker est installé, le démon ne répond plus.""" + racine = _depot(tmp_path) + lab = _lab(tmp_path, avec_service=True) + _moteur(monkeypatch, installe=True, repond=False) + + etat = lab_state.calculer(racine, lab, "essai") + + assert etat.state == lab_state.DEGRADED + + +def test_un_docker_jamais_installe_se_dit_autrement( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Les deux causes appellent des gestes opposés, donc elles se distinguent. + + Confondre « installe Docker » et « démarre son démon » envoie l'utilisateur + chercher un paquet déjà présent. + """ + racine = _depot(tmp_path) + lab = _lab(tmp_path, avec_service=True) + + _moteur(monkeypatch, installe=False, repond=False) + absent = lab_state.calculer(racine, lab, "essai") + + _moteur(monkeypatch, installe=True, repond=False) + muet = lab_state.calculer(racine, lab, "essai") + + assert absent.state == muet.state == lab_state.DEGRADED + assert absent.detail != muet.detail, "les deux causes doivent se lire" + + +def test_les_deux_details_sont_traduits( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Une clé absente des dictionnaires s'afficherait telle quelle.""" + racine = _depot(tmp_path) + lab = _lab(tmp_path, avec_service=True) + + for installe in (True, False): + _moteur(monkeypatch, installe=installe, repond=False) + detail = lab_state.calculer(racine, lab, "essai").detail + assert detail is not None + assert not detail.startswith("lab_state_"), f"clé non traduite : {detail}" + + +# ── Le lab n'est plus annoncé jouable ─────────────────────────────────────── + +def test_un_lab_deja_note_ne_passe_plus_pour_validated( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Une note obtenue hier ne rend pas le lab jouable aujourd'hui. + + C'est le cas qui trompait le plus : le score existait, donc `status` + annonçait « validé » sur un environnement qui ne démarre plus. + """ + from dsoxlab.services.lab_service import CheckResult, evaluate_lab + + racine = _depot(tmp_path) + lab = _lab(tmp_path, avec_service=True) + evaluate_lab(racine, lab, + CheckResult(ok=True, output="3 passed", passed=3, total=3)) + + _moteur(monkeypatch, installe=True, repond=False) + etat = lab_state.calculer(racine, lab, "essai") + + assert etat.state == lab_state.DEGRADED + assert etat.best_score is not None, "la note reste lisible, elle n'est pas perdue" + + +# ── L'autre bout : ne rien casser pour qui n'utilise pas Docker ──────────── + +def test_un_lab_sans_service_ignore_le_moteur( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """L'intention d'origine, préservée : pas de faux rouge, et pas d'appel. + + Un catalogue entièrement `shell` sans conteneur ne doit ni payer une sonde, + ni virer au rouge pour un moteur qu'il n'utilise pas. + """ + from dsoxlab.runtimes import services as svc + + racine = _depot(tmp_path) + lab = _lab(tmp_path, avec_service=False) + + appele = {"oui": False} + + def _sonde() -> bool: + appele["oui"] = True + return False + + monkeypatch.setattr(shutil, "which", lambda nom: None) + monkeypatch.setattr(svc, "docker_available", _sonde) + + etat = lab_state.calculer(racine, lab, "essai") + + assert etat.state != lab_state.DEGRADED + assert appele["oui"] is False, "aucun appel pour un lab qui ne déclare rien" + + +def test_un_moteur_qui_repond_laisse_l_etat_normal( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Le garde-fou du garde-fou : Docker debout ne dégrade rien.""" + from dsoxlab.runtimes import services as svc + + racine = _depot(tmp_path) + lab = _lab(tmp_path, avec_service=True) + _moteur(monkeypatch, installe=True, repond=True) + monkeypatch.setattr( + svc, "status", + lambda service, repo: type("E", (), {"detail": "absent"})()) + + etat = lab_state.calculer(racine, lab, "essai") + + assert etat.state != lab_state.DEGRADED diff --git a/uv.lock b/uv.lock index 501dd3b..a0c375b 100644 --- a/uv.lock +++ b/uv.lock @@ -313,7 +313,7 @@ wheels = [ [[package]] name = "dsoxlab" -version = "0.1.77" +version = "0.1.78" source = { editable = "." } dependencies = [ { name = "ansible-core", version = "2.19.12", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.12'" }, From 677838b269c075c7325678a47302dc6012dc3b16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Mon, 24 Aug 2026 18:45:43 +0200 Subject: [PATCH 05/11] feat(doctor): --strict traduit le diagnostic en code de sortie (0.1.79) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit doctor sortait en 0 quel que soit l'état de ses contrôles, y compris requis. Le choix était assumé en commentaire et il se défend pour un humain : un diagnostic n'est pas un échec. Mais il rendait la commande inutilisable comme portail automatisé, un script devant analyser le JSON pour savoir si quelque chose manquait. Deux codes plutôt qu'un, parce qu'il y a deux situations et qu'elles appellent des gestes différents. 9 quand un requis a échoué, c'est établi et cela se répare. 10 quand un requis n'a pas pu être mesuré, et cela se remesure. Un environnement dont une sonde n'a pas abouti n'est pas validé pour autant : c'est exactement ce qu'une construction d'image ne doit pas confondre avec un succès, et c'est le fil de tout ce lot d'issues. Quand les deux coexistent, 9 l'emporte, une certitude étant plus forte qu'une ignorance. failing() écarte délibérément les unknown pour ne pas peindre en rouge ce qu'on ignore, et c'est juste pour un tableau que lit un humain. indetermines() est son pendant, pour un appelant qui doit distinguer « c'est bon » de « je n'ai pas pu voir ». Le comportement par défaut ne bouge pas, c'est l'autre moitié du contrat, et deux tests le tiennent. --strict ne change rien d'autre : le tableau et le document restent rendus avant que le code ne tombe, comme validate-structure, pour qu'un appelant recevant un code non nul puisse encore lire ce qui n'allait pas. Un test le vérifie en lisant le JSON d'une invocation sortie en 9. Avec --fix, le rapport est recollecté avant le verdict : juger sur l'état d'avant ferait sortir en échec un --fix qui vient de réussir. Un informatif en échec ne fait pas échouer le portail : c'est l'invariant d'agnosticisme appliqué au code de sortie, un hyperviseur inutile ici n'ayant pas à bloquer un script. docs/machine-output EN + FR portent la table des codes, et la phrase du préambule qui affirmait « doctor sort en 0 » est corrigée dans les deux. fullhelp EN + FR décrivent l'option. Vérifié : 825 tests dont 9 neufs, éprouvés par trois mutations distinctes, 18 e2e, ruff, mypy strict, et les deux modes joués sur terraform-training avec un PATH sans git — défaut 0, --strict 9, --strict --json 9 avec le document lisible. Closes #176 Co-Authored-By: Claude Opus 5 --- CHANGELOG.fr.md | 29 +++++ CHANGELOG.md | 29 +++++ docs/machine-output.fr.md | 36 +++++- docs/machine-output.md | 35 +++++- pyproject.toml | 2 +- src/dsoxlab/cli/diagnostic.py | 40 +++++++ src/dsoxlab/i18n/strings/en.py | 5 + src/dsoxlab/i18n/strings/fr.py | 6 + src/dsoxlab/services/doctor.py | 21 ++++ tests/test_doctor_strict.py | 199 +++++++++++++++++++++++++++++++++ uv.lock | 2 +- 11 files changed, 398 insertions(+), 6 deletions(-) create mode 100644 tests/test_doctor_strict.py diff --git a/CHANGELOG.fr.md b/CHANGELOG.fr.md index ebfee6a..8d08c67 100644 --- a/CHANGELOG.fr.md +++ b/CHANGELOG.fr.md @@ -9,6 +9,35 @@ et le projet suit le [versionnage sémantique](https://semver.org/lang/fr/). ## [Non publié] +## [0.1.79] - 2026-08-24 + +### Ajouté + +- **`doctor --strict` traduit le diagnostic en code de sortie** (issue #176). + `doctor` sortait en 0 quel que soit l'état de ses contrôles, ce qui est le bon + choix pour un humain — un diagnostic n'est pas un échec — mais rendait la + commande inutilisable comme portail automatisé : un script devait analyser le + JSON pour savoir si quelque chose manquait. + + Deux codes plutôt qu'un, parce que les deux situations appellent des gestes + différents : + + | Mode | Code | Quand | + | --- | --- | --- | + | `doctor` | `0` | toujours, inchangé | + | `doctor --strict` | `9` | un contrôle requis a échoué | + | `doctor --strict` | `10` | un contrôle requis n'a pas pu être mesuré | + + `9` se répare, `10` se remesure. Un environnement dont une sonde n'a pas + abouti n'est pas validé pour autant — c'est précisément ce qu'une construction + d'image ne doit pas confondre avec un succès. `9` l'emporte quand les deux + coexistent : une certitude est plus forte qu'une ignorance. + + Le comportement par défaut ne bouge pas, et `--strict` ne change rien d'autre : + le tableau et le document restent rendus, **avant** que le code ne tombe, pour + qu'un appelant recevant un code non nul puisse encore lire ce qui n'allait pas. + + ## [0.1.78] - 2026-08-24 ### Corrigé diff --git a/CHANGELOG.md b/CHANGELOG.md index ce197ca..e4326fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,35 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.1.79] - 2026-08-24 + +### Added + +- **`doctor --strict` turns the diagnosis into an exit code** (issue #176). + `doctor` exited 0 whatever the state of its checks, which is the right call + for a human — a diagnosis is not a failure — but made the command unusable as + an automated gate: a script had to parse the JSON to learn whether anything + was missing. + + Two codes rather than one, because the two situations call for different + gestures: + + | Mode | Code | When | + | --- | --- | --- | + | `doctor` | `0` | always, unchanged | + | `doctor --strict` | `9` | a required check failed | + | `doctor --strict` | `10` | a required check could not be measured | + + `9` gets repaired, `10` gets measured again. An environment whose probe did + not complete is not validated for all that — which is exactly what an image + build must not mistake for a success. `9` wins when both coexist: a certainty + outweighs an ignorance. + + The default does not move, and `--strict` changes nothing else: the table and + the document are still rendered, **before** the code lands, so a caller + receiving a non-zero code can still read what went wrong. + + ## [0.1.78] - 2026-08-24 ### Fixed diff --git a/docs/machine-output.fr.md b/docs/machine-output.fr.md index 86f5efa..e79823d 100644 --- a/docs/machine-output.fr.md +++ b/docs/machine-output.fr.md @@ -35,7 +35,8 @@ traduit est posé *à côté*, pour l'affichage. Aucune intégration ne devrait Et une conséquence qui mérite d'être dite à part : **`--json` change la forme de la sortie, jamais le verdict ni le code de retour.** Un `check` sur un lab en échec sort en 1 avec ou sans lui ; `validate-structure` sort en 1 dès qu'un lab -échoue ; `doctor` sort en 0 dans les deux modes et met son verdict dans `ok`. +échoue ; `doctor` sort en 0 dans les deux modes et met son verdict dans `ok` +(c'est `--strict`, et non `--json`, qui en fait un code de sortie). Sur une erreur *dure* : identifiant de lab inconnu, `meta.yml` illisible, la sortie standard reste vide, la cause part sur la sortie d'erreur, et le code ne bouge pas. Lisez le code de retour d'abord. @@ -53,7 +54,7 @@ bouge pas. Lisez le code de retour d'abord. | `dsoxlab scores` | l'historique des notes et les verdicts d'examen | 0 | | `dsoxlab check ` | le résultat des tests et la note | 0, ou 1 si le lab échoue (le document est rendu quand même) | | `dsoxlab status` | la joignabilité SSH des hôtes déclarés | 0, ou 1 dès qu'un hôte déclaré ne répond pas (le document est rendu quand même) | -| `dsoxlab doctor` | le diagnostic de l'environnement | 0, toujours : le verdict est dans `ok` | +| `dsoxlab doctor` | le diagnostic de l'environnement | 0, toujours : le verdict est dans `ok`. Avec `--strict`, 9 (un requis échoue) ou 10 (un requis n'a pas pu être mesuré) | | `dsoxlab validate-structure` | chaque anomalie de contrat trouvée | 0, ou 1 dès qu'un lab échoue (le document est rendu quand même) | | `dsoxlab support` | le rapport de diagnostic anonymisé | 0 | @@ -395,6 +396,37 @@ une remédiation `manual` n'est jamais exécutée par `--fix`, et une rapporter `failed` jusqu'à la reconnexion ou au redémarrage. C'est un effet différé, pas un échec. +### Le code de sortie, et les deux modes + +Par défaut, `doctor` sort en **0 quoi qu'il arrive** : le verdict est dans `ok`. +C'est le bon choix pour un humain — un diagnostic n'est pas un échec — mais il +rend la commande inutilisable comme portail, un script devant lire le document +pour savoir si quelque chose manque. + +`--strict` traduit le diagnostic en code de sortie. Il ne change **rien** +d'autre : le tableau et le document restent rendus à l'identique, avant que le +code ne tombe. + +| Mode | Code | Quand | +| --- | --- | --- | +| `doctor` | `0` | toujours, y compris quand un requis échoue | +| `doctor --strict` | `0` | tous les contrôles requis sont `ok` | +| `doctor --strict` | `9` | au moins un requis est `failed` ou `choice_required` | +| `doctor --strict` | `10` | aucun échec, mais au moins un requis est `unknown` | + +Deux codes plutôt qu'un, parce que les deux situations appellent des gestes +différents : `9` se répare, `10` se remesure. Un environnement dont une sonde +n'a pas abouti n'est pas validé pour autant — c'est précisément ce qu'un script +de construction d'image ne doit pas confondre avec un succès. + +`9` l'emporte quand les deux coexistent : une certitude est plus forte qu'une +ignorance. + +`--strict` se combine à `--json`, et l'ordre compte : le document part sur la +sortie standard **avant** que le code ne soit rendu, exactement comme +`validate-structure`. Un appelant qui reçoit un code non nul peut donc encore +lire ce qui n'allait pas. + ## `validate-structure` ```json diff --git a/docs/machine-output.md b/docs/machine-output.md index 1b57090..31ddbe8 100644 --- a/docs/machine-output.md +++ b/docs/machine-output.md @@ -33,7 +33,8 @@ to know whether something is green or red. And one consequence worth stating on its own: **`--json` changes the shape of the output, never the verdict nor the exit code.** `check` on a failing lab exits 1 with or without it; `validate-structure` exits 1 as soon as one lab -fails; `doctor` exits 0 either way and puts its verdict in `ok`. On a *hard* +fails; `doctor` exits 0 either way and puts its verdict in `ok` +(it is `--strict`, not `--json`, that turns it into an exit code). On a *hard* error — an unknown lab id, an unreadable `meta.yml` — standard output stays empty, the reason goes to standard error, and the exit code is unchanged. Read the exit code first. @@ -51,7 +52,7 @@ the exit code first. | `dsoxlab scores` | the score history and exam verdicts | 0 | | `dsoxlab check ` | the test result and the score | 0, or 1 when the lab fails (document still printed) | | `dsoxlab status` | SSH reachability of the declared hosts | 0, or 1 when a declared host does not answer (document still printed) | -| `dsoxlab doctor` | the environment diagnosis | 0, always — the verdict is in `ok` | +| `dsoxlab doctor` | the environment diagnosis | 0, always — the verdict is in `ok`. With `--strict`, 9 (a required check fails) or 10 (a required check could not be measured) | | `dsoxlab validate-structure` | every contract issue found | 0, or 1 as soon as one lab fails (document still printed) | | `dsoxlab support` | the anonymised diagnostic report | 0 | @@ -390,6 +391,36 @@ remediation is never run by `--fix`, and a `needs_relogin` or `needs_reboot` one succeeds while its check keeps reporting `failed` until the session or the machine restarts — that is a delayed effect, not a failure. +### The exit code, and the two modes + +By default `doctor` exits **0 whatever happens**: the verdict lives in `ok`. +That is the right call for a human — a diagnosis is not a failure — but it made +the command useless as a gate, since a script had to read the document to learn +whether anything was missing. + +`--strict` turns the diagnosis into an exit code. It changes **nothing** else: +the table and the document are still rendered identically, before the code +lands. + +| Mode | Code | When | +| --- | --- | --- | +| `doctor` | `0` | always, including when a required check fails | +| `doctor --strict` | `0` | every required check is `ok` | +| `doctor --strict` | `9` | at least one required check is `failed` or `choice_required` | +| `doctor --strict` | `10` | no failure, but at least one required check is `unknown` | + +Two codes rather than one, because the two situations call for different +gestures: `9` gets repaired, `10` gets measured again. An environment whose +probe did not complete is not validated for all that — which is exactly what an +image build must not mistake for a success. + +`9` wins when both coexist: a certainty outweighs an ignorance. + +`--strict` combines with `--json`, and the order matters: the document goes to +standard output **before** the code is returned, exactly like +`validate-structure`. A caller receiving a non-zero code can still read what +went wrong. + ## `validate-structure` ```json diff --git a/pyproject.toml b/pyproject.toml index 69af205..32f2496 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "dsoxlab" -version = "0.1.78" +version = "0.1.79" description = "Turn declarative exercises into reproducible, runnable and verifiable lab environments" readme = "README.md" requires-python = ">=3.11" diff --git a/src/dsoxlab/cli/diagnostic.py b/src/dsoxlab/cli/diagnostic.py index 2b18d30..3c42eb7 100644 --- a/src/dsoxlab/cli/diagnostic.py +++ b/src/dsoxlab/cli/diagnostic.py @@ -46,6 +46,11 @@ FixKind, collect_checks, ) +from ..services.doctor import ( + EXIT_DOCTOR_INDETERMINE, + EXIT_DOCTOR_REQUIS_KO, + DoctorReport, +) from ..utils.shell import CommandError, run_command from ._commun import ( LabHomeOption, @@ -183,11 +188,31 @@ def install() -> None: # ── doctor ──────────────────────────────────────────────────────────────────── +def _verdict_strict(report: DoctorReport) -> None: + """Traduit le diagnostic en code de sortie, pour un appelant automatisé. + + Par défaut ``doctor`` sort en 0 quoi qu'il arrive, et c'est le bon choix + pour un humain : un diagnostic n'est pas un échec. Mais il rendait la + commande inutilisable comme portail — un script devait analyser le JSON + pour savoir si quelque chose manquait. + + Deux codes plutôt qu'un, parce qu'il y a deux situations et qu'elles + appellent des gestes différents : réparer ce qui manque, ou refaire une + mesure qui n'a pas abouti. L'échec établi l'emporte sur l'indéterminé, la + certitude étant l'information la plus forte des deux. + """ + if report.failing(): + raise typer.Exit(EXIT_DOCTOR_REQUIS_KO) + if report.indetermines(): + raise typer.Exit(EXIT_DOCTOR_INDETERMINE) + + @app.command("doctor", help=_("cmd_doctor_help")) def doctor( lab_home: LabHomeOption = None, fix: Annotated[bool, typer.Option("--fix", help=_("opt_fix"))] = False, as_json: Annotated[bool, typer.Option("--json", help=_("opt_json"))] = False, + strict: Annotated[bool, typer.Option("--strict", help=_("opt_doctor_strict"))] = False, ) -> None: root = _root(lab_home) @@ -218,6 +243,11 @@ def doctor( # contrôle échoue, et le verdict se lit dans `ok`. Un `--json` qui # inventerait un code non nul ferait diverger les deux modes. machine.emit(machine.doctor_dict(report)) + # Le document est rendu AVANT le verdict : `validate-structure` fait de + # même, et un appelant qui reçoit un code non nul doit quand même + # pouvoir lire ce qui n'allait pas. + if strict: + _verdict_strict(report) return print_doctor(report) @@ -228,6 +258,8 @@ def doctor( ] if not correctifs: info(_("fix_nothing")) + if strict: + _verdict_strict(report) return # Un correctif MANUAL n'est JAMAIS exécuté : le geste appartient à @@ -245,6 +277,8 @@ def doctor( for label, correctif in manuels: info(_("fix_manual", label=label, command=correctif.display)) if not executables: + if strict: + _verdict_strict(report) return # Pré-conditions sudo : si au moins un correctif passe par sudo, on @@ -290,6 +324,12 @@ def doctor( error(_("fix_failure", label=label, code=code)) info(_("fix_rerun")) + if strict: + # Après des correctifs, l'état de la machine a changé : juger sur le + # rapport d'avant dirait faux, et dans le mauvais sens — un `--fix` + # réussi sortirait quand même en échec. On remesure. + _verdict_strict(collect_checks(root, repo_meta) if fix else report) + def _executer_correctif(correctif: Fix) -> int: """Joue les commandes d'un correctif, dans l'ordre, sans shell. diff --git a/src/dsoxlab/i18n/strings/en.py b/src/dsoxlab/i18n/strings/en.py index edc7242..e4294ea 100644 --- a/src/dsoxlab/i18n/strings/en.py +++ b/src/dsoxlab/i18n/strings/en.py @@ -13,6 +13,8 @@ "opt_type": "Filter by type: lab, challenge or capstone", "opt_bloc": "Filter by bloc number (1-8)", "opt_top": "Number of displayed results", + "opt_doctor_strict": + "exit 9 if a required check fails, 10 if it could not be measured — for a script, not a human", "opt_fix": "Attempt automatic remediation of missing components.", "opt_no_pager": "Print everything at once instead of paging output longer than the screen.", "opt_use_provider": @@ -535,6 +537,9 @@ Informational components are left alone. [dim]--json[/dim] The diagnosis as a document, each check carrying a stable key and state. Not with [bold]--fix[/bold]. + [dim]--strict[/dim] Turn the diagnosis into an exit code, for a script: + [bold]9[/bold] if a required check fails, [bold]10[/bold] if it could not + be measured. Without it, doctor always exits 0. [cyan]demo[/cyan] Install a demonstration catalog and a first lab you can play right away, with nothing to clone or provision. diff --git a/src/dsoxlab/i18n/strings/fr.py b/src/dsoxlab/i18n/strings/fr.py index c062d54..1f5841e 100644 --- a/src/dsoxlab/i18n/strings/fr.py +++ b/src/dsoxlab/i18n/strings/fr.py @@ -13,6 +13,9 @@ "opt_type": "Filtre par type : lab, challenge ou capstone", "opt_bloc": "Filtre par numéro de bloc (1-8)", "opt_top": "Nombre de résultats affichés", + "opt_doctor_strict": + "sort en 9 si un contrôle requis échoue, en 10 s'il n'a pas pu être " + "mesuré — pour un script, pas pour un humain", "opt_fix": "Tenter la remédiation automatique des composants manquants.", "opt_no_pager": "Tout afficher d'un bloc au lieu de paginer ce qui dépasse l'écran.", "opt_use_provider": @@ -538,6 +541,9 @@ Les composants informatifs ne sont pas touchés. [dim]--json[/dim] Le diagnostic en document, chaque contrôle portant une clé et un état stables. Pas avec [bold]--fix[/bold]. + [dim]--strict[/dim] Traduit le diagnostic en code de sortie, pour un script : + [bold]9[/bold] si un requis échoue, [bold]10[/bold] s'il n'a pas pu être + mesuré. Sans elle, doctor sort toujours en 0. [cyan]demo[/cyan] Installe un catalogue de démonstration et un premier lab jouable immédiatement, sans rien cloner ni provisionner. diff --git a/src/dsoxlab/services/doctor.py b/src/dsoxlab/services/doctor.py index 5ba3bc6..a207ce0 100644 --- a/src/dsoxlab/services/doctor.py +++ b/src/dsoxlab/services/doctor.py @@ -65,6 +65,16 @@ STATE_OK = "ok" STATE_FAILED = "failed" STATE_CHOICE_REQUIRED = "choice_required" + +#: ``doctor --strict`` : un contrôle requis a échoué, c'est établi. +EXIT_DOCTOR_REQUIS_KO = 9 + +#: ``doctor --strict`` : un contrôle requis n'a **pas pu** être mesuré. Ce n'est +#: pas un échec, et ce n'est surtout pas un succès : un appelant automatisé qui +#: valide un environnement ne peut rien conclure d'une sonde qui n'a pas +#: regardé. Le code se distingue du précédent parce que les gestes diffèrent — +#: réparer, ou refaire la mesure. +EXIT_DOCTOR_INDETERMINE = 10 STATE_UNKNOWN = "unknown" """La sonde n'a pas pu mesurer : ni vert, ni rouge. @@ -233,6 +243,17 @@ def failing(self) -> list[Check]: if not c.ok and c.state != STATE_UNKNOWN ] + def indetermines(self) -> list[Check]: + """Les requis dont la sonde n'a rien pu établir. + + Le pendant de :meth:`failing` : celle-ci écarte les ``unknown`` pour ne + pas peindre l'affichage en rouge sur ce qu'on ignore, et c'est juste + pour un humain qui lit un tableau. Un script, lui, doit distinguer + « c'est bon » de « je n'ai pas pu voir », sans quoi il conclut au vert + sur une mesure qui n'a pas eu lieu. + """ + return [c for c in self.required if c.state == STATE_UNKNOWN] + def fixable(self) -> list[Check]: return [c for c in self.required if not c.ok and c.fix] diff --git a/tests/test_doctor_strict.py b/tests/test_doctor_strict.py new file mode 100644 index 0000000..ce35472 --- /dev/null +++ b/tests/test_doctor_strict.py @@ -0,0 +1,199 @@ +"""`doctor` devient utilisable comme portail, sans cesser d'être un diagnostic (#176). + +`cli/diagnostic.py` sortait en 0 quel que soit l'état des contrôles, y compris +« requis ». Le choix était assumé en commentaire, et il se défend pour un usage +interactif : un diagnostic n'est pas un échec. Mais il rendait `doctor` +inutilisable comme **portail automatisé** — un script devait analyser le JSON +pour savoir si quelque chose manquait. + +`--strict` traduit le diagnostic en code de sortie, et **deux** codes plutôt +qu'un : `9` quand un requis a échoué, `10` quand un requis n'a pas pu être +mesuré. Les deux appellent des gestes différents — réparer, ou refaire la +mesure — et surtout, un environnement dont une sonde n'a pas abouti n'est pas +validé pour autant. C'est exactement ce qu'une construction d'image ne doit pas +confondre avec un succès. + +Le comportement par défaut ne bouge pas : c'est la moitié du contrat. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from typer.testing import CliRunner + +from dsoxlab.cli import app +from dsoxlab.services.doctor import ( + EXIT_DOCTOR_INDETERMINE, + EXIT_DOCTOR_REQUIS_KO, + STATE_UNKNOWN, + Check, + DoctorReport, +) + +runner = CliRunner() + + +def _depot(tmp_path: Path) -> Path: + (tmp_path / "labs").mkdir(exist_ok=True) + (tmp_path / "meta.yml").write_text("repo:\n id: essai\n category: essai\n", + encoding="utf-8") + return tmp_path + + +def _check(key: str, *, ok: bool, state: str = "") -> Check: + """`state` est dérivé de `ok` ; seul `forced_state` le contraint.""" + return Check(key=key, label=key, ok=ok, detail="", + forced_state=state or None) + + +def _rapport(*checks: Check) -> DoctorReport: + rapport = DoctorReport() + rapport.required.extend(checks) + return rapport + + +def _imposer(monkeypatch: pytest.MonkeyPatch, rapport: DoctorReport) -> None: + """Fixe le diagnostic, pour ne pas dépendre de la machine qui joue les tests.""" + from dsoxlab.cli import diagnostic + + monkeypatch.setattr(diagnostic, "collect_checks", lambda root, meta: rapport) + + +# ── Les deux codes, un par situation ──────────────────────────────────────── + +def test_un_requis_en_echec_sort_en_neuf( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + _imposer(monkeypatch, _rapport(_check("python", ok=True), + _check("terraform", ok=False))) + + resultat = runner.invoke(app, ["doctor", "--strict", + "--lab-home", str(_depot(tmp_path))]) + + assert resultat.exit_code == EXIT_DOCTOR_REQUIS_KO + + +def test_un_requis_non_mesure_sort_en_dix( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Une sonde qui n'a rien pu établir ne vaut pas un feu vert. + + C'est la moitié du travail : sans ce code, un script conclurait « tout va + bien » sur une mesure qui n'a pas eu lieu. + """ + _imposer(monkeypatch, _rapport(_check("python", ok=True), + _check("libvirt_pool", ok=False, + state=STATE_UNKNOWN))) + + resultat = runner.invoke(app, ["doctor", "--strict", + "--lab-home", str(_depot(tmp_path))]) + + assert resultat.exit_code == EXIT_DOCTOR_INDETERMINE + + +def test_l_echec_etabli_l_emporte_sur_l_indetermine( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Une certitude est plus forte qu'une ignorance.""" + _imposer(monkeypatch, _rapport(_check("terraform", ok=False), + _check("libvirt_pool", ok=False, + state=STATE_UNKNOWN))) + + resultat = runner.invoke(app, ["doctor", "--strict", + "--lab-home", str(_depot(tmp_path))]) + + assert resultat.exit_code == EXIT_DOCTOR_REQUIS_KO + + +def test_un_environnement_sain_sort_en_zero( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + _imposer(monkeypatch, _rapport(_check("python", ok=True), + _check("pytest", ok=True))) + + resultat = runner.invoke(app, ["doctor", "--strict", + "--lab-home", str(_depot(tmp_path))]) + + assert resultat.exit_code == 0 + + +def test_un_informatif_en_echec_ne_fait_pas_echouer( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """`ok` ne porte que sur `required`, et `--strict` non plus. + + Un hyperviseur que ce catalogue n'utilisera jamais n'a pas à faire échouer + un portail. C'est l'invariant d'agnosticisme, appliqué au code de sortie. + """ + rapport = _rapport(_check("python", ok=True)) + rapport.optional.append(_check("incus", ok=False)) + _imposer(monkeypatch, rapport) + + resultat = runner.invoke(app, ["doctor", "--strict", + "--lab-home", str(_depot(tmp_path))]) + + assert resultat.exit_code == 0 + + +# ── Le défaut ne bouge pas : c'est l'autre moitié du contrat ─────────────── + +def test_sans_l_option_le_code_reste_zero( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Un diagnostic n'est pas un échec, et c'est un choix, pas un oubli.""" + _imposer(monkeypatch, _rapport(_check("terraform", ok=False))) + + resultat = runner.invoke(app, ["doctor", "--lab-home", str(_depot(tmp_path))]) + + assert resultat.exit_code == 0 + + +def test_sans_l_option_le_json_sort_en_zero_aussi( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """`--json` change la forme, jamais le verdict ni le code.""" + _imposer(monkeypatch, _rapport(_check("terraform", ok=False))) + + resultat = runner.invoke(app, ["doctor", "--json", + "--lab-home", str(_depot(tmp_path))]) + + assert resultat.exit_code == 0 + + +# ── Les deux options se combinent, et l'ordre compte ─────────────────────── + +def test_le_document_json_est_rendu_avant_le_code( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """Un appelant qui reçoit 9 doit encore pouvoir lire ce qui n'allait pas. + + Sortir avant d'écrire le document le priverait de la seule information qui + lui permet d'agir. + """ + import json + + _imposer(monkeypatch, _rapport(_check("python", ok=True), + _check("terraform", ok=False))) + + resultat = runner.invoke(app, ["doctor", "--strict", "--json", + "--lab-home", str(_depot(tmp_path))]) + + assert resultat.exit_code == EXIT_DOCTOR_REQUIS_KO + document = json.loads(resultat.stdout) + assert document["ok"] is False + assert [c["key"] for c in document["required"] if not c["ok"]] == ["terraform"] + + +def test_les_deux_codes_ne_collisionnent_avec_aucun_autre() -> None: + """Le projet donne un code dédié à chaque chemin d'échec : ils sont uniques.""" + from dsoxlab.infra.inventory import EXIT_HOTES_INJOIGNABLES + from dsoxlab.interrupt import EXIT_INTERRUPTED + from dsoxlab.locking import EXIT_LOCKED + + pris = {0, 1, 2, 5, 6, EXIT_LOCKED, EXIT_HOTES_INJOIGNABLES, EXIT_INTERRUPTED} + + assert EXIT_DOCTOR_REQUIS_KO not in pris + assert EXIT_DOCTOR_INDETERMINE not in pris + assert EXIT_DOCTOR_REQUIS_KO != EXIT_DOCTOR_INDETERMINE diff --git a/uv.lock b/uv.lock index a0c375b..bd8a432 100644 --- a/uv.lock +++ b/uv.lock @@ -313,7 +313,7 @@ wheels = [ [[package]] name = "dsoxlab" -version = "0.1.78" +version = "0.1.79" source = { editable = "." } dependencies = [ { name = "ansible-core", version = "2.19.12", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.12'" }, From 579465b8c96429a319f7cd4f0895a944ab695d67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Mon, 24 Aug 2026 18:59:14 +0200 Subject: [PATCH 06/11] =?UTF-8?q?ci(terraform):=20valider=20les=20trois=20?= =?UTF-8?q?templates=20packag=C3=A9s,=20et=20=C3=A9crire=20la=20d=C3=A9cis?= =?UTF-8?q?ion=20sur=20les=20images=20(0.1.80)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit La CI couvrait le lint, mypy, les tests unitaires, le fuzzing et une suite e2e sur la roue installée, mais ne jouait aucun terraform validate nulle part, aucun provisionnement, et sa suite e2e ne joue que le lab de démonstration, qui est shell. Les templates sont épinglés en ~> 0.9 (kvm) et ~> 0.3 (incus) : une version mineure du provider peut casser le schéma, et le dépôt a déjà vécu ce cas, les commentaires de kvm/main.tf documentant la rupture 0.8 → 0.9. Les tests unitaires existants sur ces templates sont des assertions textuelles : ils vérifient qu'un fichier contient une chaîne, pas que Terraform sait le lire. Une régression ne se découvrait donc que chez un apprenant, en langage Terraform. Le job joue init -backend=false puis validate sur les trois providers, collecte tous les échecs plutôt que de s'arrêter au premier, nomme le provider fautif par ::error::, et fait échouer la construction. Éprouvé en cassant réellement le template incus dans une copie du dépôt : deux providers restent valides, incus est nommé, code 1. Deux choix qui méritent d'être dits. Terraform vient de l'archive de l'éditeur avec la somme de contrôle qu'il publie, sur le patron déjà en place pour poutine : aucune action tierce n'entre dans la chaîne d'approvisionnement pour un binaire lancé deux fois, et la version est épinglée. Et la validation se fait en place, par -chdir : le template outscale atteint son cloud-init par ${path.module}/../../cloud-init/, si bien que copier les seuls .tf dans un répertoire isolé fait échouer un template parfaitement valide. C'est le faux rouge rencontré en écrivant ce job, et le commentaire du workflow le dit pour que personne ne le refasse. Aucun tfvars n'est nécessaire, et ce n'est pas un oubli : validate contrôle la configuration et ne résout jamais les valeurs de variables, seul plan le ferait et plan exige un hyperviseur joignable. Vérifié. La décision sur les images amont est écrite dans le README des templates. Les sept URL pointent des chemins mutables, et c'est un choix : une somme épinglée que personne ne tient à jour servirait aux apprenants une image de plus en plus périmée, avec ses vulnérabilités connues, et le durcissement deviendrait le vecteur du problème qu'il prétend traiter. Les raisons qui rendent le risque acceptable — HTTPS sur les domaines officiels, VM jetable, réseau local — et les conditions qui inverseraient la décision sont explicitées. Neuf tests gardent le job lui-même, parce qu'un job se supprime aussi silencieusement qu'un contrôle. Ils ne rejouent pas terraform : un test qui se sauterait faute de binaire rendrait un vert qui ne prouve rien. La liste des providers attendus est dérivée du disque, donc un quatrième provider fera rougir la suite tant qu'il ne sera pas couvert. Vérifié : 834 tests dont 9 neufs, éprouvés par trois mutations du workflow, ruff, mypy strict, et les trois analyseurs de chaîne CI joués localement — actionlint silencieux, zizmor « No findings », poutine « Passed » partout. Closes #175 Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 73 ++++++++++ CHANGELOG.fr.md | 34 +++++ CHANGELOG.md | 32 +++++ pyproject.toml | 2 +- src/dsoxlab/templates/terraform/README.md | 36 +++++ tests/test_ci_valide_les_templates.py | 155 ++++++++++++++++++++++ uv.lock | 2 +- 7 files changed, 332 insertions(+), 2 deletions(-) create mode 100644 tests/test_ci_valide_les_templates.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4dff415..7cc4287 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -139,6 +139,79 @@ jobs: - name: Poutine run: poutine analyze_local . --fail-on-violation --quiet + terraform: + name: Terraform templates (three providers) + needs: [zizmor, actionlint, poutine] + runs-on: ubuntu-24.04 + timeout-minutes: 10 + permissions: + contents: read + steps: + - name: Harden the runner + uses: step-security/harden-runner@b09bb98e06d4d774595224525879c09bc6e98c40 # v2.20.1 + with: + egress-policy: audit + + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + # Same pattern as poutine above: the vendor's own release archive, with + # the checksum it publishes alongside. No third-party action is added to + # the supply chain for a binary we only need to run twice. + - name: Install terraform (verified release binary) + env: + TERRAFORM_VERSION: "1.15.4" + run: | + set -euo pipefail + workdir="$(mktemp -d)" + base="https://releases.hashicorp.com/terraform/${TERRAFORM_VERSION}" + asset="terraform_${TERRAFORM_VERSION}_linux_amd64.zip" + curl -fsSL -o "${workdir}/${asset}" "${base}/${asset}" + curl -fsSL -o "${workdir}/checksums.txt" \ + "${base}/terraform_${TERRAFORM_VERSION}_SHA256SUMS" + (cd "${workdir}" && grep " ${asset}$" checksums.txt | sha256sum -c -) + unzip -q "${workdir}/${asset}" -d "${workdir}" + sudo install -m 0755 "${workdir}/terraform" /usr/local/bin/terraform + terraform version + + # The templates are pinned to `~> 0.9` (kvm) and `~> 0.3` (incus): a minor + # provider release can break the schema, and this repository has already + # lived through it — the comments in templates/terraform/kvm/main.tf + # document the 0.8 → 0.9 break. Nothing in CI could see it: the unit tests + # on these templates assert that a file contains a string, not that + # Terraform can read it. + # + # Validation runs IN PLACE, via -chdir. It matters: the outscale template + # reaches its cloud-init through `${path.module}/../../cloud-init/`, so + # copying the .tf files into an isolated directory reports a bogus failure + # on a template that is perfectly valid. + # + # No tfvars are needed, and that is not an oversight: `validate` checks + # the configuration itself and never resolves variable values — only + # `plan` would, and `plan` needs a reachable hypervisor. + - name: Validate every packaged provider + run: | + set -uo pipefail + broken="" + for provider in kvm incus outscale; do + dir="src/dsoxlab/templates/terraform/${provider}" + echo "::group::${provider}" + if terraform -chdir="${dir}" init -backend=false -input=false -no-color \ + && terraform -chdir="${dir}" validate -no-color; then + echo "${provider}: valid" + else + broken="${broken} ${provider}" + fi + echo "::endgroup::" + done + if [ -n "${broken}" ]; then + echo "::error::Invalid Terraform templates:${broken}" + exit 1 + fi + echo "All three packaged providers are valid." + quality: name: Lint, type-check and test (py${{ matrix.python-version }}) needs: [zizmor, actionlint, poutine] diff --git a/CHANGELOG.fr.md b/CHANGELOG.fr.md index 8d08c67..e3ee835 100644 --- a/CHANGELOG.fr.md +++ b/CHANGELOG.fr.md @@ -9,6 +9,40 @@ et le projet suit le [versionnage sémantique](https://semver.org/lang/fr/). ## [Non publié] +## [0.1.80] - 2026-08-24 + +### Ajouté + +- **La CI valide les trois templates Terraform packagés** (issue #175). Le + pipeline couvrait le lint, mypy, les tests unitaires, le fuzzing et une suite + e2e sur la roue installée — mais ne jouait **aucun `terraform validate` nulle + part**, aucun provisionnement, et sa suite e2e ne joue que le lab de + démonstration, qui est `shell`. Les templates sont épinglés en `~> 0.9` (kvm) + et `~> 0.3` (incus) : une version mineure du provider peut casser le schéma, + et le dépôt a déjà vécu ce cas. Les tests unitaires existants sur ces + templates vérifient qu'un fichier contient une chaîne, pas que Terraform sait + le lire : une régression de template ne se découvrait donc que chez un + apprenant, en langage Terraform. + + Le job joue `terraform init -backend=false` puis `terraform validate` sur kvm, + incus et outscale, collecte **tous** les échecs plutôt que de s'arrêter au + premier, nomme le provider fautif via `::error::`, et fait échouer la + construction. Terraform vient de l'archive de l'éditeur avec la somme de + contrôle qu'il publie, sur le patron déjà utilisé pour poutine — aucune action + tierce n'est ajoutée à la chaîne d'approvisionnement. + +### Documentation + +- **La décision sur les images amont est désormais écrite**, dans + `templates/terraform/README.md`. Les sept URL d'images pointent des chemins + mutables (`latest` / `current`), et c'est un choix : une somme épinglée que + personne ne tient à jour servirait aux apprenants une image de plus en plus + périmée, avec ses vulnérabilités connues — le durcissement deviendrait le + vecteur du problème qu'il prétend traiter. Les raisons qui rendent le risque + acceptable ici, et les conditions qui inverseraient la décision, sont + explicitées. + + ## [0.1.79] - 2026-08-24 ### Ajouté diff --git a/CHANGELOG.md b/CHANGELOG.md index e4326fa..383f42b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,38 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.1.80] - 2026-08-24 + +### Added + +- **CI validates the three packaged Terraform templates** (issue #175). The + pipeline covered lint, mypy, unit tests, fuzzing and an e2e suite on the + installed wheel — but ran **no `terraform validate` anywhere**, no + provisioning, and its e2e suite only plays the demo lab, which is `shell`. + The templates are pinned to `~> 0.9` (kvm) and `~> 0.3` (incus): a minor + provider release can break the schema, and this repository has already lived + through it. The existing unit tests on those templates assert that a file + contains a string, not that Terraform can read it, so a template regression + was only ever discovered on a learner's machine, in Terraform's own language. + + The job runs `terraform init -backend=false` then `terraform validate` on + kvm, incus and outscale, collects every failure rather than stopping at the + first, names the offending provider through `::error::`, and fails the build. + Terraform comes from the vendor's release archive with its published + checksum, following the pattern already used for poutine — no third-party + action is added to the supply chain. + +### Documentation + +- **The upstream-image decision is now written down**, in + `templates/terraform/README.md`. The seven image URLs point at mutable + `latest` / `current` paths, and that is a choice: pinning a checksum that + nobody keeps current would serve learners an increasingly stale image, with + its known vulnerabilities — the hardening would become the vector of the + problem it claims to address. The reasons it is acceptable here, and the + conditions that would reverse it, are spelled out. + + ## [0.1.79] - 2026-08-24 ### Added diff --git a/pyproject.toml b/pyproject.toml index 32f2496..67c03e0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "dsoxlab" -version = "0.1.79" +version = "0.1.80" description = "Turn declarative exercises into reproducible, runnable and verifiable lab environments" readme = "README.md" requires-python = ">=3.11" diff --git a/src/dsoxlab/templates/terraform/README.md b/src/dsoxlab/templates/terraform/README.md index cd2e5bf..cf28679 100644 --- a/src/dsoxlab/templates/terraform/README.md +++ b/src/dsoxlab/templates/terraform/README.md @@ -119,3 +119,39 @@ Compte utilisateur créé : `student` avec sudo NOPASSWD. | `proxmox` | `proxmox/` | ⏳ planifié | optionnel | ⏳ | | `vagrant` | `vagrant/` | ⏳ planifié | non | ⏳ | | `incus` | `incus/` | ⏳ planifié | non | ⏳ | + +## Images amont : le mutable est assumé, et voici pourquoi + +Les sept URL d'images des templates pointent des chemins **mutables** — +`latest` chez AlmaLinux, `current` chez Ubuntu et Debian. Aucune somme de +contrôle n'est épinglée, et c'est une décision, pas un oubli. + +**Ce que le mutable coûte.** Une image amont peut changer sous nos pieds : une +régression de l'éditeur, un cloud-init qui bouge, un paquet retiré. La +reproductibilité d'un `provision` n'est donc pas garantie dans le temps. + +**Ce qu'épingler coûterait, et pourquoi c'est pire ici.** Ubuntu et Debian +republient leurs images cloud toutes les quelques semaines, correctifs de +sécurité inclus. Une somme épinglée devrait être relevée à chaque fois ; non +faite, elle servirait aux apprenants une image de plus en plus périmée, avec ses +vulnérabilités connues. Le durcissement deviendrait alors le vecteur du +problème qu'il prétend traiter. + +**Ce qui rend le risque acceptable ici :** + +- les URL sont en HTTPS, sur les domaines officiels des trois distributions + (`cloud-images.ubuntu.com`, `cloud.debian.org`, `repo.almalinux.org`) : la + confiance repose sur TLS et sur l'éditeur, comme pour n'importe quel `apt + install` ; +- une VM de lab est **jetable** : elle vit le temps d'un exercice, ne porte + aucune donnée, et `destroy` la retire ; +- elle n'est **pas exposée** : le réseau libvirt est local à la machine de + l'apprenant. + +**Ce qui change la décision.** Si dsoxlab sert un jour des VM au-delà du +training — c'est la piste de `ROADMAP-VM-GENERALISTE` — le calcul s'inverse : +une machine qui dure et qui porte du travail mérite une image épinglée. Le +geste, alors : relever le `SHA256SUMS` publié par chaque distribution à côté de +son image, le porter dans un fichier versionné, et le vérifier après +téléchargement. Ce n'est pas fait aujourd'hui parce que ce n'est pas encore +justifié, pas parce que ce serait difficile. diff --git a/tests/test_ci_valide_les_templates.py b/tests/test_ci_valide_les_templates.py new file mode 100644 index 0000000..6cd3d4b --- /dev/null +++ b/tests/test_ci_valide_les_templates.py @@ -0,0 +1,155 @@ +"""La CI valide les templates Terraform, et rien ne peut le retirer en silence (#175). + +La CI couvrait le lint, mypy, les tests unitaires, le fuzzing et une suite e2e +sur la roue installée. Mais **aucun `terraform validate`** nulle part, aucun +provisionnement, et une suite e2e qui ne joue que le lab de démonstration — +lequel est `shell`. + +Les templates sont épinglés en `~> 0.9` (kvm) et `~> 0.3` (incus) : une version +mineure du provider peut casser le schéma, et **le dépôt a déjà vécu ce cas**, +les commentaires de `templates/terraform/kvm/main.tf` documentant la rupture +0.8 → 0.9. Les tests unitaires existants sur ces templates +(`test_kvm_disques_apparmor.py`, `test_cloud_init_templates.py`) sont des +assertions **textuelles** : ils vérifient qu'un fichier contient une chaîne, pas +que Terraform sait le lire. + +Ce module ne rejoue pas `terraform validate` — un test qui se sauterait faute de +binaire rendrait un vert qui ne prouve rien, et c'est précisément le défaut que +tout ce lot corrige. Il garde le **job**, pour qu'on ne puisse pas le retirer, le +renommer ou lui faire oublier un provider sans qu'un test rouge le dise. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +yaml = pytest.importorskip("yaml") + +RACINE = Path(__file__).resolve().parent.parent +CI = RACINE / ".github" / "workflows" / "ci.yml" +TEMPLATES = RACINE / "src" / "dsoxlab" / "templates" / "terraform" + + +def _workflow() -> dict: + return yaml.safe_load(CI.read_text(encoding="utf-8")) + + +def _job() -> dict: + jobs = _workflow()["jobs"] + assert "terraform" in jobs, ( + "le job qui valide les templates a disparu de la CI ; sans lui, une " + "régression de template se découvre chez un apprenant" + ) + return jobs["terraform"] + + +def _script_de_validation() -> str: + return "\n".join( + etape.get("run", "") for etape in _job()["steps"] if "run" in etape + ) + + +# ── Le job existe et couvre tout ce qui est packagé ───────────────────────── + +def test_la_ci_porte_un_job_de_validation_terraform() -> None: + assert _job()["steps"], "le job existe mais ne fait rien" + + +def test_les_providers_packages_sont_tous_valides() -> None: + """Le critère qui compte : ajouter un provider sans l'ajouter au job serait + livrer du Terraform que rien n'a jamais lu. + + La liste est dérivée du disque, pas écrite à la main : un quatrième provider + fait rougir ce test tant qu'il n'est pas couvert. + """ + packages = sorted( + chemin.name for chemin in TEMPLATES.iterdir() + if chemin.is_dir() and any(chemin.glob("*.tf")) + ) + script = _script_de_validation() + + manquants = [p for p in packages if p not in script] + assert manquants == [], ( + f"providers packagés mais absents du job CI : {manquants}" + ) + assert len(packages) >= 3, f"lecture du disque trop maigre : {packages}" + + +def test_le_job_joue_init_puis_validate() -> None: + """`validate` seul échouerait sur les providers non téléchargés.""" + script = _script_de_validation() + + assert "terraform" in script and "init" in script and "validate" in script + assert "-backend=false" in script, ( + "le backend n'a pas à être initialisé pour valider une configuration" + ) + + +def test_le_job_valide_en_place() -> None: + """`-chdir` plutôt qu'une copie, et ce n'est pas un détail de style. + + Le template outscale atteint son cloud-init par + `${path.module}/../../cloud-init/`. Copier les seuls `.tf` dans un + répertoire isolé casse ce chemin et fait échouer un template parfaitement + valide — un faux rouge, rencontré en écrivant ce job. + """ + assert "-chdir=" in _script_de_validation() + + +def test_le_job_nomme_le_provider_en_echec() -> None: + """Sinon il faut relire tout un journal pour savoir lequel des trois.""" + script = _script_de_validation() + + assert "::error::" in script + assert "broken" in script, "le job doit collecter les échecs, pas s'arrêter au premier" + + +# ── Le job est un portail, pas un rapport ─────────────────────────────────── + +def test_le_job_bloque_la_ci() -> None: + """Un job qui sort toujours en 0 informe, il ne garde rien.""" + assert "exit 1" in _script_de_validation() + + +def test_le_binaire_terraform_est_verifie() -> None: + """Même exigence que poutine : l'archive de l'éditeur et sa somme publiée. + + Aucune action tierce n'est ajoutée à la chaîne d'approvisionnement pour un + binaire qu'on ne lance que deux fois. + """ + script = _script_de_validation() + + assert "sha256sum -c" in script, "le binaire téléchargé doit être vérifié" + assert "releases.hashicorp.com" in script, "l'archive doit venir de l'éditeur" + + +def test_la_version_de_terraform_est_epinglee() -> None: + """`latest` ferait dépendre le verdict du jour où la CI tourne.""" + versions = [ + etape.get("env", {}).get("TERRAFORM_VERSION") + for etape in _job()["steps"] + ] + epinglee = next((v for v in versions if v), None) + + assert epinglee is not None, "la version de terraform n'est pas épinglée" + assert epinglee[0].isdigit(), f"version non littérale : {epinglee}" + + +# ── La décision sur les images amont est écrite ───────────────────────────── + +def test_le_choix_des_images_mutables_est_documente() -> None: + """Le troisième critère de l'issue : une décision écrite, pas un silence. + + Les URL d'images pointent des `latest` / `current` mutables. C'est un choix + défendable — une somme épinglée non tenue à jour servirait aux apprenants + une image de plus en plus vulnérable — mais un choix non écrit ne se + distingue pas d'un oubli. + """ + readme = (TEMPLATES / "README.md").read_text(encoding="utf-8") + + assert "mutable" in readme.lower() + for domaine in ("cloud-images.ubuntu.com", "cloud.debian.org", + "repo.almalinux.org"): + assert domaine in readme, f"{domaine} n'est pas couvert par la décision" diff --git a/uv.lock b/uv.lock index bd8a432..b8855d1 100644 --- a/uv.lock +++ b/uv.lock @@ -313,7 +313,7 @@ wheels = [ [[package]] name = "dsoxlab" -version = "0.1.79" +version = "0.1.80" source = { editable = "." } dependencies = [ { name = "ansible-core", version = "2.19.12", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.12'" }, From 6fe85861ceab9973b0f5f14980cbac5aee6b65b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Mon, 24 Aug 2026 19:09:27 +0200 Subject: [PATCH 07/11] =?UTF-8?q?test(ci):=20ancrer=20l'assertion=20d'URL?= =?UTF-8?q?=20plut=C3=B4t=20que=20supprimer=20l'alerte=20CodeQL?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL signalait py/incomplete-url-substring-sanitization en haute sévérité sur `assert "releases.hashicorp.com" in script`. Le risque est nul ici — c'est un test qui cherche une chaîne dans un script de workflow, pas une validation d'URL — mais le motif signalé est réellement dangereux ailleurs : « evil.com/releases.hashicorp.com » satisfait la même sous-chaîne. Plutôt que de supprimer l'alerte, l'assertion est ancrée sur l'assignation entière. Elle devient du même coup plus précise : elle vérifie que c'est bien `base` qui vaut cette URL, et non que le domaine est mentionné quelque part. La mutation qui l'éprouve mord toujours : remplacer l'URL par un autre hôte rend le test rouge. Co-Authored-By: Claude Opus 5 --- tests/test_ci_valide_les_templates.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/test_ci_valide_les_templates.py b/tests/test_ci_valide_les_templates.py index 6cd3d4b..19fd9b9 100644 --- a/tests/test_ci_valide_les_templates.py +++ b/tests/test_ci_valide_les_templates.py @@ -122,7 +122,15 @@ def test_le_binaire_terraform_est_verifie() -> None: script = _script_de_validation() assert "sha256sum -c" in script, "le binaire téléchargé doit être vérifié" - assert "releases.hashicorp.com" in script, "l'archive doit venir de l'éditeur" + # Ancré sur l'assignation entière plutôt que sur le seul nom de domaine. + # Chercher « releases.hashicorp.com » quelque part dans le script serait + # satisfait par « evil.com/releases.hashicorp.com », et c'est exactement le + # motif de sanitisation par sous-chaîne que CodeQL signale — à raison, même + # si le risque est nul dans un test. Autant écrire l'assertion juste. + assert any( + ligne.strip().startswith('base="https://releases.hashicorp.com/terraform/') + for ligne in script.splitlines() + ), "l'archive doit venir de l'éditeur, à une URL non ambiguë" def test_la_version_de_terraform_est_epinglee() -> None: From 3043e0f58b2d7e83efe604d297390d5eb3427a4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Mon, 24 Aug 2026 19:50:05 +0200 Subject: [PATCH 08/11] =?UTF-8?q?fix(infra):=20un=20cloud-init=20qui=20a?= =?UTF-8?q?=20mal=20fini=20le=20dit,=20au=20lieu=20d'=C3=AAtre=20jet=C3=A9?= =?UTF-8?q?=20(0.1.81)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit wait_for_hosts_ready jouait « cloud-init status --wait >/dev/null 2>&1 || true » : l'état et le code de retour partaient tous les deux à la poubelle. Hors ligne, derrière un proxy ou sur un miroir lent, les quinze paquets du premier démarrage ne s'installent pas, cloud-init finit en degraded, et l'hôte était tout de même déclaré prêt. Les labs échouaient ensuite sur des commandes absentes, sans que rien ne relie les deux. Ne pas bloquer reste la bonne décision, et elle est conservée : ce qui compte pour rendre la main, c'est que cloud-init ait terminé. Mais terminer mal doit se dire. L'attente remonte donc le code de retour et la sortie de status --long, que provision affiche à l'écran en nommant l'hôte — ce qui rend le geste de reprise immédiat. Le risque de ce mécanisme est qu'un marqueur change d'un côté et pas de l'autre : le lecteur rendrait alors None sur chaque hôte, donc un silence indistinguable d'un succès, c'est-à-dire le défaut même qu'on corrige. Un test fait parler les deux moitiés ensemble, et la commande est extraite en fonction pour qu'il éprouve ce qu'elle envoie plutôt que la façon dont le fichier est écrit. doctor gagne un contrôle egress, pour dire le problème avant de provisionner plutôt que trois labs plus tard. Les miroirs sondés sont lus dans les templates packagés, jamais écrits dans le moteur, et un test le vérifie sur la source : ce qui est en cause est l'accès sortant lui-même, donc un seul miroir joignable suffit à conclure — exiger les trois ferait rougir un poste sain dont un miroir est en maintenance. Requis sur un dépôt qui provisionne des VM, informatif sinon, vérifié sur les trois cas. La décision sur les paquets est écrite dans le README des cloud-init, avec ce qui a été écarté. Bloquer sur un degraded : cloud-init rend un état global, et traiterait un tree absent comme un lvm2 absent — un blocage sans granularité se contourne. Pré-cuire les images : c'est la vraie réponse, mais un projet à part entière. Déclarer les paquets lab par lab : ce serait le plus juste sur le papier, et cela changerait le contrat v1, gelé, pour un bénéfice que les deux autres mesures obtiennent sans rien casser. Mesuré avant d'être écrit : 15 paquets sur AlmaLinux, 14 sur Debian et Ubuntu. Vérifié : 847 tests dont 13 neufs, éprouvés par trois mutations dont celle du désaccord entre les deux moitiés, 18 e2e, ruff, mypy strict, et le contrôle egress joué dans les deux conditions réseau — failed sous coupure, ok hors coupure, ce qui prouve qu'il mesure au lieu d'échouer toujours. Closes #178 Co-Authored-By: Claude Opus 5 --- CHANGELOG.fr.md | 34 +++ CHANGELOG.md | 33 +++ pyproject.toml | 2 +- src/dsoxlab/cli/infrastructure.py | 17 +- src/dsoxlab/i18n/strings/en.py | 13 ++ src/dsoxlab/i18n/strings/fr.py | 14 ++ src/dsoxlab/infra/inventory.py | 77 ++++++- src/dsoxlab/services/doctor.py | 62 ++++++ src/dsoxlab/templates/cloud-init/README.md | 54 +++++ tests/test_cloud_init_degrade.py | 234 +++++++++++++++++++++ tests/test_doctor.py | 2 +- tests/test_json_output.py | 2 +- uv.lock | 2 +- 13 files changed, 535 insertions(+), 11 deletions(-) create mode 100644 src/dsoxlab/templates/cloud-init/README.md create mode 100644 tests/test_cloud_init_degrade.py diff --git a/CHANGELOG.fr.md b/CHANGELOG.fr.md index e3ee835..8c9d3a2 100644 --- a/CHANGELOG.fr.md +++ b/CHANGELOG.fr.md @@ -9,6 +9,40 @@ et le projet suit le [versionnage sémantique](https://semver.org/lang/fr/). ## [Non publié] +## [0.1.81] - 2026-08-24 + +### Corrigé + +- **Un cloud-init qui a mal fini le dit désormais** (issue #178). + `wait_for_hosts_ready` jouait `cloud-init status --wait >/dev/null 2>&1 || + true` : l'état **et** le code de retour partaient tous les deux à la poubelle. + Hors ligne, derrière un proxy ou sur un miroir lent, les quinze paquets du + premier démarrage ne s'installent pas, cloud-init finit en `degraded`, et + l'hôte était **tout de même déclaré prêt** — les labs échouaient ensuite sur + des commandes absentes, sans que rien ne relie les deux. Ne pas bloquer reste + la bonne décision : ce qui compte pour rendre la main, c'est que cloud-init + ait *terminé*. Mais terminer mal doit se dire. + +### Ajouté + +- **`doctor` gagne un contrôle `egress`.** Le provisionnement télécharge une + image, puis cloud-init installe des paquets : sans accès sortant, les deux + échouent. Les miroirs sondés sont **lus dans les templates packagés**, jamais + écrits dans le moteur, et un seul miroir joignable suffit à conclure — ce qui + est en cause est l'accès sortant lui-même, pas la disponibilité d'un miroir. + Requis sur un dépôt qui provisionne des VM, informatif sinon. + +### Documentation + +- **La décision sur les paquets du premier démarrage est écrite**, dans + `templates/cloud-init/README.md`. Bloquer sur un `degraded` a été écarté : + cloud-init rend un état global, et traiterait donc un `tree` absent comme un + `lvm2` absent. Les images pré-cuites sont la vraie réponse, mais un projet à + part entière. Déclarer les paquets lab par lab changerait le contrat v1, gelé. + Ce qui est retenu, c'est de rendre l'échec visible aux trois moments qui + comptent : avant, pendant, et dans le message qui nomme l'hôte. + + ## [0.1.80] - 2026-08-24 ### Ajouté diff --git a/CHANGELOG.md b/CHANGELOG.md index 383f42b..435c64d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,39 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.1.81] - 2026-08-24 + +### Fixed + +- **A cloud-init that finished badly now says so** (issue #178). + `wait_for_hosts_ready` ran `cloud-init status --wait >/dev/null 2>&1 || true`: + both the state *and* the return code went to the bin. Offline, behind a proxy + or on a slow mirror, the fifteen first-boot packages do not install, + cloud-init ends up `degraded`, and the host was **still declared ready** — + labs then failed on missing commands with nothing linking the two. Not + blocking remains the right call: what matters to hand back control is that + cloud-init has *finished*. But finishing badly must be said. + +### Added + +- **`doctor` gains an `egress` check.** Provisioning downloads an image, then + cloud-init installs packages: without outbound access both fail. The mirrors + it probes are **read from the packaged templates**, never written into the + engine, and a single reachable mirror is enough to conclude — what is at + stake is outbound access itself, not one mirror's availability. Required on a + repository that provisions VMs, informational otherwise. + +### Documentation + +- **The first-boot package decision is written down**, in + `templates/cloud-init/README.md`. Blocking on a `degraded` was rejected — + cloud-init reports a global state, so it would treat a missing `tree` like a + missing `lvm2`. Pre-baked images are the real answer but a project of their + own. Per-lab package declarations would change the frozen v1 contract. What + was retained is making the failure visible at the three moments that matter: + before, during, and in the message that names the host. + + ## [0.1.80] - 2026-08-24 ### Added diff --git a/pyproject.toml b/pyproject.toml index 67c03e0..a4a2ce0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "dsoxlab" -version = "0.1.80" +version = "0.1.81" description = "Turn declarative exercises into reproducible, runnable and verifiable lab environments" readme = "README.md" requires-python = ">=3.11" diff --git a/src/dsoxlab/cli/infrastructure.py b/src/dsoxlab/cli/infrastructure.py index 21706e0..0538947 100644 --- a/src/dsoxlab/cli/infrastructure.py +++ b/src/dsoxlab/cli/infrastructure.py @@ -187,6 +187,11 @@ def provision( for message in result.warnings: warn(message) + # Déclaré ici, avant tout branchement : quand il n'y a aucun hôte à + # attendre, le bloc plus bas n'est pas entré, et la boucle d'affichage + # doit quand même avoir une liste à parcourir. + avertissements_cloud_init: list[str] = [] + # Étape 3 : attendre que les VMs soient réellement joignables (sshd + # compte student + cloud-init terminé). Sans ça, le premier `dsoxlab run` # échoue en « unreachable » car la VM boote encore. @@ -226,7 +231,11 @@ def _on_attempt(fqdn: str, attempt: int) -> None: try: with interruptible(Stage.HOSTS_WAIT): - wait_for_hosts_ready( + # Les hôtes sont joignables, mais leur cloud-init a + # peut-être mal fini : sans ces avertissements, les labs + # échoueraient plus tard sur des paquets absents, sans que + # rien ne relie l'échec au provisionnement. + avertissements_cloud_init = wait_for_hosts_ready( repo_meta, ready_hosts, on_attempt=_on_attempt ) except HostReadyTimeout as exc: @@ -245,6 +254,12 @@ def _on_attempt(fqdn: str, attempt: int) -> None: progress.stop() _interrompu(exc, "dsoxlab provision") + # Les hôtes répondent, mais leur configuration a peut-être mal fini. Le dire + # ici, à l'écran, et non dans un journal : c'est la seule occasion de relier + # un paquet absent au provisionnement qui n'a pas pu l'installer. + for message in avertissements_cloud_init: + warn(message) + # Le fragment SSH, écrit à CHAQUE provision et non seulement quand des # machines viennent d'être créées : relancer un provision sur une infra # déjà en place laissait sinon l'apprenant sans fragment, alors que c'est diff --git a/src/dsoxlab/i18n/strings/en.py b/src/dsoxlab/i18n/strings/en.py index e4294ea..91ea4fc 100644 --- a/src/dsoxlab/i18n/strings/en.py +++ b/src/dsoxlab/i18n/strings/en.py @@ -933,6 +933,14 @@ # ── git and docker: two tools nothing declared ────────────────────────── "check_git": "git", "check_docker": "Docker", + "check_egress": "Outbound access", + "detail_egress_absent": + "no image mirror is reachable ({hosts}): provisioning will download an " + "image and cloud-init will install packages, both will fail", + "detail_egress_indetermine": + "no template declares an image: nothing can be concluded", + "reason_egress_sans_vm": + "Outbound access is informational: this repository provisions no VM.", "detail_git_missing": "git is missing: 'dsoxlab catalog add' clones a repository and cannot " "work without it", @@ -1212,6 +1220,11 @@ "err_inventory_role_unknown": "role '{role}' → '{fqdn}' is not in the list of known hosts: {known} " "(host not declared in meta.yml, or not provisioned).", + "cloud_init_degrade": + "{host}: cloud-init finished with an error (code {code}). The packages " + "it installs on first boot may be missing, and labs relying on them " + "will fail. Detail:\n{detail}", + "cloud_init_sans_detail": "cloud-init said nothing more.", "err_host_ready_timeout": "{fqdn} is unreachable over SSH after {timeout}s " "(cloud-init too slow, or the VM failed to boot).", diff --git a/src/dsoxlab/i18n/strings/fr.py b/src/dsoxlab/i18n/strings/fr.py index 1f5841e..ba42619 100644 --- a/src/dsoxlab/i18n/strings/fr.py +++ b/src/dsoxlab/i18n/strings/fr.py @@ -943,6 +943,15 @@ # ── git et docker : deux outils que rien ne déclarait ─────────────────── "check_git": "git", "check_docker": "Docker", + "check_egress": "Accès sortant", + "detail_egress_absent": + "aucun miroir d'images n'est joignable ({hosts}) : le provisionnement " + "téléchargera une image et cloud-init installera des paquets, les deux " + "échoueront", + "detail_egress_indetermine": + "aucun template ne déclare d'image : impossible de conclure", + "reason_egress_sans_vm": + "L'accès sortant est informatif : ce dépôt ne provisionne aucune VM.", "detail_git_missing": "git est absent : « dsoxlab catalog add » clone un dépôt et ne peut " "pas fonctionner sans lui", @@ -1233,6 +1242,11 @@ "err_inventory_role_unknown": "role « {role} » → « {fqdn} » n'est pas dans la liste des hôtes " "connus : {known} (host non déclaré dans meta.yml, ou non provisionné).", + "cloud_init_degrade": + "{host} : cloud-init a terminé en erreur (code {code}). Les paquets " + "qu'il installe au premier démarrage manquent peut-être, et les labs " + "qui en dépendent échoueront. Détail :\n{detail}", + "cloud_init_sans_detail": "cloud-init n'a rien dit de plus.", "err_host_ready_timeout": "{fqdn} injoignable en SSH après {timeout}s " "(cloud-init trop long, ou VM en échec de démarrage).", diff --git a/src/dsoxlab/infra/inventory.py b/src/dsoxlab/infra/inventory.py index f0224d6..c98d5b3 100644 --- a/src/dsoxlab/infra/inventory.py +++ b/src/dsoxlab/infra/inventory.py @@ -445,6 +445,46 @@ def _reset_kvm_domain(repo_meta: RepoMetadata, fqdn: str) -> bool: return False +#: Préfixe de la ligne par laquelle l'hôte distant remonte le code de retour de +#: ``cloud-init status --wait``. Un marqueur explicite plutôt qu'une position : +#: la sortie de ``status --long`` change d'une version à l'autre. +_MARQUEUR_RC = "__dsoxlab_cloudinit_rc=" + + +def _etat_cloud_init(sortie: str) -> tuple[int | None, str]: + """Le code de retour de cloud-init et ce qu'il a dit, lus dans la sortie SSH. + + Rend ``(None, "")`` quand l'hôte n'a pas de cloud-init : ce n'est pas un + défaut, seulement une image qui n'en embarque pas. + """ + code: int | None = None + detail: list[str] = [] + for ligne in sortie.splitlines(): + if ligne.startswith(_MARQUEUR_RC): + valeur = ligne[len(_MARQUEUR_RC):].strip() + code = int(valeur) if valeur.isdigit() else None + continue + if code is not None and ligne.strip(): + detail.append(ligne.strip()) + return code, "\n".join(detail[:12]) + + +def _commande_cloud_init() -> str: + """La commande jouée sur l'hôte pour attendre cloud-init ET lire son état. + + Extraite plutôt qu'inline : ce qu'elle envoie est un contrat — un marqueur + de code de retour, et la sortie de ``status --long`` — que les tests + éprouvent en l'appelant, sans relire le fichier source. + """ + return ( + "command -v cloud-init >/dev/null 2>&1 || exit 0\n" + "sudo -n cloud-init status --wait >/dev/null 2>&1\n" + f"echo \"{_MARQUEUR_RC}$?\"\n" + "sudo -n cloud-init status --long 2>&1 || true\n" + "exit 0\n" + ) + + def wait_for_hosts_ready( repo_meta: RepoMetadata, hosts: list[str], @@ -454,7 +494,7 @@ def wait_for_hosts_ready( connect_timeout: int = 8, reset_after: float = 60.0, on_attempt: Callable[[str, int], None] | None = None, -) -> None: +) -> list[str]: """Attend que chaque host soit réellement utilisable après ``terraform apply``. Juste après le provisioning, la VM boote encore : ``sshd`` démarre, puis @@ -474,11 +514,16 @@ def wait_for_hosts_ready( connect_timeout: ``ConnectTimeout`` SSH de chaque tentative, en secondes. on_attempt: callback ``(fqdn, numéro_tentative)`` pour l'affichage. + Returns: + Les avertissements constatés pendant l'attente — un cloud-init qui a + terminé en erreur, notamment. La liste est vide quand tout s'est bien + passé ; l'appelant les affiche, il n'a pas à les déduire. + Raises: HostReadyTimeout: si un host reste injoignable au-delà de ``timeout``. """ if not hosts: - return + return [] timeout = _host_ready_timeout(timeout) @@ -486,6 +531,7 @@ def wait_for_hosts_ready( repo_meta, terraform_outputs=read_terraform_outputs(repo_meta) ) ssh_cfg = write_ssh_config(inventory, repo_meta) + avertissements: list[str] = [] # SSH réussit dès que sshd répond ET que le compte ansible existe ; on enchaîne sur # `cloud-init status --wait` (best-effort) pour ne rendre la main qu'une fois @@ -497,10 +543,21 @@ def wait_for_hosts_ready( # rc=1. Le `|| true` avalait cet échec et l'attente ne garantissait plus rien : # on rendait la main avant la fin de cloud-init, en croyant l'avoir attendue. # `-n` (non interactif) évite de bloquer si sudo réclamait un mot de passe. - remote_cmd = ( - "command -v cloud-init >/dev/null 2>&1 " - "&& sudo -n cloud-init status --wait >/dev/null 2>&1 || true" - ) + # `--wait` bloque jusqu'à la FIN de cloud-init, et son code de retour dit + # comment il a fini. L'ancienne forme jetait les deux (`>/dev/null` et + # `|| true`) : un cloud-init `degraded` — une quinzaine de paquets qui + # n'ont pas pu s'installer parce qu'il n'y avait pas d'accès sortant — + # devenait indistinguable d'un succès. L'hôte était déclaré prêt, puis les + # labs échouaient sur des commandes absentes, sans que rien ne relie les + # deux. + # + # Ne pas bloquer reste la bonne décision : ce qui compte pour rendre la + # main, c'est que cloud-init ait TERMINÉ. Mais terminer mal doit se dire. + # + # `sudo -n` est indispensable : sans privilèges, la commande sort en + # `PermissionError: /run/cloud-init/cloud.cfg` (mesuré sur AlmaLinux 9). + # `-n` évite de bloquer si sudo réclamait un mot de passe. + remote_cmd = _commande_cloud_init() for fqdn in hosts: start = time.monotonic() @@ -533,6 +590,12 @@ def wait_for_hosts_ready( ) if proc.returncode == 0: logger.info("Host %s prêt (tentative %d).", fqdn, attempt) + code, detail = _etat_cloud_init(proc.stdout) + if code is not None and code != 0: + avertissements.append(_( + "cloud_init_degrade", host=fqdn, code=code, + detail=detail or _("cloud_init_sans_detail"), + )) break if time.monotonic() >= deadline: raise HostReadyTimeout(_( @@ -548,6 +611,8 @@ def wait_for_hosts_ready( reset_done = _reset_kvm_domain(repo_meta, fqdn) or True time.sleep(poll_interval) + return avertissements + def write_inventory_file( inventory: dict[str, Any], repo_meta: RepoMetadata diff --git a/src/dsoxlab/services/doctor.py b/src/dsoxlab/services/doctor.py index a207ce0..20ea100 100644 --- a/src/dsoxlab/services/doctor.py +++ b/src/dsoxlab/services/doctor.py @@ -31,6 +31,7 @@ import re import shlex import shutil +import socket import subprocess import sys from collections.abc import Sequence @@ -990,6 +991,58 @@ def _check_docker() -> Check: return _check("docker", True, result.stdout.strip() or "ok") +#: Les URL d'images des templates packagés. Le moteur ne **connaît** aucun +#: domaine : il les lit dans les templates, comme il lit le reste du contrat. +_URL_IMAGE = re.compile(r'https://([A-Za-z0-9.-]+)/[^"\s]*\.(?:qcow2|img)') + +#: Une sonde d'accès sortant doit être brève : `doctor` en enchaîne déjà +#: plusieurs, et un réseau coupé se constate en deux secondes. +_DELAI_EGRESS = 2.0 + + +def _hotes_images() -> list[str]: + """Les hôtes que le provisionnement ira chercher, lus dans les templates.""" + racine = Path(__file__).resolve().parent.parent / "templates" / "terraform" + hotes: list[str] = [] + for chemin in sorted(racine.rglob("*.tf")): + try: + contenu = chemin.read_text(encoding="utf-8") + except OSError: + continue + for hote in _URL_IMAGE.findall(contenu): + if hote not in hotes: + hotes.append(hote) + return hotes + + +def _check_egress() -> Check: + """Le provisionnement télécharge une image, puis cloud-init des paquets. + + Sans accès sortant — salle de formation fermée, proxy d'entreprise — le + téléchargement échoue ou cloud-init finit en `degraded`, et les labs + échouent plus tard sur des commandes absentes. Le dire **avant** de + provisionner coûte deux secondes et évite de chercher la panne au mauvais + endroit. + + Un seul hôte joignable suffit à conclure : ce qui est en cause est l'accès + sortant lui-même, pas la disponibilité d'un miroir en particulier. + """ + hotes = _hotes_images() + if not hotes: + # Aucun template ne déclare d'image : rien à joindre, rien à conclure. + return _check("egress", False, _("detail_egress_indetermine"), + forced_state=STATE_UNKNOWN) + for hote in hotes: + try: + with socket.create_connection((hote, 443), timeout=_DELAI_EGRESS): + return _check("egress", True, hote) + except OSError: + continue + return _check("egress", False, + _("detail_egress_absent", hosts=", ".join(hotes)), + hint="https://docs.docker.com/network/proxy/") + + def _hypervisor_checks() -> dict[str, Check]: return {"kvm": _check_kvm(), "incus": _check_incus()} @@ -1023,6 +1076,15 @@ def collect_checks(root: Path, repo_meta: RepoMetadata | None) -> DoctorReport: report.notes.append(_("reason_docker_no_services")) needs_vm = uses_vm(labs) + # Le provisionnement télécharge une image puis laisse cloud-init installer + # des paquets : sans accès sortant, l'hôte est déclaré prêt et les labs + # échouent plus tard sur des commandes absentes. Un dépôt entièrement + # `shell` ne provisionne rien, donc n'a pas à en voir du rouge. + if needs_vm: + report.required.append(_check_egress()) + else: + report.optional.append(_check_egress()) + report.notes.append(_("reason_egress_sans_vm")) active = repo_meta.infra.provider if repo_meta else "" candidates = list(repo_meta.infra.providers_available) if repo_meta else [] hypervisors = _hypervisor_checks() diff --git a/src/dsoxlab/templates/cloud-init/README.md b/src/dsoxlab/templates/cloud-init/README.md new file mode 100644 index 0000000..069f17f --- /dev/null +++ b/src/dsoxlab/templates/cloud-init/README.md @@ -0,0 +1,54 @@ +# Les paquets installés au premier démarrage — la décision, et pourquoi + +Chaque template pose un bloc `packages:` installé au **premier boot** de la +machine : 15 entrées pour AlmaLinux, 14 pour Debian et Ubuntu. C'est autant de +dépendances réseau, au moment le plus fragile du cycle de vie d'un lab. + +## Ce que ça coûte + +Hors ligne, derrière un proxy d'entreprise, ou sur un miroir lent, cloud-init +finit en `degraded`. Le symptôme était le pire possible : `provision` annonçait +des hôtes **prêts**, puis les labs échouaient sur des commandes absentes, et +rien ne reliait les deux. C'est aussi ce qui gonfle le premier démarrage dans le +budget d'attente de `wait_for_hosts_ready`. + +## Ce qui a été décidé, et ce qui a été écarté + +**Écarté — bloquer sur un `degraded`.** cloud-init ne dit pas *quel* paquet a +manqué : il rend un état global. Faire échouer `provision` traiterait `tree` +absent comme `lvm2` absent, alors que le premier n'empêche aucun lab et le +second en casse toute une famille. Un blocage sans granularité se contourne, et +un garde-fou qu'on contourne ne garde plus rien. + +**Écarté pour l'instant — pré-cuire les images.** C'est la vraie réponse : un +paquet déjà présent n'est pas une dépendance réseau. Mais construire et publier +des images est un projet à part entière, avec son propre cycle de vie et sa +propre chaîne d'approvisionnement. Il vit hors de ce dépôt. + +**Écarté — déclarer les paquets lab par lab.** Ce serait la solution la plus +juste sur le papier : chaque lab annonce ce dont il a besoin, et `run` le +vérifie. Cela change le contrat `lab.yaml` de la v1, gelé, pour un bénéfice que +les deux mesures ci-dessous obtiennent sans rien casser. + +**Retenu — rendre l'échec visible, aux trois moments où il compte :** + +1. **Avant.** Le contrôle `egress` de `dsoxlab doctor` joint les miroirs + d'images déclarés par les templates. Sur un dépôt qui provisionne des VM il + est **requis** : une salle sans accès sortant se voit avant de lancer quoi + que ce soit, pas trois labs plus tard. +2. **Pendant.** `wait_for_hosts_ready` lit désormais le code de retour de + `cloud-init status --wait` et sa sortie `--long`, au lieu de les jeter dans + `/dev/null`. Un hôte dont la configuration a mal fini le **dit à l'écran**, + avec ce que cloud-init a rapporté. +3. **Après.** Le message nomme l'hôte, ce qui rend le geste de reprise + immédiat : `ssh ` puis `sudo cloud-init status --long` pour lire le + détail, et l'installation manuelle du paquet manquant. + +Rendre la main reste la bonne décision — ce qui compte pour continuer, c'est que +cloud-init ait **terminé**. Mais terminer mal doit se dire. + +## Ce qui changerait la décision + +Le jour où les images sont pré-cuites, ce bloc `packages:` se vide de tout ce +qui n'est pas propre à la machine, et le contrôle `egress` cesse d'être requis +pour provisionner. C'est le sens de la marche, pas un contournement de plus. diff --git a/tests/test_cloud_init_degrade.py b/tests/test_cloud_init_degrade.py new file mode 100644 index 0000000..74b8e9a --- /dev/null +++ b/tests/test_cloud_init_degrade.py @@ -0,0 +1,234 @@ +"""Un cloud-init qui a mal fini se dit, au lieu d'être jeté (#178). + +`wait_for_hosts_ready` lançait `cloud-init status --wait >/dev/null 2>&1 || +true` : l'état **et** le code de retour partaient tous les deux à la poubelle. +Hors ligne, derrière un proxy ou sur un miroir lent, les quinze paquets du +premier boot ne s'installaient pas, cloud-init finissait en `degraded`, et +l'hôte était **tout de même déclaré prêt**. Les labs échouaient ensuite sur des +commandes absentes, sans que rien ne relie les deux. + +Ne pas bloquer reste la bonne décision — ce qui compte pour rendre la main, +c'est que cloud-init ait *terminé*. Mais terminer mal doit se dire, et à trois +moments : avant (le contrôle `egress`), pendant (l'avertissement), après (le +geste de reprise, dans le message). +""" + +from __future__ import annotations + +import socket +from pathlib import Path +from typing import Self + +import pytest + +from dsoxlab.infra.inventory import _etat_cloud_init + +# ── La lecture de ce que l'hôte a répondu ─────────────────────────────────── + +def test_un_cloud_init_reussi_ne_dit_rien() -> None: + code, detail = _etat_cloud_init( + "__dsoxlab_cloudinit_rc=0\nstatus: done\nextended_status: done\n" + ) + + assert code == 0 + assert "done" in detail + + +def test_un_cloud_init_degrade_est_lu_avec_son_detail() -> None: + """Le cas de l'issue : cloud-init a fini, mais mal.""" + code, detail = _etat_cloud_init( + "__dsoxlab_cloudinit_rc=2\n" + "status: degraded done\n" + "errors:\n" + " - Package installation failed: firewalld\n" + ) + + assert code == 2 + assert "degraded" in detail + assert "firewalld" in detail, "le détail doit nommer ce qui a échoué" + + +def test_un_hote_sans_cloud_init_n_est_pas_un_defaut() -> None: + """Une image qui n'embarque pas cloud-init n'a rien à signaler. + + Sans ce cas, chaque hôte d'une telle image produirait un avertissement que + rien ne justifie — et un avertissement systématique cesse d'être lu. + """ + code, detail = _etat_cloud_init("") + + assert code is None + assert detail == "" + + +def test_une_sortie_illisible_ne_fait_pas_lever() -> None: + """La sortie de `status --long` change d'une version à l'autre. + + Un parseur qui lève sur une forme inattendue ferait planter un + provisionnement réussi. + """ + code, _detail = _etat_cloud_init("__dsoxlab_cloudinit_rc=inattendu\nbruit\n") + + assert code is None + + +def test_le_detail_est_borne() -> None: + """`status --long` peut rendre des dizaines de lignes ; un mur de texte + dans un terminal ne se lit pas plus qu'un journal.""" + sortie = "__dsoxlab_cloudinit_rc=2\n" + "\n".join( + f"ligne {n}" for n in range(200) + ) + + _, detail = _etat_cloud_init(sortie) + + assert len(detail.splitlines()) <= 12 + + +# ── La commande distante remonte ce qu'il faut ────────────────────────────── + +def test_la_commande_distante_ne_jette_plus_l_etat() -> None: + """Le défaut tenait en deux redirections : `>/dev/null 2>&1 || true`. + + La commande est appelée, pas relue dans le fichier : c'est ce qu'elle + **envoie** qui est le contrat, et une assertion sur la source aurait été + satisfaite par du texte que le programme ne produit jamais. + """ + from dsoxlab.infra.inventory import _MARQUEUR_RC, _commande_cloud_init + + commande = _commande_cloud_init() + + assert "--wait" in commande, "il faut toujours attendre la fin" + assert "--long" in commande, "l'état doit être demandé, pas seulement attendu" + assert _MARQUEUR_RC in commande, ( + "le code de retour doit remonter, sinon on ne sait pas comment ça a fini" + ) + assert "sudo -n" in commande, ( + "sans privilèges, cloud-init sort en PermissionError et l'attente ne " + "garantit plus rien" + ) + + +def test_la_commande_et_le_lecteur_parlent_la_meme_langue() -> None: + """Les deux moitiés doivent s'accorder, sinon l'état est perdu en silence. + + C'est le seul vrai risque de ce mécanisme : un marqueur changé d'un côté + et pas de l'autre rendrait `code = None` sur chaque hôte, donc un silence + indistinguable d'un succès — le défaut même que cette issue corrige. + """ + from dsoxlab.infra.inventory import _commande_cloud_init + + # Ce que l'hôte renverrait si cloud-init avait fini en degraded. + sortie = _commande_cloud_init().splitlines()[2].replace( + 'echo "', "").replace('"', "").replace("$?", "2") + + code, _detail = _etat_cloud_init(sortie + "\nstatus: degraded done") + + assert code == 2 + + +# ── Le contrôle d'accès sortant : avant, plutôt que trois labs plus tard ──── + +def test_les_hotes_sondes_viennent_des_templates() -> None: + """Le moteur ne connaît aucun domaine : il les lit dans ce qui est packagé. + + Écrire `cloud-images.ubuntu.com` dans `doctor.py` serait exactement le + couplage que le projet s'interdit. + """ + from dsoxlab.services.doctor import _hotes_images + + hotes = _hotes_images() + + assert len(hotes) >= 3, f"lecture des templates trop maigre : {hotes}" + assert all("/" not in h for h in hotes), "ce sont des hôtes, pas des URL" + + +def test_le_moteur_ne_cite_aucun_miroir_en_dur() -> None: + """L'invariant, vérifié sur la source plutôt que sur la bonne volonté.""" + from dsoxlab.services import doctor + + source = Path(doctor.__file__).read_text(encoding="utf-8") + + for miroir in ("cloud-images.ubuntu.com", "cloud.debian.org", + "repo.almalinux.org"): + assert miroir not in source, f"« {miroir} » est écrit en dur dans doctor.py" + + +def test_un_acces_sortant_coupe_se_voit(monkeypatch: pytest.MonkeyPatch) -> None: + from dsoxlab.services.doctor import _check_egress + + def _refuse(*args: object, **kwargs: object) -> None: + raise OSError("Network is unreachable") + + monkeypatch.setattr(socket, "create_connection", _refuse) + + assert _check_egress().ok is False + + +def test_un_seul_miroir_joignable_suffit(monkeypatch: pytest.MonkeyPatch) -> None: + """Ce qui est en cause est l'accès sortant, pas un miroir en particulier. + + Exiger que les trois répondent ferait rougir un poste parfaitement + fonctionnel dont un seul miroir est en maintenance. + """ + from dsoxlab.services.doctor import _check_egress + + class _Fausse: + """Un contexte qui ne fait rien : seule sa réussite compte ici.""" + + def __enter__(self) -> Self: + return self + + def __exit__(self, *args: object) -> None: + return None + + essais = {"n": 0} + + def _un_seul(adresse: tuple[str, int], **kwargs: object) -> _Fausse: + essais["n"] += 1 + if essais["n"] == 1: + raise OSError("refusé") + return _Fausse() + + monkeypatch.setattr(socket, "create_connection", _un_seul) + + assert _check_egress().ok is True + + +def test_le_controle_suit_ce_que_le_depot_provisionne(tmp_path: Path) -> None: + """Requis si le dépôt a des labs `vm`, informatif sinon. + + Un catalogue entièrement `shell` ne provisionne rien : lui montrer du rouge + pour un accès sortant qu'il n'utilise pas serait le contre-exemple même de + l'agnosticisme. + """ + from dsoxlab.services.doctor import collect_checks + + (tmp_path / "meta.yml").write_text("repo:\n id: essai\n category: essai\n", + encoding="utf-8") + base = tmp_path / "labs" / "l1" + base.mkdir(parents=True) + (base / "lab.yaml").write_text( + "id: l1\ntitle: T\nlevel: l1\nskills: [s]\ndistros: [any]\n" + "doc_url: https://example.org/\n" + "runtime:\n type: shell\n workdir: challenge/work\n", + encoding="utf-8") + + rapport = collect_checks(tmp_path, None) + + assert "egress" in [c.key for c in rapport.optional] + assert "egress" not in [c.key for c in rapport.required] + + +# ── La décision est écrite ────────────────────────────────────────────────── + +def test_la_decision_sur_les_paquets_est_ecrite() -> None: + """Le deuxième critère de l'issue : un choix non écrit ne se distingue pas + d'un oubli, et celui-ci a coûté des labs injouables en salle.""" + from dsoxlab.services import doctor + + readme = (Path(doctor.__file__).resolve().parent.parent + / "templates" / "cloud-init" / "README.md") + + texte = readme.read_text(encoding="utf-8") + assert "degraded" in texte + for ecarte in ("bloquer", "pré-cuire", "lab par lab"): + assert ecarte in texte, f"l'option « {ecarte} » n'est pas tranchée" diff --git a/tests/test_doctor.py b/tests/test_doctor.py index 93c2d1d..9c47a26 100644 --- a/tests/test_doctor.py +++ b/tests/test_doctor.py @@ -170,7 +170,7 @@ def test_shell_only_repo_never_shows_a_red_hypervisor( report = doctor.collect_checks(tmp_path, _repo()) assert _labels(report.optional) == {_("check_kvm"), _("check_incus"), - _("check_docker")} + _("check_docker"), _("check_egress")} assert not report.failing() assert report.notes diff --git a/tests/test_json_output.py b/tests/test_json_output.py index 022bd71..3cac425 100644 --- a/tests/test_json_output.py +++ b/tests/test_json_output.py @@ -320,7 +320,7 @@ def test_doctor_rend_des_cles_stables(catalogue: Path, sans_hyperviseur: None) - # Un catalogue 100 % shell : les hyperviseurs sont informatifs, et le kvm # en échec ne doit donc pas peindre le verdict en rouge. assert [c["key"] for c in document["informational"]] == [ - "docker", "kvm", "incus"] + "docker", "egress", "kvm", "incus"] def test_un_correctif_expose_sa_categorie( diff --git a/uv.lock b/uv.lock index b8855d1..ce17965 100644 --- a/uv.lock +++ b/uv.lock @@ -313,7 +313,7 @@ wheels = [ [[package]] name = "dsoxlab" -version = "0.1.80" +version = "0.1.81" source = { editable = "." } dependencies = [ { name = "ansible-core", version = "2.19.12", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.12'" }, From 075cc052bf42e75404438f721b7e76fddd1fb980 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Mon, 24 Aug 2026 20:11:45 +0200 Subject: [PATCH 09/11] =?UTF-8?q?test(doctor):=20la=20sonde=20d'acc=C3=A8s?= =?UTF-8?q?=20sortant=20ne=20fait=20plus=20de=20vraies=20connexions=20en?= =?UTF-8?q?=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Défaut introduit par le commit précédent, trouvé en jouant la suite sous charge : le contrôle egress ouvrait de VRAIES connexions pendant les tests unitaires. Six fichiers appellent collect_checks sans le savoir, chacun payant jusqu'à trois délais d'attente et rendant un verdict différent selon que le réseau répondait ou non. Les mêmes tests passaient sur un runner GitHub et échouaient derrière un pare-feu — c'est-à-dire qu'ils ne mesuraient plus ce qu'ils prétendaient. test_doctor.py portait déjà la règle, et son commentaire disait le défaut mot pour mot : « sans cela, ces tests mesurent la machine qui les exécute : ils passaient en local et échouaient sur un runner CI qui ne les a pas ». La sonde réseau rouvrait la brèche, en pire. La sonde est extraite en _joignable(), et un tests/conftest.py la neutralise pour toute la suite. Neutraliser socket.create_connection aurait été plus court et faux : runtimes/services.py s'en sert pour attendre qu'un conteneur écoute, et le rendre toujours vrai ferait passer des tests de services qui ne prouveraient plus rien. Les deux tests qui éprouvent le contrôle lui-même repatchent _joignable par-dessus. Vérifié dans les deux sens, ce qui est le seul contrôle qui vaille ici : sans le conftest et réseau coupé, deux tests de doctor échouent ; avec, la suite rend 847 verts réseau coupé ET réseau ouvert. Le verdict ne dépend plus du réseau. test_doctor.py passe au passage de plusieurs secondes à 0,21 s. Co-Authored-By: Claude Opus 5 --- src/dsoxlab/services/doctor.py | 28 ++++++++++++++++++++----- tests/conftest.py | 36 ++++++++++++++++++++++++++++++++ tests/test_cloud_init_degrade.py | 35 ++++++++++--------------------- 3 files changed, 70 insertions(+), 29 deletions(-) create mode 100644 tests/conftest.py diff --git a/src/dsoxlab/services/doctor.py b/src/dsoxlab/services/doctor.py index 20ea100..015678f 100644 --- a/src/dsoxlab/services/doctor.py +++ b/src/dsoxlab/services/doctor.py @@ -1015,6 +1015,27 @@ def _hotes_images() -> list[str]: return hotes +def _joignable(hote: str) -> bool: + """Une connexion sortante vers cet hôte aboutit-elle ? + + Isolée en fonction parce que c'est le seul appel **réseau** de tout + `collect_checks`. Les tests la neutralisent d'un coup (`tests/conftest.py`), + faute de quoi chaque test de classement ouvrirait de vraies connexions : + lentes, dépendantes du réseau du moment, et fausses dès qu'une CI est + fermée. C'est le défaut que la fixture voisine corrigeait déjà pour + terraform et ansible — « ces tests mesurent la machine qui les exécute ». + + Patcher ``socket`` globalement ne convenait pas : ``runtimes/services.py`` + s'en sert pour attendre un conteneur, et le neutraliser rendrait cette + attente-là toujours vraie. + """ + try: + with socket.create_connection((hote, 443), timeout=_DELAI_EGRESS): + return True + except OSError: + return False + + def _check_egress() -> Check: """Le provisionnement télécharge une image, puis cloud-init des paquets. @@ -1033,11 +1054,8 @@ def _check_egress() -> Check: return _check("egress", False, _("detail_egress_indetermine"), forced_state=STATE_UNKNOWN) for hote in hotes: - try: - with socket.create_connection((hote, 443), timeout=_DELAI_EGRESS): - return _check("egress", True, hote) - except OSError: - continue + if _joignable(hote): + return _check("egress", True, hote) return _check("egress", False, _("detail_egress_absent", hosts=", ".join(hotes)), hint="https://docs.docker.com/network/proxy/") diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..3391b13 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,36 @@ +"""Réglages communs à toute la suite unitaire. + +Un test unitaire ne doit mesurer que le code, jamais la machine qui l'exécute ni +le réseau du moment. `test_doctor.py` porte déjà cette règle pour terraform, +ansible et les prérequis matériels — « sans cela, ces tests mesurent la machine +qui les exécute : ils passaient en local et échouaient sur un runner CI ». + +Le contrôle d'accès sortant ajouté en 0.1.81 a rouvert la brèche, et plus +largement : il ouvrait de **vraies connexions**. Six fichiers de tests +appelaient `collect_checks` sans le savoir, chacun payant jusqu'à trois délais +d'attente et rendant un verdict différent selon que le réseau répondait ou non. +Les mêmes tests passaient sur un runner GitHub et échouaient derrière un +pare-feu — c'est-à-dire qu'ils ne mesuraient plus ce qu'ils prétendaient. +""" + +from __future__ import annotations + +import pytest + + +@pytest.fixture(autouse=True) +def _aucun_acces_reseau(monkeypatch: pytest.MonkeyPatch) -> None: + """Neutralise l'unique sonde réseau de `doctor`, pour toute la suite. + + On neutralise `_joignable`, pas `socket.create_connection` : ce dernier sert + aussi à `runtimes/services.py` pour attendre qu'un conteneur écoute, et le + rendre toujours vrai ferait passer des tests de services qui ne prouveraient + plus rien. + + Un test qui veut éprouver le contrôle lui-même repatche `_joignable` par + dessus — c'est ce que fait `test_cloud_init_degrade.py`, et le monkeypatch + le plus récent l'emporte. + """ + from dsoxlab.services import doctor + + monkeypatch.setattr(doctor, "_joignable", lambda hote: True) diff --git a/tests/test_cloud_init_degrade.py b/tests/test_cloud_init_degrade.py index 74b8e9a..e84859b 100644 --- a/tests/test_cloud_init_degrade.py +++ b/tests/test_cloud_init_degrade.py @@ -15,9 +15,7 @@ from __future__ import annotations -import socket from pathlib import Path -from typing import Self import pytest @@ -153,14 +151,11 @@ def test_le_moteur_ne_cite_aucun_miroir_en_dur() -> None: def test_un_acces_sortant_coupe_se_voit(monkeypatch: pytest.MonkeyPatch) -> None: - from dsoxlab.services.doctor import _check_egress - - def _refuse(*args: object, **kwargs: object) -> None: - raise OSError("Network is unreachable") + from dsoxlab.services import doctor - monkeypatch.setattr(socket, "create_connection", _refuse) + monkeypatch.setattr(doctor, "_joignable", lambda hote: False) - assert _check_egress().ok is False + assert doctor._check_egress().ok is False def test_un_seul_miroir_joignable_suffit(monkeypatch: pytest.MonkeyPatch) -> None: @@ -169,28 +164,20 @@ def test_un_seul_miroir_joignable_suffit(monkeypatch: pytest.MonkeyPatch) -> Non Exiger que les trois répondent ferait rougir un poste parfaitement fonctionnel dont un seul miroir est en maintenance. """ - from dsoxlab.services.doctor import _check_egress - - class _Fausse: - """Un contexte qui ne fait rien : seule sa réussite compte ici.""" - - def __enter__(self) -> Self: - return self - - def __exit__(self, *args: object) -> None: - return None + from dsoxlab.services import doctor essais = {"n": 0} - def _un_seul(adresse: tuple[str, int], **kwargs: object) -> _Fausse: + def _un_seul(hote: str) -> bool: essais["n"] += 1 - if essais["n"] == 1: - raise OSError("refusé") - return _Fausse() + return essais["n"] > 1 + + monkeypatch.setattr(doctor, "_joignable", _un_seul) - monkeypatch.setattr(socket, "create_connection", _un_seul) + resultat = doctor._check_egress() - assert _check_egress().ok is True + assert resultat.ok is True + assert essais["n"] == 2, "le premier miroir refusé doit être suivi du second" def test_le_controle_suit_ce_que_le_depot_provisionne(tmp_path: Path) -> None: From 5f9be00748843565361c5339ee38dabc6ccbf219 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Mon, 24 Aug 2026 20:20:03 +0200 Subject: [PATCH 10/11] =?UTF-8?q?test(services):=20un=20test=20=C3=A0=20co?= =?UTF-8?q?nteneur=20r=C3=A9el=20ne=20d=C3=A9marre=20plus=20sans=20sonde?= =?UTF-8?q?=20(0.1.82)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deux tests de test_services.py échouaient par intermittence, toujours sous charge, jamais au repos. Leur point commun : ils attendaient un vrai conteneur. Sans sonde, start rend la main dès que docker run a répondu, ce qui ne dit rien de l'état du service à l'intérieur — l'étape suivante devient une course, gagnée au repos et perdue sous charge. Ce que la mesure a rendu, et il faut le dire tel quel : l'instabilité ne se reproduit plus. Sous charge Docker soutenue, 12 exécutions ciblées et 4 suites complètes, aucun échec. Le ready_exec ajouté dans une PR antérieure l'a vraisemblablement corrigée, et le commentaire du test l'énonçait déjà. Je n'ai pas pu mesurer la cause d'un défaut que je ne reproduis pas, et je préfère l'écrire plutôt que de proposer une explication que rien n'établit. Ce qui est ajouté est donc un garde-fou contre la RÉGRESSION de cette correction : un test qui démarre un vrai conteneur sans déclarer ready_exec ni ready_tcp fait rougir la suite. Les exemptions sont nommées une par une et motivées — un conteneur qui meurt par conception ne peut pas porter de sonde, et l'exiger transformerait le garde-fou en obstacle. Un second test vérifie qu'aucune exemption ne désigne un test disparu : elle survivrait au renommage et couvrirait silencieusement le suivant portant le même nom. Le garde-fou a trouvé un défaut au passage. test_post_start_en_echec_reel_leve_service_error passait pour la mauvaise raison : sans sonde, le ServiceError qu'il attend pouvait venir du conteneur pas encore prêt à recevoir un docker exec, plutôt que de la commande en échec qu'il prétend prouver. Il déclare maintenant une sonde, et vérifie que l'erreur nomme la commande fautive. Vérifié : 850 tests dont 3 neufs, éprouvés par trois mutations (sonde retirée, exemption orpheline, détection cassée), 18 e2e, ruff, mypy strict. Closes #155 Co-Authored-By: Claude Opus 5 --- CHANGELOG.fr.md | 25 +++++ CHANGELOG.md | 24 +++++ pyproject.toml | 2 +- tests/test_services.py | 10 +- tests/test_sondes_des_tests_docker.py | 132 ++++++++++++++++++++++++++ uv.lock | 2 +- 6 files changed, 192 insertions(+), 3 deletions(-) create mode 100644 tests/test_sondes_des_tests_docker.py diff --git a/CHANGELOG.fr.md b/CHANGELOG.fr.md index 8c9d3a2..282d100 100644 --- a/CHANGELOG.fr.md +++ b/CHANGELOG.fr.md @@ -9,6 +9,31 @@ et le projet suit le [versionnage sémantique](https://semver.org/lang/fr/). ## [Non publié] +## [0.1.82] - 2026-08-24 + +### Corrigé + +- **Un test à conteneur réel ne peut plus démarrer sans sonde de disponibilité** + (issue #155). Deux tests de `test_services.py` échouaient par intermittence, + toujours sous charge, jamais au repos. Leur point commun : ils attendaient un + **vrai** conteneur. Sans sonde, `start` rend la main dès que `docker run` a + répondu, ce qui ne dit rien de l'état du service à l'intérieur — l'étape + suivante devient une course, gagnée au repos et perdue sous charge. + + L'instabilité elle-même ne se reproduit plus : mesuré sous charge Docker + soutenue, 12 exécutions ciblées et 4 suites complètes, aucun échec. Ce qui est + ajouté est un garde-fou contre la **régression** de la correction, avec des + exemptions nommées une par une et motivées — un test dont le conteneur meurt + par conception ne peut pas porter de sonde, et l'exiger transformerait le + garde-fou en obstacle. + +- **`test_post_start_en_echec_reel_leve_service_error` passait pour la mauvaise + raison.** Sans sonde, le `ServiceError` qu'il attend pouvait venir du + conteneur pas encore prêt à recevoir un `docker exec`, plutôt que de la + commande en échec qu'il prétend prouver. Il déclare désormais une sonde et + vérifie que l'erreur nomme la commande fautive. + + ## [0.1.81] - 2026-08-24 ### Corrigé diff --git a/CHANGELOG.md b/CHANGELOG.md index 435c64d..1ba4d53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.1.82] - 2026-08-24 + +### Fixed + +- **A real-container test can no longer start without a readiness probe** + (issue #155). Two tests of `test_services.py` failed intermittently, always + under load, never at rest. Their common trait: they waited for a **real** + container. Without a probe, `start` hands back control as soon as `docker + run` answers, which says nothing about the service inside — the next step + becomes a race, won at rest and lost under load. + + The instability itself no longer reproduces: measured under sustained Docker + load, 12 targeted runs and 4 full suites, not one failure. What this adds is + a guard against the **regression** of the fix, with exemptions named one by + one and motivated — a test whose container dies on purpose cannot carry a + probe, and requiring one would turn the guard into an obstacle. + +- **`test_post_start_en_echec_reel_leve_service_error` was passing for the + wrong reason.** Without a probe, the `ServiceError` it expects could come + from the container not yet accepting a `docker exec`, rather than from the + failing command it means to prove. It now declares a probe and asserts that + the error names the offending command. + + ## [0.1.81] - 2026-08-24 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index a4a2ce0..3848360 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "dsoxlab" -version = "0.1.81" +version = "0.1.82" description = "Turn declarative exercises into reproducible, runnable and verifiable lab environments" readme = "README.md" requires-python = ">=3.11" diff --git a/tests/test_services.py b/tests/test_services.py index d9a892a..864e1a2 100644 --- a/tests/test_services.py +++ b/tests/test_services.py @@ -733,11 +733,19 @@ def test_post_start_en_echec_reel_leve_service_error() -> None: s = Service( name="pytest-poststart-ko", image="nginx:alpine", + # Sans sonde, `start` pourrait lever parce que le conteneur n'accepte + # pas encore un `docker exec` — et le test passerait pour la MAUVAISE + # raison, en croyant prouver que `exit 3` remonte. Avec elle, la seule + # cause possible du ServiceError est la commande elle-même. + ready_exec=["true"], post_start=[["sh", "-c", "exit 3"]], ) repo = "dsoxlab-test" try: - with pytest.raises(svc.ServiceError): + with pytest.raises(svc.ServiceError) as exc: svc.start(s, repo) + assert "exit 3" in str(exc.value) or "3" in str(exc.value), ( + f"l'erreur doit nommer la commande fautive : {exc.value}" + ) finally: svc.stop(s, repo) diff --git a/tests/test_sondes_des_tests_docker.py b/tests/test_sondes_des_tests_docker.py new file mode 100644 index 0000000..e883ed8 --- /dev/null +++ b/tests/test_sondes_des_tests_docker.py @@ -0,0 +1,132 @@ +"""Tout test qui démarre un vrai conteneur déclare une sonde (#155). + +Deux tests de `test_services.py` échouaient par intermittence, toujours sous +charge, jamais au repos. Leur point commun : ils attendaient qu'un **vrai** +conteneur soit prêt. + +C'est la mécanique que le contrat décrit déjà pour les labs. `ready_tcp` seul ne +prouve rien sur un port publié — le proxy de Docker accepte les connexions dès +le `run`, avant que le service écoute — et sans aucune sonde, `start` rend la +main sans qu'aucune étape n'ait établi que le conteneur accepte un `docker +exec`. L'enchaînement devient alors une course, que seule une machine chargée +perd. + +**Ce module ne rejoue pas l'instabilité** : elle ne se reproduit plus. Mesuré le +2026-08-24 sous une charge Docker soutenue — 12 exécutions ciblées et 4 suites +complètes, aucun échec. Il empêche la **régression** de ce qui l'a corrigée : +qu'un test crée un conteneur sans déclarer de sonde, ce qui ramènerait la course +sans que rien ne le dise, et rouvrirait une issue dont le coût principal est +d'apprendre à ne plus croire la suite. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +RACINE = Path(__file__).resolve().parent + +#: Les tests dont le conteneur ne devient JAMAIS prêt, par conception : leur +#: sujet est justement ce qui se passe quand il meurt. Une sonde y échouerait, +#: et l'exiger transformerait le garde-fou en obstacle. +#: +#: Nommés un par un, avec la raison. Une exemption qui se déduit d'un motif — +#: « les tests dont le nom contient mort » — finit par couvrir des cas qu'on +#: n'a pas examinés, et le contrôle s'érode sans que personne ne le décide. +_SANS_SONDE_A_DESSEIN = { + "test_start_status_stop_cycle": + "hello-world s'arrête aussitôt : le cycle testé est start → status → " + "stop, pas l'attente d'un service.", + "test_conteneur_mort_ne_avant_post_start_dit_pourquoi_en_vrai": + "le conteneur meurt volontairement ; c'est le diagnostic de sa mort " + "qui est éprouvé.", +} + + +def _services_sans_sonde(chemin: Path) -> list[str]: + """Les `Service(...)` construits sans sonde, dans un test d'intégration. + + Seuls comptent les tests qui touchent un vrai Docker : les autres passent + par un `run_command` neutralisé et n'attendent rien de réel. + """ + arbre = ast.parse(chemin.read_text(encoding="utf-8")) + coupables: list[str] = [] + + for fonction in ast.walk(arbre): + if not isinstance(fonction, ast.FunctionDef): + continue + if not _touche_un_vrai_docker(fonction): + continue + for noeud in ast.walk(fonction): + if not isinstance(noeud, ast.Call): + continue + if getattr(noeud.func, "id", "") != "Service": + continue + if fonction.name in _SANS_SONDE_A_DESSEIN: + continue + sondes = {kw.arg for kw in noeud.keywords} & {"ready_exec", "ready_tcp"} + if not sondes: + coupables.append(f"{fonction.name} (ligne {noeud.lineno})") + return coupables + + +def _touche_un_vrai_docker(fonction: ast.FunctionDef) -> bool: + """Le test est-il gardé par `skipif(not svc.docker_available())` ? + + C'est la marque, dans ce dépôt, d'un test qui parle au démon plutôt qu'à un + `run_command` neutralisé. + """ + return any( + "docker_available" in ast.dump(decorateur) + for decorateur in fonction.decorator_list + ) + + +def test_la_lecture_des_tests_est_representative() -> None: + """Sans ce contrôle, une lecture cassée rendrait le suivant toujours vert. + + C'est le défaut que tout ce lot corrige, et il serait piquant de l'écrire + dans le garde-fou censé l'empêcher. + """ + chemin = RACINE / "test_services.py" + arbre = ast.parse(chemin.read_text(encoding="utf-8")) + integration = [ + f for f in ast.walk(arbre) + if isinstance(f, ast.FunctionDef) and _touche_un_vrai_docker(f) + ] + + assert len(integration) >= 3, ( + f"seulement {len(integration)} tests d'intégration repérés : la " + "détection est cassée, et le contrôle suivant ne mesure rien" + ) + + +def test_chaque_exemption_designe_un_test_existant() -> None: + """Une exemption dont le test a disparu couvre le vide, et le cache. + + Elle survivrait au renommage du test qu'elle exemptait, et exempterait + silencieusement le suivant portant le même nom. + """ + source = (RACINE / "test_services.py").read_text(encoding="utf-8") + + orphelines = [nom for nom in _SANS_SONDE_A_DESSEIN + if f"def {nom}(" not in source] + + assert orphelines == [], f"exemptions sans test correspondant : {orphelines}" + + +def test_aucun_conteneur_reel_ne_demarre_sans_sonde() -> None: + """La correction de #155, tenue par un test plutôt que par la mémoire. + + Un `Service` sans sonde rend la main dès que `docker run` a répondu, ce qui + ne dit rien de l'état du service à l'intérieur. Le test qui suit devient + alors une course, gagnée au repos et perdue sous charge — un rouge qui + redevient vert sans qu'on ait rien fait, c'est-à-dire pire qu'un test + absent : il apprend à relancer plutôt qu'à lire. + """ + coupables = _services_sans_sonde(RACINE / "test_services.py") + + assert coupables == [], ( + "ces tests démarrent un vrai conteneur sans déclarer ready_exec ni " + f"ready_tcp : {coupables}" + ) diff --git a/uv.lock b/uv.lock index ce17965..e70f8e6 100644 --- a/uv.lock +++ b/uv.lock @@ -313,7 +313,7 @@ wheels = [ [[package]] name = "dsoxlab" -version = "0.1.81" +version = "0.1.82" source = { editable = "." } dependencies = [ { name = "ansible-core", version = "2.19.12", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.12'" }, From 8d239d85cb487b54eaf1f3715b669045576aacd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Mon, 24 Aug 2026 20:42:33 +0200 Subject: [PATCH 11/11] refactor(logging): le journal parle une seule langue, et c'est l'anglais (0.1.83) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Le journal mélangeait le français et l'anglais. Ce n'est pas une question de goût : c'est le fichier que dsoxlab support collecte et qu'un utilisateur colle dans un rapport de bug. Mesuré avant d'être corrigé, par lecture AST des 45 appels logger.* : 41 messages français contre 4 anglais. Les appels logger.* restent délibérément hors du garde-fou i18n, et cette exclusion tient toujours : un message de journal n'est pas un texte d'interface, il ne passe par _() nulle part, et le traduire à l'exécution rendrait deux rapports de bug incomparables selon la locale de qui les produit. Ce que l'exclusion justifiait, c'était de ne pas le traduire — pas de le laisser incohérent. Le commentaire du garde-fou annonçait d'ailleurs ce lot mot pour mot ; il est mis à jour. L'anglais l'emporte pour trois raisons, dans l'ordre de poids. Un message de journal se cherche mot pour mot dans un moteur de recherche. Il se compare entre deux machines aux locales différentes. Et il est lu par quelqu'un qui diagnostique, pas par quelqu'un qui apprend — à côté des sorties de terraform, ansible et virsh, qui sont déjà anglaises. Chaque traduction est écrite à la main plutôt que devinée, et un contrôle compare les marqueurs de format avant et après : un %s perdu casse le rendu au moment précis où quelqu'un diagnostique. La règle est écrite là où un contributeur la rencontre — le gabarit de PR, lu à chaque contribution — et tenue par test_journal_en_anglais.py. Sans test, elle se redéferait ligne par ligne, ce qui est exactement ce qui est arrivé à l'interface avant que son propre garde-fou n'existe. Le détecteur est éprouvé dans les deux sens : deux tests lui montrent du français qu'il DOIT attraper, et cinq messages anglais ou techniques qu'il ne doit pas signaler — un faux positif ferait désactiver le contrôle plutôt que corriger le message. Vérifié sur un journal réel : les lignes produites par list-labs, doctor et status, puis celles de deux chemins d'avertissement (contexte illisible, lab.yaml qui lève), ne portent plus un mot français. 854 tests dont 4 neufs, éprouvés par deux mutations, 18 e2e, ruff, mypy strict. Closes #140 Co-Authored-By: Claude Opus 5 --- .github/PULL_REQUEST_TEMPLATE.md | 5 ++ CHANGELOG.fr.md | 27 +++++++ CHANGELOG.md | 26 ++++++ pyproject.toml | 2 +- src/dsoxlab/cli/etat.py | 2 +- src/dsoxlab/config.py | 4 +- src/dsoxlab/discovery/repo.py | 2 +- src/dsoxlab/discovery/scanner.py | 6 +- src/dsoxlab/infra/inventory.py | 14 ++-- src/dsoxlab/infra/libvirt.py | 8 +- src/dsoxlab/infra/snapshot/kvm.py | 22 +++--- src/dsoxlab/infra/terraform.py | 8 +- src/dsoxlab/interrupt.py | 4 +- src/dsoxlab/locking.py | 8 +- src/dsoxlab/runtimes/shell.py | 4 +- src/dsoxlab/runtimes/vm.py | 2 +- tests/test_i18n_coverage.py | 8 +- tests/test_journal_en_anglais.py | 126 ++++++++++++++++++++++++++++++ tests/test_snapshot_kvm.py | 3 +- uv.lock | 2 +- 20 files changed, 236 insertions(+), 47 deletions(-) create mode 100644 tests/test_journal_en_anglais.py diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index c16f55b..a4d3206 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -47,6 +47,11 @@ box reads as "forgotten", an explicit `N/A` reads as "considered". EN **and** FR — `fullhelp` must never describe a command that no longer exists - [ ] Checked with `DSOXLAB_LANG=en` and `DSOXLAB_LANG=fr` +- [ ] Any new `logger.*` message is written **in English**. The log is not + interface text — it never goes through `_()` — but it is the file + `dsoxlab support` collects: it gets searched word for word, and compared + between machines with different locales. `test_journal_en_anglais.py` + enforces this. ### When `.github/workflows/` is touched — otherwise N/A diff --git a/CHANGELOG.fr.md b/CHANGELOG.fr.md index 282d100..5606977 100644 --- a/CHANGELOG.fr.md +++ b/CHANGELOG.fr.md @@ -9,6 +9,33 @@ et le projet suit le [versionnage sémantique](https://semver.org/lang/fr/). ## [Non publié] +## [0.1.83] - 2026-08-24 + +### Modifié + +- **Le journal parle désormais une seule langue, et c'est l'anglais** + (issue #140). Il mélangeait le français et l'anglais, ce qui n'est pas une + question de goût : c'est le fichier que `dsoxlab support` collecte et qu'un + utilisateur colle dans un rapport de bug. Mesuré avant d'être corrigé — + **41 messages français contre 4 anglais**. + + Les appels `logger.*` restent **délibérément hors** du garde-fou i18n : un + message de journal n'est pas un texte d'interface, il ne passe par `_()` nulle + part, et le traduire à l'exécution rendrait deux rapports de bug incomparables + selon la locale de qui les produit. Cette exclusion justifiait de ne pas le + *traduire*, pas de le laisser incohérent. + + L'anglais l'emporte pour trois raisons : un message de journal se cherche + **mot pour mot** dans un moteur de recherche, il se compare entre deux + machines aux locales différentes, et il est lu par quelqu'un qui diagnostique + — à côté des sorties de terraform, ansible et virsh, qui sont déjà anglaises. + + La règle est écrite là où un contributeur la rencontre (le gabarit de PR) et + tenue par `test_journal_en_anglais.py`. Sans test, elle se redéferait ligne + par ligne, ce qui est exactement ce qui est arrivé à l'interface avant que son + propre garde-fou n'existe. + + ## [0.1.82] - 2026-08-24 ### Corrigé diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ba4d53..ff8f993 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.1.83] - 2026-08-24 + +### Changed + +- **The log now speaks one language, and it is English** (issue #140). It mixed + French and English, which is not a matter of taste: this is the file + `dsoxlab support` collects and that a user pastes into a bug report. Measured + before being fixed — **41 French messages against 4 English ones**. + + `logger.*` calls stay **deliberately outside** the i18n guard: a log line is + not interface text, it never goes through `_()`, and translating it at + runtime would make two bug reports incomparable depending on the locale of + whoever produced them. That exclusion justified not *translating* the log, + not leaving it incoherent. + + English wins for three reasons: a log line gets searched **word for word**, + it gets compared between machines with different locales, and it is read by + someone diagnosing — next to the output of terraform, ansible and virsh, + which is already English. + + The rule is written where a contributor meets it (the PR template) and held + by `test_journal_en_anglais.py`. Without a test it would come undone line by + line, which is exactly what happened to the interface before its own guard + existed. + + ## [0.1.82] - 2026-08-24 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 3848360..6fb14ce 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "dsoxlab" -version = "0.1.82" +version = "0.1.83" description = "Turn declarative exercises into reproducible, runnable and verifiable lab environments" readme = "README.md" requires-python = ">=3.11" diff --git a/src/dsoxlab/cli/etat.py b/src/dsoxlab/cli/etat.py index 726378e..1f784f7 100644 --- a/src/dsoxlab/cli/etat.py +++ b/src/dsoxlab/cli/etat.py @@ -202,7 +202,7 @@ def _etat_domaine(fqdn: str) -> libvirt.DomainStatus | None: try: return libvirt.inspect_host(fqdn, known=domaines_connus) except CommandError as exc: - logger.debug("état libvirt indisponible pour %s : %s", fqdn, exc) + logger.debug("libvirt state unavailable for %s: %s", fqdn, exc) return None ok_count = 0 diff --git a/src/dsoxlab/config.py b/src/dsoxlab/config.py index edb3798..13f3798 100644 --- a/src/dsoxlab/config.py +++ b/src/dsoxlab/config.py @@ -113,14 +113,14 @@ def read_context(root: Path) -> ActiveContext: except (json.JSONDecodeError, OSError, UnicodeDecodeError): # UnicodeDecodeError descend de ValueError, pas d'OSError : un fichier # d'octets arbitraires passait donc à travers l'ancien filet. - logger.warning("Contexte illisible, ignoré : %s", path) + logger.warning("Unreadable context, ignored: %s", path) return ActiveContext() # La racine du document peut être n'importe quel type JSON. Sur une liste # ou une chaîne, `.get()` n'existe pas et lève un AttributeError que rien # n'attrapait. if not isinstance(data, dict): - logger.warning("Contexte non conforme (racine %s), ignoré : %s", + logger.warning("Malformed context (root %s), ignored: %s", type(data).__name__, path) return ActiveContext() diff --git a/src/dsoxlab/discovery/repo.py b/src/dsoxlab/discovery/repo.py index 68d306f..5ed872a 100644 --- a/src/dsoxlab/discovery/repo.py +++ b/src/dsoxlab/discovery/repo.py @@ -85,7 +85,7 @@ def read_repo_metadata( # Erreur de résolution provider : on remonte le message # complet à l'utilisateur via stderr (la CLI catch et # affiche), au lieu d'un simple warning silencieux. - logger.error("meta.yml : %s", exc) + logger.error("meta.yml: %s", exc) # Re-raise pour que l'erreur de provider ambigu remonte au # CLI (qui guidera vers ``dsoxlab use``). raise diff --git a/src/dsoxlab/discovery/scanner.py b/src/dsoxlab/discovery/scanner.py index 73cb96b..8453342 100644 --- a/src/dsoxlab/discovery/scanner.py +++ b/src/dsoxlab/discovery/scanner.py @@ -142,13 +142,13 @@ def scan_catalog( # la même chose, dont une en français quel que soit DSOXLAB_LANG. La # trace reste dans le fichier de journal, qui garde tout le DEBUG. logger.debug( - "lab.yaml écarté (%s) : il déclare schema_version %d, " - "au-delà de la version %d que ce dsoxlab sait lire.", + "lab.yaml skipped (%s): it declares schema_version %d, " + "beyond version %d that this dsoxlab can read.", yaml_path, exc.found, exc.supported, ) scan.unsupported.append(exc) except (KeyError, ValueError, yaml.YAMLError) as exc: - logger.warning("lab.yaml ignoré (%s) : %s", yaml_path, exc) + logger.warning("lab.yaml ignored (%s): %s", yaml_path, exc) scan.illisibles.append((yaml_path, f"{type(exc).__name__}: {exc}")) scan.labs = _sort_labs(scan.labs, root, repo_meta) diff --git a/src/dsoxlab/infra/inventory.py b/src/dsoxlab/infra/inventory.py index c98d5b3..9aaa271 100644 --- a/src/dsoxlab/infra/inventory.py +++ b/src/dsoxlab/infra/inventory.py @@ -128,8 +128,8 @@ def build_inventory( ip = tf_hosts.get(host_def.name) or host_def.ip if not ip: logger.warning( - "Host %s sans IP : ni outputs Terraform ni meta.yml ip:. " - "Lance d'abord 'dsoxlab provision'.", + "Host %s has no IP: neither Terraform outputs nor meta.yml ip:. " + "Run 'dsoxlab provision' first.", host_def.name, ) continue @@ -399,7 +399,7 @@ def _host_ready_timeout(explicite: float | None) -> float: valeur = float(brut) except ValueError: logger.warning( - "%s=%r n'est pas un nombre : on garde %.0f s.", + "%s=%r is not a number: keeping %.0f s.", HOST_READY_TIMEOUT_ENV, brut, HOST_READY_TIMEOUT_DEFAULT, @@ -407,7 +407,7 @@ def _host_ready_timeout(explicite: float | None) -> float: return HOST_READY_TIMEOUT_DEFAULT if valeur <= 0: logger.warning( - "%s=%r doit être positif : on garde %.0f s.", + "%s=%r must be positive: keeping %.0f s.", HOST_READY_TIMEOUT_ENV, brut, HOST_READY_TIMEOUT_DEFAULT, @@ -440,7 +440,7 @@ def _reset_kvm_domain(repo_meta: RepoMetadata, fqdn: str) -> bool: except CommandError: return False if res.ok: - logger.info("reset envoyé à %s (déblocage du premier boot)", fqdn) + logger.info("reset sent to %s (unblocking first boot)", fqdn) return True return False @@ -589,7 +589,7 @@ def wait_for_hosts_ready( check=False, ) if proc.returncode == 0: - logger.info("Host %s prêt (tentative %d).", fqdn, attempt) + logger.info("Host %s ready (attempt %d).", fqdn, attempt) code, detail = _etat_cloud_init(proc.stdout) if code is not None and code != 0: avertissements.append(_( @@ -662,7 +662,7 @@ def read_terraform_outputs(repo_meta: RepoMetadata) -> dict[str, Any] | None: check=False, ) except Exception as exc: # noqa: BLE001 — best-effort - logger.debug("Lecture outputs Terraform impossible : %s", exc) + logger.debug("Cannot read Terraform outputs: %s", exc) return None if not result.ok or not result.stdout.strip(): diff --git a/src/dsoxlab/infra/libvirt.py b/src/dsoxlab/infra/libvirt.py index 16f6e91..306c0d5 100644 --- a/src/dsoxlab/infra/libvirt.py +++ b/src/dsoxlab/infra/libvirt.py @@ -132,12 +132,12 @@ def _prefixe(*, timeout: int = _TIMEOUT) -> list[str]: for candidat in ([], ["sudo", "-n"]): if _sonder(candidat, timeout=timeout): logger.debug( - "virsh joignable avec le préfixe %r sur %s", candidat, _uri() + "virsh reachable with prefix %r on %s", candidat, _uri() ) _prefixe_retenu = candidat return candidat - logger.debug("virsh injoignable, ni en direct ni par sudo -n") + logger.debug("virsh unreachable, neither directly nor through sudo -n") _prefixe_retenu = [] return _prefixe_retenu @@ -232,7 +232,7 @@ def resolve_domain(host_fqdn: str, *, known: list[str] | None = None) -> str: candidates = [host_fqdn] if short == host_fqdn else [host_fqdn, short] for candidate in candidates: if candidate in domains: - logger.debug("domaine libvirt résolu : %s → %s", host_fqdn, candidate) + logger.debug("libvirt domain resolved: %s -> %s", host_fqdn, candidate) return candidate raise DomainNotFound(host_fqdn, candidates, domains) @@ -345,7 +345,7 @@ def inspect_host(host_fqdn: str, *, known: list[str] | None = None) -> DomainSta except CommandError as exc: # Le domaine existe — on vient de le résoudre. Ne pas connaître son état # n'autorise pas à le déclarer absent. - logger.debug("état indisponible pour %s : %s", domain, exc) + logger.debug("state unavailable for %s: %s", domain, exc) return DomainStatus(host=host_fqdn, domain=domain) adresses = lease_addresses(domain) if etat in RUNNING_STATES else [] return DomainStatus(host=host_fqdn, domain=domain, state=etat, addresses=adresses) diff --git a/src/dsoxlab/infra/snapshot/kvm.py b/src/dsoxlab/infra/snapshot/kvm.py index 11b3c34..3f82fb2 100644 --- a/src/dsoxlab/infra/snapshot/kvm.py +++ b/src/dsoxlab/infra/snapshot/kvm.py @@ -251,7 +251,7 @@ def _delete_volume(pool: str, path: str) -> bool: result = run_virsh(["vol-delete", "--pool", pool, nom], check=False, timeout=120) if not result.ok: logger.warning( - "vol-delete sans effet pour %s dans %s : %s", + "vol-delete had no effect for %s in %s: %s", nom, pool, result.stderr.strip(), ) return result.ok @@ -298,7 +298,7 @@ def create(repo_meta: RepoMetadata, hosts: list[str], name: str) -> None: for fqdn in hosts: domain = resolve_domain(fqdn, known=known) if name in _snapshot_names(domain): - logger.info("snapshot %s déjà présent sur %s : remplacé", name, domain) + logger.info("snapshot %s already present on %s: replaced", name, domain) _drop(domain, name) args = [ "snapshot-create-as", @@ -310,7 +310,7 @@ def create(repo_meta: RepoMetadata, hosts: list[str], name: str) -> None: ] for cible, chemin in sorted(_writable_disks(domain).items()): args += ["--diskspec", f"{cible},snapshot=external,file={chemin}.{name}"] - logger.info("virsh snapshot-create-as %s %s (externe)", domain, name) + logger.info("virsh snapshot-create-as %s %s (external)", domain, name) run_virsh(args, timeout=300) @@ -337,7 +337,7 @@ def revert(repo_meta: RepoMetadata, hosts: list[str], name: str) -> None: if tournait: run_virsh(["destroy", domain], timeout=120) for couche in couches: - logger.info("retour arrière %s:%s → %s", domain, couche.target, couche.base) + logger.info("revert %s:%s -> %s", domain, couche.target, couche.base) _reset_overlay(couche) if tournait: run_virsh(["start", domain], timeout=300) @@ -374,7 +374,7 @@ def delete(repo_meta: RepoMetadata, hosts: list[str], name: str) -> None: try: domain = resolve_domain(fqdn, known=known) except DomainNotFound as exc: - logger.warning("snapshot-delete ignoré : %s", exc) + logger.warning("snapshot-delete skipped: %s", exc) continue _drop(domain, name) @@ -395,7 +395,7 @@ def _drop(domain: str, name: str) -> None: return except CommandError as exc: logger.warning( - "snapshot-delete a échoué pour %s/%s : %s", + "snapshot-delete failed for %s/%s: %s", domain, name, exc.result.stderr.strip(), ) result = run_virsh( @@ -403,12 +403,12 @@ def _drop(domain: str, name: str) -> None: ) if result.ok: logger.warning( - "snapshot %s/%s oublié sans fusion : le recouvrement reste la " - "couche vive du disque", domain, name, + "snapshot %s/%s dropped without merge: the overlay remains the " + "live disk layer", domain, name, ) else: logger.warning( - "snapshot %s/%s non supprimé : %s", domain, name, result.stderr.strip() + "snapshot %s/%s not deleted: %s", domain, name, result.stderr.strip() ) @@ -434,7 +434,7 @@ def purge(repo_meta: RepoMetadata, hosts: list[str]) -> list[str]: try: known = list_domains() except CommandError as exc: - logger.warning("purge des snapshots impossible : %s", exc) + logger.warning("cannot purge snapshots: %s", exc) return retires for fqdn in hosts: @@ -446,7 +446,7 @@ def purge(repo_meta: RepoMetadata, hosts: list[str]) -> list[str]: try: couches = _snapshot_layers(domain, name) except (SnapshotError, CommandError) as exc: - logger.warning("snapshot %s/%s illisible : %s", domain, name, exc) + logger.warning("snapshot %s/%s unreadable: %s", domain, name, exc) couches = [] run_virsh( ["snapshot-delete", domain, name, "--metadata"], diff --git a/src/dsoxlab/infra/terraform.py b/src/dsoxlab/infra/terraform.py index a36dc58..a6dce04 100644 --- a/src/dsoxlab/infra/terraform.py +++ b/src/dsoxlab/infra/terraform.py @@ -334,7 +334,7 @@ def _domains_in_state(state_file: Path) -> set[str] | None: try: data = json.loads(state_file.read_text(encoding="utf-8")) except (OSError, json.JSONDecodeError) as exc: - logger.warning("tfstate illisible (%s) : %s", state_file, exc) + logger.warning("tfstate unreadable (%s): %s", state_file, exc) return None if not isinstance(data, dict): return None @@ -526,7 +526,7 @@ def _ensure_kvm_dhcp_leases( except CommandError as exc: res = exc.result if res.ok: - logger.info("bail DHCP ajouté à chaud: %s -> %s (%s)", host.name, ip, mac) + logger.info("DHCP lease added live: %s -> %s (%s)", host.name, ip, mac) else: # Best-effort ne veut pas dire muet. Sans ce bail, l'hôte n'obtiendra # pas son IP et l'attente échouera plus tard sur un « injoignable » @@ -535,7 +535,7 @@ def _ensure_kvm_dhcp_leases( # est un silence. erreur = (res.stderr or res.stdout).strip() logger.warning( - "bail DHCP refusé pour %s (%s) : %s", host.name, mac, erreur, + "DHCP lease refused for %s (%s): %s", host.name, mac, erreur, ) avertissements.append( _("provision_lease_refused", host=host.name, mac=mac, error=erreur) @@ -900,7 +900,7 @@ def _read_outputs(tf_dir: Path, *, env: dict[str, str] | None = None) -> Provisi try: outputs = json.loads(result.stdout) except json.JSONDecodeError: - logger.warning("Sortie 'terraform output -json' non parsable.") + logger.warning("Output of 'terraform output -json' is not parsable.") hosts_output = outputs.get("hosts", {}).get("value", {}) hosts: dict[str, str] = { diff --git a/src/dsoxlab/interrupt.py b/src/dsoxlab/interrupt.py index e1f6805..84f7c71 100644 --- a/src/dsoxlab/interrupt.py +++ b/src/dsoxlab/interrupt.py @@ -155,12 +155,12 @@ def is_requested(self) -> bool: def _handler(self, signum: int, frame: FrameType | None) -> None: del frame self.count += 1 - logger.info("signal %d reçu (%d fois)", signum, self.count) + logger.info("signal %d received (%d times)", signum, self.count) if self._on_notice is not None: try: self._on_notice(self.count) except Exception: # un affichage ne casse pas un arrêt - logger.exception("notification d'interruption en échec") + logger.exception("interrupt notification failed") if self.count >= 2: # Le premier signal a demandé l'annulation ; le second dit que # l'utilisateur n'attend plus. On repasse par le chemin normal de diff --git a/src/dsoxlab/locking.py b/src/dsoxlab/locking.py index ead02fa..b5e1acf 100644 --- a/src/dsoxlab/locking.py +++ b/src/dsoxlab/locking.py @@ -237,8 +237,8 @@ def acquire(self) -> None: # Dégradé assumé et tracé : mieux vaut un outil qui travaille # sans filet qu'un outil qui refuse de démarrer. logger.warning( - "verrou indisponible sur %s (%s) : la commande continue " - "sans protection contre une invocation concurrente", + "lock unavailable on %s (%s): the command proceeds " + "without protection against a concurrent invocation", self.path, exc.strerror, ) self._fd = fd @@ -262,7 +262,7 @@ def _ecrire_detenteur(self, fd: int) -> None: os.lseek(fd, 0, os.SEEK_SET) os.write(fd, charge.encode("utf-8")) except OSError: - logger.warning("verrou pris, mais son détenteur n'a pas pu être inscrit") + logger.warning("lock acquired, but its holder could not be recorded") def release(self) -> None: """Relâche le verrou et efface la trace du détenteur. @@ -278,7 +278,7 @@ def release(self) -> None: try: os.ftruncate(fd, 0) except OSError: - logger.debug("troncature du verrou impossible", exc_info=True) + logger.debug("cannot truncate the lock file", exc_info=True) self._degrade = False os.close(fd) diff --git a/src/dsoxlab/runtimes/shell.py b/src/dsoxlab/runtimes/shell.py index d92ee2a..00305c0 100644 --- a/src/dsoxlab/runtimes/shell.py +++ b/src/dsoxlab/runtimes/shell.py @@ -109,7 +109,7 @@ def start( # l'arborescence s'écrasaient l'un l'autre). dst.parent.mkdir(parents=True, exist_ok=True) shutil.copy2(src, dst) - logger.info("fixture %s → %s", src.name, dst) + logger.info("fixture %s -> %s", src.name, dst) def session_spec(self, lab: LabDefinition) -> SessionSpec: """Un sous-shell dans ``/``. @@ -152,7 +152,7 @@ def clean( workdir = self._workdir_path(lab) if workdir.exists(): shutil.rmtree(workdir) - logger.info("workdir supprimé : %s", workdir) + logger.info("workdir removed: %s", workdir) def status(self, lab: LabDefinition, target_name: str | None = None) -> str: del target_name diff --git a/src/dsoxlab/runtimes/vm.py b/src/dsoxlab/runtimes/vm.py index 309c36a..cf7365f 100644 --- a/src/dsoxlab/runtimes/vm.py +++ b/src/dsoxlab/runtimes/vm.py @@ -188,7 +188,7 @@ def clean( snapshot_infra.delete(repo_meta, [target.host], self._snap_name(lab)) except Exception as exc: # noqa: BLE001 — nettoyage best-effort logger.warning( - "Point de reprise %s non retiré : %s", self._snap_name(lab), exc + "Checkpoint %s not removed: %s", self._snap_name(lab), exc ) def status(self, lab: LabDefinition, target_name: str | None = None) -> str: diff --git a/tests/test_i18n_coverage.py b/tests/test_i18n_coverage.py index 2aa1e5c..a590ac6 100644 --- a/tests/test_i18n_coverage.py +++ b/tests/test_i18n_coverage.py @@ -57,8 +57,12 @@ rapports de bug incomparables selon la locale de qui les produit, et se heurte au formatage paresseux (``logger.info("x %s", v)``) que la famille de règles ``G`` impose ici : ``_()`` formate à l'appel, ``logging`` au rendu. Le journal -doit être *cohérent* — il mélange aujourd'hui le français et l'anglais, ce qui -est un vrai défaut — mais cohérent n'est pas traduit, et c'est un autre lot. +doit être *cohérent* sans être traduit, et il l'est depuis #140 : **il s'écrit +en anglais**, règle tenue par ``test_journal_en_anglais.py``. Un message de +journal se cherche mot pour mot dans un moteur de recherche, se compare entre +deux machines aux locales différentes, et voisine déjà avec les sorties de +terraform, ansible et virsh. Il reste hors du périmètre de CE garde-fou : ce +n'est pas de l'interface, et il ne passe pas par ``_()``. **``models/`` est dans le périmètre depuis #139**, et la dette qui l'en tenait dehors est soldée. Les 24 ``ValueError`` du contrat ont été triées sur une seule diff --git a/tests/test_journal_en_anglais.py b/tests/test_journal_en_anglais.py new file mode 100644 index 0000000..7d657a0 --- /dev/null +++ b/tests/test_journal_en_anglais.py @@ -0,0 +1,126 @@ +"""Le journal parle une seule langue, et c'est l'anglais (#140). + +Le journal mélangeait le français et l'anglais. Ce n'est pas un détail +d'esthétique : c'est le fichier que `dsoxlab support` collecte et qu'un +utilisateur colle dans un rapport de bug. + +Les appels `logger.*` sont **délibérément exclus** du garde-fou i18n +(`test_i18n_coverage.py`), et cette exclusion tient toujours : un message de +journal n'est pas un texte d'interface, il ne passe pas par `_()`, et le +traduire à l'exécution rendrait deux rapports incomparables selon la locale de +qui les produit. Mais l'exclusion justifiait de ne pas le **traduire**, pas de +le laisser incohérent. + +L'anglais l'emporte pour trois raisons, dans l'ordre de poids : + +1. un message de journal se cherche **mot pour mot** dans un moteur de + recherche ; +2. il se compare entre deux machines aux locales différentes ; +3. il est lu par quelqu'un qui **diagnostique**, pas par quelqu'un qui apprend — + et il voisine déjà avec les sorties de terraform, ansible et virsh, qui sont + anglaises. + +Sans ce test, la règle se redéfera ligne par ligne, ce qui est exactement ce +qui s'est passé pour l'interface avant que son garde-fou n'existe. +""" + +from __future__ import annotations + +import ast +import re +from pathlib import Path + +import dsoxlab + +RACINE = Path(dsoxlab.__file__).resolve().parent + +_NIVEAUX = {"debug", "info", "warning", "error", "exception", "critical"} + +#: Des mots qui n'existent qu'en français, et qu'aucun nom d'outil, d'option ou +#: de chemin ne porte. La liste est volontairement courte : elle doit produire +#: **zéro** faux positif, faute de quoi on apprendrait à l'ignorer. Elle ne +#: prétend pas détecter tout le français — un message qui y échappe passera, +#: et c'est assumé : ce test est un garde-fou, pas un correcteur. +_MOTS_FRANCAIS = frozenset({ + "aucun", "aucune", "avec", "chemin", "commande", "dans", "depuis", "déjà", + "échec", "échoué", "écarté", "état", "fichier", "ignoré", "ignorée", + "illisible", "impossible", "introuvable", "lecture", "les", "mais", + "nest", "pas", "pour", "sans", "sur", "vers", "verrou", +}) + +_MOT = re.compile(r"[a-zà-ÿ]+", re.IGNORECASE) + + +def _messages_de_journal() -> list[tuple[str, int, str]]: + """Chaque littéral passé en premier argument d'un `logger.*`.""" + trouves: list[tuple[str, int, str]] = [] + for chemin in sorted(RACINE.rglob("*.py")): + arbre = ast.parse(chemin.read_text(encoding="utf-8")) + for noeud in ast.walk(arbre): + if not isinstance(noeud, ast.Call): + continue + fonction = noeud.func + if not (isinstance(fonction, ast.Attribute) + and fonction.attr in _NIVEAUX): + continue + if getattr(fonction.value, "id", "") != "logger" or not noeud.args: + continue + premier = noeud.args[0] + if isinstance(premier, ast.Constant) and isinstance(premier.value, str): + trouves.append(( + str(chemin.relative_to(RACINE)), noeud.lineno, premier.value, + )) + return trouves + + +def _mots_francais(message: str) -> set[str]: + return {m.lower() for m in _MOT.findall(message)} & _MOTS_FRANCAIS + + +def test_la_lecture_des_sources_est_representative() -> None: + """Sans ce contrôle, une lecture cassée rendrait le suivant toujours vert. + + C'est le motif que tout ce lot corrige, et il vaut d'abord pour le + garde-fou lui-même. + """ + assert len(_messages_de_journal()) >= 40 + + +def test_aucun_message_de_journal_n_est_en_francais() -> None: + """La règle, tenue par un test plutôt que par la bonne volonté.""" + coupables = [ + f"{fichier}:{ligne} {message[:60]} ← {sorted(mots)}" + for fichier, ligne, message in _messages_de_journal() + if (mots := _mots_francais(message)) + ] + + assert coupables == [], ( + "ces messages de journal sont en français ; le journal est en anglais, " + "parce qu'il se cherche mot pour mot et se compare entre machines :\n " + + "\n ".join(coupables) + ) + + +def test_le_detecteur_mord_sur_un_message_francais() -> None: + """L'autre bout : un détecteur qui ne détecte rien passerait aussi au vert. + + Il faut donc lui montrer un message qu'il **doit** attraper, sinon le test + précédent ne prouve que l'absence de bug dans la liste de mots. + """ + assert _mots_francais("verrou indisponible sur %s : commande ignorée") + assert _mots_francais("Contexte illisible, ignoré : %s") + + +def test_le_detecteur_epargne_l_anglais_et_les_noms_techniques() -> None: + """Un faux positif ferait désactiver le contrôle plutôt que corriger. + + Les noms d'outils, d'options et de chemins ne doivent jamais le déclencher. + """ + for message in ( + "lock unavailable on %s (%s): the command proceeds without protection", + "virsh snapshot-create-as %s %s (external)", + "run: %s (cwd=%s)", + "terraform output -json failed: %s", + "DHCP lease added live: %s -> %s (%s)", + ): + assert not _mots_francais(message), f"faux positif sur : {message}" diff --git a/tests/test_snapshot_kvm.py b/tests/test_snapshot_kvm.py index 0b406f3..e004d36 100644 --- a/tests/test_snapshot_kvm.py +++ b/tests/test_snapshot_kvm.py @@ -746,7 +746,8 @@ def test_un_libvirt_ancien_retombe_sur_l_oubli_de_la_metadonnee( assert kvm.list_(meta, "web1.lab") == [] assert "travail-de-l-apprenant" in faux.contenu("web1.lab") assert faux.domaines["web1.lab"]["disques"]["vda"] in faux.fichiers() - assert "sans fusion" in caplog.text + # Le journal est en anglais depuis #140 : l'assertion suit la règle. + assert "without merge" in caplog.text def test_delete_tolere_un_domaine_absent_mais_le_journalise( diff --git a/uv.lock b/uv.lock index e70f8e6..c1bd419 100644 --- a/uv.lock +++ b/uv.lock @@ -313,7 +313,7 @@ wheels = [ [[package]] name = "dsoxlab" -version = "0.1.82" +version = "0.1.83" source = { editable = "." } dependencies = [ { name = "ansible-core", version = "2.19.12", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.12'" },