RFC download keyring debs files too #85
Conversation
WalkthroughReplaces the prior static keyring-generation workflow with a Bash-driven pipeline that discovers the latest Ubuntu suite, parses Ubuntu and Debian pages for keyring .deb URLs (including ports), downloads packages with retry/timeout logic into a temp workspace, stages files under data/keyrings, and creates per-package and generic "latest" symlinks. Commits and pushes only when changes exist. Renames job/step labels. Separately, the directory-listing workflow adds repo-level permissions, renames the checkout step, and adds a force-rewrite orphan branch step that preserves only the data directory and force-pushes it. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/generate-keyring-data.yaml(2 hunks)
🔇 Additional comments (5)
.github/workflows/generate-keyring-data.yaml (5)
28-37: Well-structured retry helper with proper error handling.The
retry_curlfunction correctly usescurlwith timeout, retry logic, and error codes. The dual-mode output (file vs. stdout) is useful. No concerns here.
42-66: Ubuntu suite detection and URL retrieval are robust.Meta-release parsing is sound; explicit error checks for missing suite and missing package URL are appropriate. URL normalization (removing CDN prefixes on line 59) is correct and handles the expected format well.
68-97: Debian package detection and download pipeline is well-designed.Associative array usage is correct; URL validation with
|| truefallback and explicit error on missing packages is appropriate. Consistent use ofretry_curlacross all downloads ensures uniform retry behavior.
139-155: Conditional commit logic is clean and appropriate.The check
git diff --cached --quietensures commits only occur when files change. This avoids unnecessary commits and aligns well with the overall design.
103-106: Git branch checkout pattern is fragile but will not fail silently.The script uses
set -euo pipefail, so errors will cause explicit exit, not silent failure. However, the current pattern has a reliability issue:git fetch origin data || truefetches the remote but doesn't create a local branch (it only updatesorigin/data). While modern git auto-creates a tracking branch ongit checkout data, this is version-dependent and unclear.Use explicit branch creation instead:
if ! git rev-parse --verify data >/dev/null 2>&1; then git fetch origin data:data || { echo "ERROR: Unable to fetch data branch from origin" >&2 exit 1 } fi git checkout dataOr use
git checkout --track origin/datadirectly iforigin/dataexists.
igorpecovnik
force-pushed
the
rfckeyring
branch
from
49305a4 to
ed1a9e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/generate-keyring-data.yaml (1)
81-83: Debian URL extraction useshead -n 1, which is non-deterministic if multiple URLs exist.The grep pattern may match multiple URLs in the HTML;
head -n 1selects the first, but URL order in the page is not guaranteed to be stable across Debian site updates.Consider preferring a specific mirror (e.g.,
deb.debian.orgoverftp.debian.org) or use a more specific regex to ensure consistent selection:- url=$(retry_curl "$DEB_PAGE_URL" \ - | grep -oP 'https?://(deb|ftp)\.debian\.org/debian/pool/main/d/[^/]+/[^"]+\.deb' \ - | head -n 1 || true) + url=$(retry_curl "$DEB_PAGE_URL" \ + | grep -oP 'https?://deb\.debian\.org/debian/pool/main/d/[^/]+/[^"]+\.deb' \ + | head -n 1 || true)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/directory-listing.yml(1 hunks).github/workflows/generate-keyring-data.yaml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Download Keyring Data
🔇 Additional comments (3)
.github/workflows/directory-listing.yml (2)
8-11: Permissions block is correct.The permissions align with intended operations:
contents: writefor force-push to data branch,pages: writefor Pages deployment, andid-token: writefor deployment source verification.
29-58: Orphan branch rewrite logic is sound.The force-rewrite step correctly preserves the generated
data/directory via temporary backup, clears all tracked files from the orphan branch, restores only the data directory, and force-pushes to reset history. Theset -euo pipefailensures strict error handling..github/workflows/generate-keyring-data.yaml (1)
26-37: Retry function has appropriate timeout and retry semantics.The
retry_curlhelper includes sensible defaults: 60-second timeout, 3 retries with--retry-all-errors, and compression. This improves resilience for downloading keyrings.
igorpecovnik
had a problem deploying
to
github-pages
GitHub Actions
Failure
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
igorpecovnik
had a problem deploying
to
github-pages
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/generate-keyring-data.yaml (1)
76-85: Debian error handling still lacks error suppression, unlike Ubuntu flow.Line 78 uses
page="$(retry_curl "$DEB_PAGE_URL")"without error handling. Withset -euo pipefail, a transient network failure or curl timeout will exit the entire workflow. By contrast, the Ubuntu code (line 55) uses2>/dev/null || trueto gracefully suppress fetch errors.The past review (commit 34a1ddb) indicated this was addressed, but the current implementation still exposes the script to transient network failures halting the workflow. Consider adding error suppression consistent with Ubuntu:
- page="$(retry_curl "$DEB_PAGE_URL")" - url="$(printf '%s' "$page" \ - | grep -oP 'https?://(deb|ftp)\.debian\.org/debian/pool/main/d/[^/]+/[^"]+\.deb' \ - | head -n 1 || true)" - [[ -z "${url:-}" ]] && { echo "ERROR: Unable to find ${p} package URL from $DEB_PAGE_URL" >&2; exit 1; } + if page="$(retry_curl "$DEB_PAGE_URL" 2>/dev/null || true)"; then + url="$(printf '%s' "$page" \ + | grep -oP 'https?://(deb|ftp)\.debian\.org/debian/pool/main/d/[^/]+/[^"]+\.deb' \ + | head -n 1 || true)" + [[ -z "${url:-}" ]] && { echo "ERROR: Unable to find ${p} package URL from $DEB_PAGE_URL" >&2; exit 1; } + else + echo "ERROR: Unable to fetch Debian page for ${p}" >&2; exit 1 + fiThis distinguishes between fetch failures (network issues; fail fast with clear error) and parse failures (missing .deb link; validate and error).
🧹 Nitpick comments (1)
.github/workflows/generate-keyring-data.yaml (1)
39-46: Temporary working directory is not cleaned up.Line 39 creates a temp directory with
mktemp -d, but it is never explicitly removed. While GitHub Actions runners are typically ephemeral and cleanup happens after the job, explicit cleanup is a best practice and helps if the runner is reused or if the workflow is adapted to self-hosted infrastructure:workdir="$(mktemp -d)" trap "rm -rf '$workdir'" EXIT echo "Workdir: $workdir"This ensures the directory is cleaned up even if the script exits early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/generate-keyring-data.yaml(2 hunks)
🔇 Additional comments (2)
.github/workflows/generate-keyring-data.yaml (2)
114-132: Symlink strategy is now deterministic and well-structured.The
link_if_presenthelper (lines 114–124) directly links to just-downloaded filenames, avoiding the previous alphabetical sort issue flagged in earlier reviews. This approach is:
- Deterministic (always uses what was actually downloaded, not sorted guesses).
- Simple and maintainable (no version-comparison logic needed).
- Per-package explicit (each symlink maps to its own keyring variant).
This cleanly resolves the prior concern about non-deterministic version ordering. The per-variant links (
latest-ubuntu-keyring.deb,latest-debian-archive-keyring.deb, etc.) are clearer than a single genericlatestlink.
100-103: Data branch validation now properly handles non-existent remote branch.The code now explicitly checks for local branch existence before attempting checkout (line 100:
git rev-parse --verify data), which addresses the prior concern about silent fetch failures. This is an improvement.However, note that if the local check fails but the remote exists,
git fetch origin data(line 101) is attempted with|| true, which suppresses errors. If that fetch also fails (e.g., network issue) and the local branch still doesn't exist, thegit checkout dataon line 103 will fail with an unclear ref error. Consider logging the state more explicitly or pre-validating the remote branch:if ! git rev-parse --verify data >/dev/null 2>&1; then if ! git fetch origin data 2>/dev/null; then echo "ERROR: Unable to fetch or create data branch" >&2 exit 1 fi fi git checkout data
|
I think I'm confused as to why cache the pkgs themselves. they are standard pkgs, mirrored a hundred places, I can't see why one would get rate-limited for these pkgs and not all of the other pkgs one downloads to build the I would even venture that it would be a good idea to use ACNG in the autobuilder, but that's a discussion for another day/week/year. |
|
It's also odd to store |
Yes. On the other hand, this is data exchange folder. Config files, databases, ... deb files containing keys. Doesn't smell the best but does the job. We have a predicted link from known infra to latest keys. Isn't that the main goal? |
|
End result works, yes.
More my objections:
a) General idea that binary files in git is an anti-pattern. Sometimes unavoidable, especially with JPG/PNG/GIF etc.
b) this will cost more transfer/bandwidth on Armbian.gh.io, as the files are much bigger. Enough to have a meaningful impact? I dunno. Especially if gh.io is "free" and remains free/gratis.
Both are more value judgements than technical objections. But I really like the idea of using ACNG for what it was designed for, and when ACNG isn't available making use of a pre-existing [and largely supported by a much larger organization that is less likely to be susceptible to the vagaries of corporate whims vs whenever MSFT decides they don't need the community's goodwill like Docker tried to do in 2023] mirror pool.
Sent from myPhone.
|
Agree. GitHub provides Git LFS, packages / Container Registry or releases for this. As basic idea for this repo was to mainly provide JSON exchange data and configurations, I choose not to go that direction.
When using history in binary deployment, that grows a lot. I have dropped this within this commit too. If we are talking about keyring deb packages, those won't add much - this is not something that is changing a lot. And its small already. All together, we won't be reaching any GH limits in decades. |
…from armbian.github.io (#8881) followup to #8785, armbian/armbian.github.io#82 & armbian/armbian.github.io#85 Pull the latest keyring pkgs from armbian's github mirror
@tabrisnet
✨ Summary
This workflow automates fetching, storing, and maintaining up-to-date Debian and Ubuntu keyring packages in https://github.armbian.com/keyrings/
🧩 What It Does
Detects the latest Ubuntu release from https://changelogs.ubuntu.com/meta-release
Retrieves the most recent .deb packages for:
Downloads official .deb files from Ubuntu and Debian mirrors.
Stores all keyring files under data/keyrings/.
Creates per-variant symlinks for quick reference:
🛠️ Implementation Details
Uses curl with retries (--retry-all-errors) and compressed transfers for reliability. Tracks downloaded filenames in an associative map (PKG_FILES) to generate symlinks deterministically — avoiding stale matches.
✅ Example Output
After successful execution, the keyrings/ directory will look like this:
🧹 Notes
Replaces old .deb files with newer versions as they are published.