Validates each source by its kind, to match what the build accepts

.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
 

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 '

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:

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')]

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/<name>.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):

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':