Conversation
mgreau
left a comment
There was a problem hiding this comment.
The PR adds retry with exponential backoff functionality to git-checkout, which is a useful feature for handling transient network failures. However, there are a few issues that should be addressed:
-
Logging regression: The git clone command is no longer wrapped with
vr(verbose run), so the command won't be logged with the 'execute:' prefix, making debugging harder. -
Missing input validation: The numeric parameters (max-retries, initial-backoff, max-backoff) are not validated, which could cause cryptic arithmetic errors if invalid values are passed.
The retry logic itself is well-implemented with proper exponential backoff and jitter.
| msg "Attempting git clone with retry (max_retries=$max_retries, initial_backoff=${initial_backoff}s, max_backoff=${max_backoff}s)" | ||
| retry_with_backoff "$max_retries" "$initial_backoff" "$max_backoff" \ | ||
| git clone $quiet "--origin=$remote" \ | ||
| "--config=user.name=Melange Build" \ | ||
| "--config=user.email=melange-build@cgr.dev" \ | ||
| $flags \ | ||
| ${depthflag:+"$depthflag"} "$repo" "$workdir" | ||
| ${depthflag:+"$depthflag"} "$repo" "$workdir" || |
There was a problem hiding this comment.
ERROR: The git clone command is no longer logged because it's passed directly to retry_with_backoff without the vr wrapper. This is a regression in debugging capability. The command should be logged at least once before retrying. Consider using vr inside the retry or logging the command explicitly.
| msg "Attempting git clone with retry (max_retries=$max_retries, initial_backoff=${initial_backoff}s, max_backoff=${max_backoff}s)" | |
| retry_with_backoff "$max_retries" "$initial_backoff" "$max_backoff" \ | |
| git clone $quiet "--origin=$remote" \ | |
| "--config=user.name=Melange Build" \ | |
| "--config=user.email=melange-build@cgr.dev" \ | |
| $flags \ | |
| ${depthflag:+"$depthflag"} "$repo" "$workdir" | |
| ${depthflag:+"$depthflag"} "$repo" "$workdir" || | |
| msg "Attempting git clone with retry (max_retries=$max_retries, initial_backoff=${initial_backoff}s, max_backoff=${max_backoff}s)" | |
| msg "execute: git clone $quiet --origin=$remote --config=user.name=Melange Build --config=user.email=melange-build@cgr.dev $flags ${depthflag:+$depthflag} $repo $workdir" | |
| retry_with_backoff "$max_retries" "$initial_backoff" "$max_backoff" \ | |
| git clone $quiet "--origin=$remote" \ | |
| "--config=user.name=Melange Build" \ | |
| "--config=user.email=melange-build@cgr.dev" \ | |
| $flags \ | |
| ${depthflag:+"$depthflag"} "$repo" "$workdir" || |
| "tag='$tag' expcommit='$expcommit' recurse='$recurse'" \ | ||
| "sparse_paths='$sparse_paths'" | ||
| "sparse_paths='$sparse_paths' max_retries='$max_retries'" \ | ||
| "initial_backoff='$initial_backoff' max_backoff='$max_backoff'" | ||
|
|
||
| case "$recurse" in |
There was a problem hiding this comment.
ERROR: The numeric parameters (max_retries, initial_backoff, max_backoff) are not validated. If non-numeric values are passed, the script will fail with cryptic arithmetic errors. Add validation to ensure these are positive integers.
| "tag='$tag' expcommit='$expcommit' recurse='$recurse'" \ | |
| "sparse_paths='$sparse_paths'" | |
| "sparse_paths='$sparse_paths' max_retries='$max_retries'" \ | |
| "initial_backoff='$initial_backoff' max_backoff='$max_backoff'" | |
| case "$recurse" in | |
| local max_retries="${10:-3}" initial_backoff="${11:-2}" max_backoff="${12:-60}" | |
| # Validate numeric parameters | |
| case "$max_retries" in | |
| ''|*[!0-9]*) fail "max-retries must be a non-negative integer, got '$max_retries'";; | |
| esac | |
| case "$initial_backoff" in | |
| ''|*[!0-9]*) fail "initial-backoff must be a non-negative integer, got '$initial_backoff'";; | |
| esac | |
| case "$max_backoff" in | |
| ''|*[!0-9]*) fail "max-backoff must be a non-negative integer, got '$max_backoff'";; | |
| esac | |
| msg "repo='$repo' dest='$dest' depth='$depth' branch='$branch'" \ |
Melange Pull Request Template
This PR is a copy of chainguard-dev#2301 for testing the code-reviewer CLI.
Functional Changes
Notes:
SCA Changes
Notes:
Linter
Notes: