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

Aperta
  • Qualcosa non va?
#21 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
4/5
Tempo stimato
3-5 giorni
Idoneità per principianti
47/100
Tipo di issue
Bug
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
github-actions, python, yaml
Ambito
ci-cd, devtools, testing

Direzione di ricerca

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.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

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.

Lingua principale
Python
Stelle
1
Fork
0
Merge medio
1g 3h
PR unite (30g)
17

Preparare l'ambiente

Questo progetto non fornisce container di sviluppo, Dockerfile né guida per i contributori, quindi l'ambiente è a tuo carico: parti dal suo README e consulta la nostra guida al primo contributo per i passaggi generali.

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di QuantEcon/lectures

Tutte le issue di QuantEcon/lectures

Issue simili

Altre issue su Python

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.