[crc-support] Add fallback url for download#125
Conversation
WalkthroughAdds an optional fallback asset-base URL and a helper download_check to Changes
Sequence DiagramsequenceDiagram
participant Task as Tekton Task
participant Preparer as Preparer Script
participant Primary as Primary URL
participant Fallback as Fallback URL
Task->>Preparer: invoke preparer with -aBaseURLFallback
Preparer->>Primary: download_check(asset, primary_base, sha)
Primary-->>Preparer: success or failure
alt primary failed and fallback provided
Preparer->>Fallback: download_check(asset, fallback_base, sha)
Fallback-->>Preparer: success or failure
end
alt any success
Preparer->>Task: exit(0)
else all attempts failed
Preparer->>Task: exit(1) with error
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In `@crc-support/oci/lib/unix/run.sh`:
- Around line 88-98: The script declares #!/bin/sh but uses Bash-only features;
update the shebang to #!/bin/bash or refactor to POSIX shell; specifically,
change the top-level shebang line or eliminate Bash-specific constructs used
across functions such as download_check (which uses local), any uses of [[ ...
]], ${BASH_SOURCE[0]}, and pushd/popd so the script is either explicitly run
with /bin/bash or rewritten to use POSIX-compatible alternatives (use plain
variable assignments, replace [[ ]] with test/[ ], remove ${BASH_SOURCE[0]}
usage, and replace pushd/popd with cd and manual directory saving/restoring).
- Around line 125-138: The script currently uses "local doExit=0" in the MAIN
section which is invalid outside functions and causes errors with set -u; change
this to a normal variable assignment (doExit=0) before the download_check calls,
ensuring doExit is initialized in the MAIN scope (refer to the doExit variable
and the download_check "$aName" "$aBaseURL" "$aSHAName" / aBaseURLFallback
logic) so subsequent checks like if [[ ${doExit} -eq 1 ]] work without
unbound-variable errors.
| download_check () { | ||
| local name="$1" | ||
| local base="$2" | ||
| local sha="$3" | ||
|
|
||
| # Required to download | ||
| rm -f $name | ||
| dURL="$base/$name" | ||
| download $dURL | ||
| check_download $name $sha | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the shebang and local usage in run.sh
head -1 crc-support/oci/lib/unix/run.sh
echo "---"
# Count usage of 'local' keyword in run.sh
rg -n '\blocal\b' crc-support/oci/lib/unix/run.sh | head -20Repository: crc-org/ci-definitions
Length of output: 236
🏁 Script executed:
# Search for how run.sh is invoked
rg -n '\brun\.sh\b' --glob '!**/vendor/**' --glob '!**/.git/**'Repository: crc-org/ci-definitions
Length of output: 1512
🏁 Script executed:
# Check for other bash-specific features in run.sh
rg -n '\[|\[\[|function|local|pushd|popd' crc-support/oci/lib/unix/run.sh | head -30Repository: crc-org/ci-definitions
Length of output: 813
Update shebang to #!/bin/bash or refactor the script to use only POSIX shell features.
The script uses multiple bash-specific features ([[ ]] conditionals, local variables, ${BASH_SOURCE[0]}, pushd/popd) but declares #!/bin/sh. This will fail on systems where /bin/sh is not bash (Alpine Linux, dash, ash). Either change the shebang to #!/bin/bash to match the actual requirements, or refactor the code to use only POSIX-compliant features.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 89-89: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 90-90: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 91-91: In POSIX sh, 'local' is undefined.
(SC3043)
🤖 Prompt for AI Agents
In `@crc-support/oci/lib/unix/run.sh` around lines 88 - 98, The script declares
#!/bin/sh but uses Bash-only features; update the shebang to #!/bin/bash or
refactor to POSIX shell; specifically, change the top-level shebang line or
eliminate Bash-specific constructs used across functions such as download_check
(which uses local), any uses of [[ ... ]], ${BASH_SOURCE[0]}, and pushd/popd so
the script is either explicitly run with /bin/bash or rewritten to use
POSIX-compatible alternatives (use plain variable assignments, replace [[ ]]
with test/[ ], remove ${BASH_SOURCE[0]} usage, and replace pushd/popd with cd
and manual directory saving/restoring).
| local doExit=0 | ||
| download_check "$aName" "$aBaseURL" "$aSHAName" | ||
| if [[ ${?} -ne 0 ]]; then | ||
| doExit=1 | ||
| if [[ ${aBaseURLFallback} != '' ]]; then | ||
| doExit=0 | ||
| echo "Error with downloading $aName, using fallback" | ||
| download_check "$aName" "$aBaseURLFallback" "$aSHAName" | ||
| if [[ ${?} -ne 0 ]]; then | ||
| doExit=1 | ||
| fi | ||
| fi | ||
| fi | ||
| if [[ ${doExit} -eq 1 ]]; then |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Get the file and examine context around line 125
cat -n crc-support/oci/lib/unix/run.sh | sed -n '100,150p'Repository: crc-org/ci-definitions
Length of output: 1635
🏁 Script executed:
# Check for function boundaries by looking for function declarations
rg -n 'function |^[a-zA-Z_][a-zA-Z0-9_]*\(\)' crc-support/oci/lib/unix/run.sh | head -30Repository: crc-org/ci-definitions
Length of output: 48
🏁 Script executed:
# Check if set -u is used in the script
rg -n 'set -u|set.*-u' crc-support/oci/lib/unix/run.shRepository: crc-org/ci-definitions
Length of output: 48
Remove local from doExit initialization.
At line 125, local is used outside a function in the MAIN section of the script, which is invalid syntax and causes local: can only be used in a function. With set -u enabled (line 104), this will trigger unbound-variable errors. Initialize it as a normal variable instead.
🔧 Proposed fix
- local doExit=0
+ doExit=0🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 125-125: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 127-127: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 129-129: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 133-133: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 138-138: In POSIX sh, [[ ]] is undefined.
(SC3010)
🤖 Prompt for AI Agents
In `@crc-support/oci/lib/unix/run.sh` around lines 125 - 138, The script currently
uses "local doExit=0" in the MAIN section which is invalid outside functions and
causes errors with set -u; change this to a normal variable assignment
(doExit=0) before the download_check calls, ensuring doExit is initialized in
the MAIN scope (refer to the doExit variable and the download_check "$aName"
"$aBaseURL" "$aSHAName" / aBaseURLFallback logic) so subsequent checks like if
[[ ${doExit} -eq 1 ]] work without unbound-variable errors.
|
This fixes a problem if internal url for bundles is cleanup, and public mirror already contains same bundles. checksum file might be different, but to avoid extra parameters it is not added still. I'm reluctant to apply
|
albfan
force-pushed
the
download-fallback
branch
from
8879277 to
5af4531
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crc-support/oci/lib/unix/run.sh (1)
120-138: Fallback still fails if the primary URL is gone.Two blockers prevent the fallback from working in the “primary cleaned up” case:
download()retries forever, sodownload_checknever returns a failure to trigger fallback.- The SHA file is always fetched from the primary base; if it’s missing there, fallback still fails even when the asset exists on the mirror.
Consider bounding retries and fetching the checksum from the same base used for the asset.
🛠️ Possible fix
- # Download sha256sum - curl --insecure -LO "$aBaseURL/$aSHAName" + # sha256sum downloaded per-base inside download_checkdownload_check () { local name="$1" local base="$2" local sha="$3" # Required to download rm -f $name dURL="$base/$name" - download $dURL - check_download $name $sha + download "$dURL" || return 1 + curl --insecure -LO "$base/$sha" || return 1 + check_download "$name" "$sha" }download () { - local binary_url="${1}" - local download_result=1 - while [[ ${download_result} -ne 0 ]] - do - curl --insecure -LO -C - ${binary_url} - download_result=$? - done + local binary_url="${1}" + local max_retries="${2:-5}" + local attempt=1 + while true; do + curl --insecure -LO -C - "${binary_url}" && return 0 + if [ "$attempt" -ge "$max_retries" ]; then + return 1 + fi + attempt=$((attempt + 1)) + sleep 2 + done }
|
@albfan windows script also needs to update |
2 participants
Allow to check alternative download url if provided.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.