pull-mirrors: fix null mirror flags (rate-limited geoip) + show table in run summary - #956
Conversation
Every flag in the generated Mirrors.md became flagsapi.com/null/... The
country code came from ipwhois.app, whose free tier now rate-limits after
~2 requests and returns {"success":false,"country_code":null}; the ~40-mirror
loop (sleep 1) tripped it almost immediately, so jq emitted the literal null.
- Switch the lookup to ip-api.com (free, 45 req/min, no key -> countryCode).
- Retry a few times on a transient/rate-limited response and validate a
2-letter code; leave it empty rather than emitting "null".
- Guard the empty-IP case: ip-api.com/json/ with no address geolocates the
runner itself, which would stamp a wrong flag on an unresolved mirror.
- When no country is resolved, render a plain region link instead of a broken
flagsapi/null image.
- Also append the rendered Current Mirrors table to the workflow run summary.
Signed-off-by: Igor Pecovnik <igor@armbian.com>
github-actions
Bot
added
size/small
|
Warning Review limit reached
Next review available in: 31 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe workflow replaces Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🧹 Nitpick comments (1)
.github/workflows/pull-mirrors-from-db.yml (1)
98-98: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid accepting cleartext geolocation response for the flag.
http://ip-api.comis the current free endpoint, butstatusandcountryCodecan be modified in cleartext before the two-letter validation. Use a more trusted secure source for the mirror country if any tampering can publish the wrong flag.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pull-mirrors-from-db.yml at line 98, Update the GEO lookup in the mirror-country flow to use a trusted HTTPS geolocation endpoint instead of the cleartext ip-api.com URL, preserving the existing status and countryCode response contract and downstream two-letter validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pull-mirrors-from-db.yml:
- Around line 95-105: Move the decommissioning status check ahead of the
geolocation block in the mirror-processing flow, and only execute the IP lookup
when the mirror is not decommissioning. Preserve the existing SITE_FLAG
initialization and country-code validation for active mirrors, using the
existing status-check symbol visible nearby.
---
Nitpick comments:
In @.github/workflows/pull-mirrors-from-db.yml:
- Line 98: Update the GEO lookup in the mirror-country flow to use a trusted
HTTPS geolocation endpoint instead of the cleartext ip-api.com URL, preserving
the existing status and countryCode response contract and downstream two-letter
validation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 22b78db2-856e-43c0-ada5-2f2abd50a2b1
📒 Files selected for processing (1)
.github/workflows/pull-mirrors-from-db.yml
The workflow opened every auto-update as "Automatic documentation update" with the body "Generate documentation." - which says nothing about what changed. Retitle it to name the file it refreshes and describe what gets regenerated (Current Mirrors table + rsync sizes, sourced from NetBox), so reviewers can see the intent at a glance. Signed-off-by: Igor Pecovnik <igor@armbian.com>
github-actions
Bot
added
size/medium
The decommissioning `continue` ran after dig + up to three ip-api lookups, wasting network work and rate-limit budget on a mirror that's dropped anyway. Move the check ahead of the IP/geolocation block so it short-circuits first; SITE_FLAG init and country-code validation are unchanged for kept mirrors. Signed-off-by: Igor Pecovnik <igor@armbian.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/pull-mirrors-from-db.yml (2)
97-105: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnforce the
ip-api.comrate limit across retries.
ip-api.comallows 45 requests/minute from one IP. This workflow makes up to 30 lookup attempts plus 40 post-mirror delays, so rapid or failed retries can exceed the limit. If throttled, later mirror flags can be dropped; cap retries and pace the batch to stay under the allowed interval.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pull-mirrors-from-db.yml around lines 97 - 105, Update the retry loop that performs the ip-api.com lookup to enforce the service rate limit: cap the total lookup attempts and add sufficient pacing between requests, including failed retries, so the full batch remains below 45 requests per minute. Preserve the existing successful country-code validation and SITE_FLAG behavior.
95-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFilter
IPto an address before geolocation.
[[ -n "$IP" ]]also accepts valid DNS hostnames such as CNAME targets. The retry loop still runs three times for these values, then falls back to the plain-region flag becauseip-api.comreturnsfailfor non-IP queries. Use an IPv4/IPv6 match or an explicit address query before callingip-api.com.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pull-mirrors-from-db.yml around lines 95 - 98, Update the geolocation guard around the retry loop in the workflow to proceed only when IP contains a valid IPv4 or IPv6 address, not merely any non-empty hostname or CNAME. Preserve the existing retry and fallback behavior, and skip the ip-api.com request for non-address values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/pull-mirrors-from-db.yml:
- Around line 97-105: Update the retry loop that performs the ip-api.com lookup
to enforce the service rate limit: cap the total lookup attempts and add
sufficient pacing between requests, including failed retries, so the full batch
remains below 45 requests per minute. Preserve the existing successful
country-code validation and SITE_FLAG behavior.
- Around line 95-98: Update the geolocation guard around the retry loop in the
workflow to proceed only when IP contains a valid IPv4 or IPv6 address, not
merely any non-empty hostname or CNAME. Preserve the existing retry and fallback
behavior, and skip the ip-api.com request for non-address values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 42175b7f-5a58-4e55-a7f4-edb801772703
📒 Files selected for processing (1)
.github/workflows/pull-mirrors-from-db.yml
The GeoIP approach (ip-api.com, and ipwhois.app before it) left flags missing under rate limits and was wrong for CDN-fronted mirrors (Hetzner geolocated to FI, Macarne/Yandex to US). Use the site's human-maintained NetBox region slug instead: deterministic, no rate limits, no external call. US mirrors live under North America as coast regions, so those map to US; a region with no country mapping (e.g. "Dynamic") renders flagless rather than wrong. Verified against all active mirrors: every one resolves to the correct flag, with only the intentionally country-less "Dynamic" mirror left flagless. Drops the dig lookup and the per-row sleep that only paced the GeoIP calls. Signed-off-by: Igor Pecovnik <igor@armbian.com>
- Flag image alt is now the country name (mapped from the ISO code) instead of the region label, so a US mirror under the "East coast" region reads as "United States", not "East coast". - The "Dynamic" region is a Cloudflare-anycast mirror with no single country; render it with the UN "global" flag as an icon (flagsapi has no UN flag, flagcdn does) and a "Cloudflare anycast" alt, instead of a plain text link. Signed-off-by: Igor Pecovnik <igor@armbian.com>
1 participant
Why the flags broke
In PR #955 every flag in
docs/Mirrors.mdturned intohttps://flagsapi.com/null/shiny/32.png.Root cause is in
.github/workflows/pull-mirrors-from-db.yml:SITE_FLAG=$(curl --max-time 5 -s http://ipwhois.app/json/"${IP}" | jq -r '.country_code')ipwhois.app's free tier now rate-limits after ~2 requests and responds:{"success":false,"message":"Rate limit exceeded","country_code":null}The loop runs over ~40 mirrors with only
sleep 1, so it trips the limit almost immediately andjq -remits the literalnullfor nearly every mirror →flagsapi.com/null. (Reproduced: the 3rd rapid request already returnsRate limit exceeded.)Fix
ip-api.com(free, 45 req/min, no key) which returnscountryCode.null.ip-api.com/json/with no address geolocates the runner — which would stamp a wrong flag on a mirror whose DNS didn't resolve. Now skipped.$GITHUB_STEP_SUMMARYso it's visible on the workflow run.Validation
185.94.135.238 → IT,65.21.120.247 → FI; empty IP → empty flag (fallback), nonull, no runner-location leak.Once merged, the next scheduled/dispatched run regenerates
Mirrors.mdwith correct flags — superseding the bad auto-update in #955.Documentation website preview will be available shortly:
Open WWW preview
Also: descriptive auto-PR title/body
While in this workflow, replaced the generated PR's uninformative title (
Automatic documentation update) and body (Generate documentation.) with a title that names the refreshed file and a body describing what's regenerated (Current Mirrors table + rsync sizes, sourced from NetBox). This applies to future auto-update PRs like #955 / #957.Documentation website preview will be available shortly:
Open WWW preview