CI: a typo in sync/canonical.yml or a manifest `class:` passes the PR check, then stops every refresh

Abierto
  • ¿Algo no está bien?
#21 0 comentarios 0 reacciones 0 asignados Ver en GitHub

Los mantenedores suelen responder en 1 día

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
4/5
Tiempo estimado
3-5 días
Aptitud para principiantes
47/100
Tipo de issue
Error
Claridad
Bien especificado
Estado de actividad
Activo
Stack tecnológico
github-actions, python, yaml
Área
ci-cd, devtools, testing

Línea de trabajo

Start with tools/drift-check and its promote_module() import, then read load_manifest() and load_canonical_map() in tools/promote; inspect drift-check.yml and tests.yml for the existing path filters and exit-code handling. Add validation coverage for malformed sync/manifest.yml and sync/canonical.yml, including the proposed test using the committed files. Done means invalid configuration fails the PR check while valid configuration continues to pass.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

bug

Follow-up from the review of https://github.com/QuantEcon/lectures/pull/14.

What happens

#14 adds two hand-edited inputs: sync/canonical.yml and the class: of a manifest stanza. Only tools/promote validates them, at start-up, before it plans anything (load_manifest, tools/promote:389, and load_canonical_map, tools/promote:418). An unknown series, a consumer series named as canonical, an entry with no series, a YAML error or an unknown class makes every promote run exit 1.

Nothing runs that validation before merge. A pull request that touches only sync/** gets one check, the check job of drift-check.yml, which runs tools/drift-check. The refresh job is skipped on pull requests, and tests.yml's path filter covers only tools/** (and tests.yml itself). drift-check never reads sync/canonical.yml, and consumer_series (tools/drift-check:171-173) reads any class other than consumer as source. So the edit merges green.

After the merge, CI shows nothing until a canonical copy's text moves upstream, because only then does the refresh job run. Promote then stops at start-up, so every entry is held back, not just the one the edit was meant to settle (elsewhere #14 makes sure one refused entry does not hold back the rest), and no refresh pull request is opened. The run goes red: the refresh job's guard sees what a crash part-way looks like (a non-zero exit with no summary [refresh] line) and reports that the writes may be partial, although nothing was written. This repeats on every refresh until the file is corrected.

Reproduction (with the tools/tests harness)

repo = make_repo(["intro", "intermediate", "dp"], consumers=["dp"])
repo.lecture("intro", "career", lecture_text("Career"))
repo.lecture("intermediate", "mle", lecture_text("MLE"))
assert repo.promote("career", "mle").returncode == 0

# A pull request that touches only sync/**: its one check passes.
repo.write_canonical_map({"mle": "intermedate"})
assert repo.drift_check().returncode <= 1  # 0 here; the check step accepts 0 or 1

# After the merge, an unrelated canonical copy moves upstream.
repo.lecture("intro", "career", lecture_text("Career", "an upstream fix"))
repo.move_pin("intro")
assert repo.drift_check().returncode == 1  # refresh_needed: the refresh job runs

res = repo.promote("--refresh")
assert res.returncode == 1 and res.stderr.startswith("error: ")
assert "summary [refresh]" not in res.stdout + res.stderr  # what the refresh job's guard checks
assert "an upstream fix" not in repo.pool("career.md").read_text()  # career is held back too

promote's stderr:

error: invalid entries in .../sync/canonical.yml:
       mle: unknown series 'intermedate'

The other edits behave the same way. In each case drift-check exits 0 (clean) and promote exits 1 with no summary line:

Edit promote's error
mle: dp (a consumer series) mle: 'dp' is a consumer series and is never canonical
mle: (no series) 'mle': None (expected `<slug>: <series>`)
mle: 'intermediate (an unclosed quote) could not parse .../sync/canonical.yml:, then the YAML error
class: consumers on dp in the manifest series 'dp' in .../sync/manifest.yml has unknown class 'consumers' (valid: source, consumer).

On a copy of the real pool and mirror at the pinned SHAs, changing career: intermediate to career: intermediat, appending calvo: dp, or setting class: consumers on dp behaves the same way: drift-check is clean with exit 0, and tools/promote --refresh --exclude dp jax --dry-run exits 1 with the matching error and no summary line. With the committed files, that dry run plans 41 entries and refuses none.

When it bites

It cannot happen yet, because the committed files are valid. It becomes possible with the first hand edit after #14 merges, and the documented workflow calls for such edits. A refused refresh entry tells the operator to "name the canonical series in sync/canonical.yml" (tools/promote:761-765, and also the refresh pull request and the README). Map lines change or go as the renames and merges in QuantEcon/project-monorepo#76 land, and QuantEcon/project-monorepo#24 edits both files when it retires dp-test. #14 is also adding two new errors to load_canonical_map: a repeated key, and two keys that name one lecture. These checks also run only inside promote, so more bad edits will pass the PR check and then stop the refresh. This is new with #14: main has no sync/canonical.yml and no class:.

Suggested fix

  1. Validate both files in drift-check, using promote's own loaders so that the two tools cannot disagree. Since #17, drift-check already imports tools/promote (promote_module(), tools/drift-check:180-195). Call load_manifest() and load_canonical_map() at start-up, and use the consumer set that load_manifest() returns in place of consumer_series. The loaders raise SystemExit with a message, which exits with status 1, and the check step accepts 0 or 1 (test "$rc" -le 1), so catch it and raise a DriftError (exit 2); left as it is, the pull request stays green. drift-check.yml's pull_request paths already include sync/**.
  2. Add sync/** to tests.yml's paths:, with a test that runs the same loaders on the committed sync/manifest.yml and sync/canonical.yml. The loaders need no mirror.
  3. Optionally, also run tools/promote --refresh --dry-run, with CI's exclusions, in the check job on pull requests. This also catches a bad PROMOTE_EXCLUDE and exercises promote changes on real data. However, it needs a mirror of every considered series, and it should fail only on an exit without a summary line, so that a pull request is not red because of an entry the nightly refresh already refuses.
  4. Optionally, give promote's configuration errors an exit status of their own (today they exit 1, as a refused entry does). The guard could then say that nothing was written and repeat promote's error: line in its annotation.

The test above fails once drift-check exits 2 on these edits. A fix made only in the workflows would leave it passing, so that fix needs its own test, such as the one in item 2.

Lenguaje dominante
Python
Estrellas
1
Forks
0
Merge medio
1 d 3 h
PR fusionados (30 d)
17

Preparar el entorno

Este proyecto no incluye contenedor de desarrollo, Dockerfile ni guía de contribución, así que la configuración corre por tu cuenta: empieza por su README y consulta nuestra guía para la primera contribución para los pasos generales.

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de QuantEcon/lectures

Todos los issues de QuantEcon/lectures

Issues similares

Más issues de Python

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.