geoip: T5746: Download ASN database - #1215
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe chroot hook script now downloads both DB-IP country-lite and ASN-lite datasets. The country download switches from ChangesGeoIP Download Script and Excludes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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: 2
🤖 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 `@data/live-build-config/hooks/live/40-init-geoip-database.chroot`:
- Around line 16-19: The GeoIP download error handling at lines 16-19 logs the
failure and removes the partial file but fails to exit with a non-zero status,
allowing the script to continue and potentially leave the image in a partially
initialized state. After the rm $OUT_PATH command in the error handling block,
add exit 1 to terminate the script with a failure status. This same fix needs to
be applied to the other GeoIP download failure path mentioned at lines 24-27 as
well, ensuring all download failure paths terminate immediately with a non-zero
exit code.
In `@data/live-build-config/rootfs/excludes`:
- Around line 48-49: The comment on line 48 labeled "T7926: VyOS GeoIP database
location" is misleading because the exclusion pattern
`var/cache/!(cracklib|vyos)` actually preserves the `/var/cache/vyos` directory
(the vyos cache directory), not the GeoIP database itself which is located
elsewhere at `/usr/share/vyos-geoip`. Reword the comment next to the exclusion
pattern to explicitly clarify that it preserves the vyos cache directory by
changing the description to something that accurately reflects why
`/var/cache/vyos` is being kept in the exclusion.
🪄 Autofix (Beta)
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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b0af7fe2-26ee-4709-9dc1-c66093123b3b
📒 Files selected for processing (2)
data/live-build-config/hooks/live/40-init-geoip-database.chrootdata/live-build-config/rootfs/excludes
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
📜 Review details
⏰ 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). (3)
- GitHub Check: test_config_load
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
| if [ $? -ne 0 ]; then | ||
| echo "Failed to download GeoIP database" | ||
| echo "Failed to download GeoIP country-code database" | ||
| rm $OUT_PATH | ||
| fi |
There was a problem hiding this comment.
Fail fast when either GeoIP download fails.
Right now the hook logs and removes the partial file, but still continues to Line 30. That can leave the image in a partially initialized state. Make each failure path terminate with non-zero status.
Suggested patch
-mkdir -p $(dirname $OUT_PATH)
-curl -sSfL -o $OUT_PATH $URL
-
-if [ $? -ne 0 ]; then
+mkdir -p "$(dirname "$OUT_PATH")"
+if ! curl -sSfL -o "$OUT_PATH" "$URL"; then
echo "Failed to download GeoIP country-code database"
- rm $OUT_PATH
+ rm -f "$OUT_PATH"
+ exit 1
fi
-mkdir -p $(dirname $ASN_OUT_PATH)
-curl -sSfL -o $ASN_OUT_PATH $ASN_URL
-
-if [ $? -ne 0 ]; then
+mkdir -p "$(dirname "$ASN_OUT_PATH")"
+if ! curl -sSfL -o "$ASN_OUT_PATH" "$ASN_URL"; then
echo "Failed to download GeoIP ASN database"
- rm $ASN_OUT_PATH
+ rm -f "$ASN_OUT_PATH"
+ exit 1
fiAlso applies to: 24-27
🤖 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 `@data/live-build-config/hooks/live/40-init-geoip-database.chroot` around lines
16 - 19, The GeoIP download error handling at lines 16-19 logs the failure and
removes the partial file but fails to exit with a non-zero status, allowing the
script to continue and potentially leave the image in a partially initialized
state. After the rm $OUT_PATH command in the error handling block, add exit 1 to
terminate the script with a failure status. This same fix needs to be applied to
the other GeoIP download failure path mentioned at lines 24-27 as well, ensuring
all download failure paths terminate immediately with a non-zero exit code.
| # T7926: VyOS GeoIP database location | ||
| var/cache/!(cracklib|vyos) |
There was a problem hiding this comment.
Clarify the T7926 comment to match the actual path context.
Line 48 says “VyOS GeoIP database location” in a /var/cache exclusion block, while the downloader writes to /usr/share/vyos-geoip in data/live-build-config/hooks/live/40-init-geoip-database.chroot (Lines 8 and 11). Please reword this to explicitly describe why /var/cache/vyos is preserved here.
🤖 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 `@data/live-build-config/rootfs/excludes` around lines 48 - 49, The comment on
line 48 labeled "T7926: VyOS GeoIP database location" is misleading because the
exclusion pattern `var/cache/!(cracklib|vyos)` actually preserves the
`/var/cache/vyos` directory (the vyos cache directory), not the GeoIP database
itself which is located elsewhere at `/usr/share/vyos-geoip`. Reword the comment
next to the exclusion pattern to explicitly clarify that it preserves the vyos
cache directory by changing the description to something that accurately
reflects why `/var/cache/vyos` is being kept in the exclusion.
|
CI integration 👍 passed! Details
|
sever-sever
left a comment
sever-sever
left a comment
There was a problem hiding this comment.
Download GeoIP ASN Database in live-build hook
c-po
left a comment
c-po
left a comment
There was a problem hiding this comment.
Download GeoIP ASN database to image
vyos-bot
Bot
added
mirror-initiated
3 participants
Change summary
Download initial GeoIP ASN database for system image
Types of changes
Related Task(s)
Related PR(s)
How to test / Smoketest result
Checklist: