From ffc252ed99064254e3cf52789ae03e3e90068f9a Mon Sep 17 00:00:00 2001 From: Alicia Sykes Date: Fri, 25 Sep 2026 20:00:01 +0100 Subject: [PATCH] Validates each source by its kind, to match what the build accepts --- .github/workflows/pr-check.yml | 23 +++++++-------- lib/combine.py | 14 +++------ lib/download.py | 4 +-- lib/list.py | 6 ++-- lib/sources_list.py | 36 ++++++----------------- lib/validate_sources.py | 52 ++++++++++++++++++++-------------- 6 files changed, 55 insertions(+), 80 deletions(-) diff --git a/.github/workflows/pr-check.yml b/.github/workflows/pr-check.yml index 8904053..ae2b14b 100644 --- a/.github/workflows/pr-check.yml +++ b/.github/workflows/pr-check.yml @@ -134,29 +134,26 @@ jobs: - name: Validate sources.csv + overrides.json run: python lib/validate_sources.py sources.csv overrides.json - # Fetch any source URL this PR adds and check it the way the build will, - # so a broken template is caught here rather than in the nightly build + # Check source urls this PR changes: a collection the build can't read stops the nightly - name: Validate newly added sources if: github.event_name == 'pull_request' env: BASE_SHA: ${{ github.event.pull_request.base.sha }} run: | set -euo pipefail - # -d '[:blank:]' strips spaces/tabs but keeps the newline between urls - added=$(git diff "$BASE_SHA" -- sources.csv \ - | grep '^+[^+]' \ - | cut -d, -f2 \ - | tr -d '[:blank:]' \ - | grep -E '^https?://' || true) - if [ -z "$added" ]; then - echo "No new sources in this PR." + # '|| true' since grep exits 1 when this PR only touches overrides.json + urls=$(git diff "$BASE_SHA" -- sources.csv \ + | grep '^+[^+]' | cut -c2- \ + | awk -F, '{ gsub(/[[:blank:]]/, "", $2) + if ($2 ~ /^https?:\/\//) print $2 }' || true) + if [ -z "$urls" ]; then + echo "This PR changes no source urls, nothing to fetch." exit 0 fi status=0 - for url in $added; do - echo "::group::$url" + for url in $urls; do + echo "--- Validating $url" python lib/validate_sources.py "$url" || status=1 - echo "::endgroup::" done exit $status diff --git a/lib/combine.py b/lib/combine.py index 18cdf2a..7b3bd0e 100644 --- a/lib/combine.py +++ b/lib/combine.py @@ -56,9 +56,7 @@ def template_rank(t, pinned=None): return (t['_source'] == pinned, -t['_priority'], template_score(t)) def source_priority(sources): - """File name -> dedup priority, where lower wins. An app source outranks every - collection, because its author maintains that one app and knows it best. - Within each kind, the order they're listed in sources.csv decides.""" + """File name -> dedup priority, lower wins: app sources first, then sources.csv order""" ordered = [s for s in sources if s.is_app] + [s for s in sources if not s.is_app] return {s.filename: i for i, s in enumerate(ordered, start=1)} @@ -248,9 +246,7 @@ def normalize_template_fields(templates): return normalized def schema_errors(t): - """Schema violations in a single template. Checked per template so that one bad - entry from an upstream source gets dropped, rather than failing the whole build - at the final validation.""" + """Schema violations in one template, so a bad entry is dropped instead of failing the build""" probe = {k: v for k, v in t.items() if not k.startswith('_')} probe.setdefault('id', 1) # combine assigns real ids at write time return [e.message for e in ITEM_VALIDATOR.iter_errors(probe)] @@ -392,9 +388,7 @@ def audit_overrides(templates, overrides): log.warning(f'Redundant override: normal priority already picks {prefer} for {where}') def audit_app_sources(templates, sources): - """An author who publishes their own template, but whose app also still sits in - sources/local/, never gets their updates published: local wins every time. - Warn, so the stale local copy can be deleted.""" + """Warn when a local copy shadows an app source, since the author's updates never publish""" local = {} for t in templates: if t['_priority'] == 0 and isinstance(t.get('title'), str): @@ -449,7 +443,7 @@ if __name__ == '__main__': previous = len(json.load(f)['templates']) except (OSError, ValueError, KeyError): previous = 0 - # An app source going away costs only its own app, so it just gets left out + # A missing app source only costs its own app, so it's left out rather than fatal unavailable = missing_sources(sources, sources_list.APP) if unavailable: log.warning(f'{len(unavailable)} app sources were not downloaded, so they are left ' diff --git a/lib/download.py b/lib/download.py index 9285515..6d4d02e 100644 --- a/lib/download.py +++ b/lib/download.py @@ -54,9 +54,7 @@ failures = [source for source in sources if not download(source)] log.info(f'Downloaded {len(sources) - len(failures)}/{len(sources)} sources') -# A collection carries hundreds of apps, so losing one is worth stopping for. -# An app source costs only its own app, and there are far more of them to go -# stale, so the build goes on without it. +# A lost collection stops the build; a lost app source only costs its own app failed_apps = [source.name for source in failures if source.is_app] failed_collections = [source.name for source in failures if not source.is_app] if failed_apps: diff --git a/lib/list.py b/lib/list.py index 2109c03..1f6d830 100644 --- a/lib/list.py +++ b/lib/list.py @@ -47,8 +47,7 @@ def generate_app_list(): return markdown_content def github_user(*urls): - """The GitHub account behind a source, for its avatar and credit link. Falls back to - the maintainer link, since a source's JSON may be served from anywhere.""" + """The GitHub account behind a source, for its avatar: from its url, else its maintainer""" for url in urls: parsed = urllib.parse.urlparse(url or '') path_parts = [p for p in parsed.path.split('/') if p] @@ -66,8 +65,7 @@ def source_credit(source, label): f'[@{username}](https://github.com/{username})') def app_label(source): - """An app source's own title, read back from what was downloaded, so the credit reads - "Cantinarr" rather than "cantinarr". Falls back to the source name when unfetched.""" + """An app's own title from what was downloaded, so the credit reads Cantinarr not cantinarr""" try: templates = load_json_file(os.path.join(external_dir, source.filename))['templates'] titles = [t['title'] for t in templates if isinstance(t, dict) and t.get('title')] diff --git a/lib/sources_list.py b/lib/sources_list.py index 85e08cb..6a4a001 100644 --- a/lib/sources_list.py +++ b/lib/sources_list.py @@ -1,19 +1,4 @@ -"""Reading sources.csv, and fetching what it points at, in one place. - -Each row is: name, url, maintainer, kind - - name unique, lowercase; becomes sources/external/.json - url http(s) link to the raw JSON - maintainer the source's home page, shown as its "Report issues" link - kind 'collection' for someone's maintained list of apps, 'app' for a - single app published by its own author. Blank means collection, - so the original three-column rows still read correctly. - -The build trusts the two kinds differently. An app source outranks every -collection when the same app appears in both, since its author maintains that -one app and knows it best. In exchange it may only carry a handful of -templates, and if it stops resolving the build carries on without it. -""" +"""Reads sources.csv (name, url, maintainer, kind) and fetches what it points at.""" import csv import os import re @@ -27,6 +12,7 @@ log = get_logger() SOURCES_CSV = os.path.join(os.path.dirname(os.path.abspath(__file__)), '..', 'sources.csv') +# 'collection' is a list of many apps, 'app' is one app published by its own author COLLECTION = 'collection' APP = 'app' KINDS = (COLLECTION, APP) @@ -34,8 +20,7 @@ KINDS = (COLLECTION, APP) # A source name becomes a file name, so keep it to something unambiguous NAME_PATTERN = re.compile(r'^[a-z0-9][a-z0-9_-]*$') -# An app may reasonably ship a container and a stack, or a plain build and a -# GPU one. Much past that is a collection, and belongs in one. +# Enough for a container plus a stack, or a plain plus a GPU build; more is a collection MAX_APP_TEMPLATES = 5 class Source(NamedTuple): @@ -63,8 +48,7 @@ def rows(path=SOURCES_CSV): yield line, cells def row_problems(cells): - """Everything wrong with a row, as (level, message) pairs. Any problem at all - means load() skips the row; the level only decides how loudly to report it.""" + """Everything wrong with a row, as (level, message) pairs; any problem makes load() skip it""" if len(cells) < 2 or not cells[0] or not cells[1]: return [('warning', f'malformed row, would be skipped: {cells}')] problems = [] @@ -89,8 +73,7 @@ def parse(cells): return Source(cells[0], cells[1], maintainer, kind) def load(path=SOURCES_CSV): - """Every usable source, in listed order. Rows with problems are skipped here - and reported by validate_sources.py, which is what gates a PR.""" + """Every usable source, in listed order. validate_sources.py reports the rows skipped here""" sources, seen = [], set() for line, cells in rows(path): problems = row_problems(cells) @@ -106,9 +89,8 @@ def load(path=SOURCES_CSV): return sources def fetch_json(url, attempts=3): - """Fetch and parse JSON from a url, retrying a couple of times on a blip. - Returns (payload, error message), of which exactly one is set.""" - import requests # only the fetching path needs it, so keep it off the parsers + """Fetch and parse JSON, retrying on a blip. Returns (payload, error), one of them None""" + import requests # only the fetching path needs it for attempt in range(attempts): try: response = requests.get(url, timeout=30) @@ -120,9 +102,7 @@ def fetch_json(url, attempts=3): time.sleep(2 ** attempt) def templates_in(payload, allow_bare=False): - """The template list inside a downloaded source, whatever shape it arrived in, - or None if there isn't one. `allow_bare` accepts a single unwrapped template - object, which is the natural way to publish just the one app.""" + """The template list in a downloaded source, or None. `allow_bare` takes a lone object""" if isinstance(payload, list): return payload if isinstance(payload, dict): diff --git a/lib/validate_sources.py b/lib/validate_sources.py index d7b14a1..868ed5e 100644 --- a/lib/validate_sources.py +++ b/lib/validate_sources.py @@ -45,7 +45,7 @@ def quality_warnings(where, template): def check_templates(name, templates): """Validate a list of templates: each must normalize to a schema-valid entry""" - errors, warnings, titles = [], [], Counter() + errors, warnings, seen = [], [], Counter() if not templates: warnings.append(f'{name}: contains no templates') for index, raw in enumerate(templates): @@ -55,7 +55,6 @@ def check_templates(name, templates): continue title = raw.get('title') if isinstance(title, str) and title.strip(): - titles[title.strip()] += 1 where = f'{name} "{title.strip()}"' # Normalize a copy exactly as combine.py would, then judge the published result try: @@ -68,9 +67,13 @@ def check_templates(name, templates): errors += [f'{where}: {e.message}' for e in VALIDATOR.iter_errors(template)] errors += stackfile_errors(where, template) warnings += quality_warnings(where, template) + # Keyed like combine's dedup, so one app may ship both a container and a stack + if isinstance(title, str) and title.strip(): + seen[(normalize_string(title), template.get('type', 1))] += 1 - errors += [f'{name}: duplicate title {title!r} ({count} templates)' - for title, count in titles.items() if count > 1] + errors += [f'{name}: duplicate template {title!r} of type {kind} ({count} copies), ' + 'so all but one would be dropped' + for (title, kind), count in seen.items() if count > 1] return errors, warnings def check_source(path): @@ -86,24 +89,32 @@ def check_source(path): return check_templates(os.path.basename(path), templates) +def url_kind(url): + """The kind sources.csv gives this url, defaulting to app, the stricter of the two""" + for source in sources_list.load(): + if source.url == url: + return source.kind + return sources_list.APP + def check_url(url): - """Fetch a remote source and check it the way the nightly build will, so an author - can test their own template before opening a PR""" + """Check a remote source as download.py will: an app per template, a collection on shape""" payload, error = sources_list.fetch_json(url) if error: return [f'{url}: could not fetch valid JSON ({error})'], [] - templates = sources_list.templates_in(payload, allow_bare=True) - if templates is None: - return [f'{url}: found no templates. Publish a single template as a JSON object, ' - 'or several under a top-level "templates" array'], [] - - errors, warnings = check_templates(url, templates) + is_app = url_kind(url) == sources_list.APP + templates = sources_list.templates_in(payload, allow_bare=is_app) + if not templates: + shape = ('one template as a JSON object, or several under a top-level "templates" ' + 'array') if is_app else 'its templates under a top-level "templates" array' + return [f'{url}: found no templates, so the build would skip it. Publish {shape}'], [] + if not is_app: + return [], [] # the shape is all the build needs from a collection if len(templates) > sources_list.MAX_APP_TEMPLATES: - warnings.append(f'{url}: holds {len(templates)} templates, so it can only be listed ' - f'as a collection, not an app source (which may carry at most ' - f'{sources_list.MAX_APP_TEMPLATES})') - return errors, warnings + return [f'{url}: holds {len(templates)} templates, so it can only be listed as a ' + f'collection, not an app source (which may carry at most ' + f'{sources_list.MAX_APP_TEMPLATES})'], [] + return check_templates(url, templates) def check_stack(path): """A compose stack file must be valid YAML with a non-empty services mapping""" @@ -122,16 +133,14 @@ def check_stack(path): return [], warnings def local_source_names(): - """File names (minus .json) in sources/local/. A source in sources.csv must not reuse - one, since combine.py tells sources apart by file name alone""" + """File names in sources/local/, which a source name can't reuse: combine keys on them""" local_dir = os.path.join(ROOT, 'sources', 'local') if not os.path.isdir(local_dir): return set() return {os.path.splitext(f)[0] for f in os.listdir(local_dir) if f.endswith('.json')} def check_csv(path): - """Validate sources.csv: each row needs a unique, file-name-safe name, an http(s) - url, and a recognized kind""" + """Validate sources.csv: unique file-name-safe name, http(s) url, recognized kind""" try: rows = list(sources_list.rows(path)) except OSError as err: @@ -207,8 +216,7 @@ def is_url(target): return target.startswith(('http://', 'https://')) def validate_path(path): - """Dispatch a target to the right checker: remote urls first, then by file name, - then by extension""" + """Dispatch a target to the right checker, by url then file name then extension""" if is_url(path): return check_url(path) if os.path.basename(path) == 'overrides.json':