Copilot commented on code in PR #67:
URL:
https://github.com/apache/grails-github-actions/pull/67#discussion_r2969544502
##########
deploy-github-pages/entrypoint.sh:
##########
@@ -316,6 +316,21 @@ git status
echo "Committing changes."
git commit -m "Deploying to documentation branch - $(date +"%T")" --quiet
--allow-empty
-git push "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}"
-echo "Deployment successful!"
+
+MAX_PUSH_ATTEMPTS=5
+PUSH_ATTEMPT=1
+while [ $PUSH_ATTEMPT -le $MAX_PUSH_ATTEMPTS ]; do
+ echo "Push attempt ${PUSH_ATTEMPT}/${MAX_PUSH_ATTEMPTS}"
+ if git push "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}" 2>&1; then
+ echo "Deployment successful!"
+ break
+ fi
+ if [ $PUSH_ATTEMPT -eq $MAX_PUSH_ATTEMPTS ]; then
+ echo "ERROR: Push failed after ${MAX_PUSH_ATTEMPTS} attempts." >&2
+ exit 1
+ fi
+ echo "Push rejected, pulling remote changes and retrying..."
+ git pull --rebase "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}"
+ PUSH_ATTEMPT=$((PUSH_ATTEMPT + 1))
Review Comment:
The log message says "Push rejected" unconditionally, but this branch runs
for any push error. Consider making the message reflect the actual failure
reason (from the captured push output) and/or only printing "rejected" when the
error indicates a non-fast-forward rejection.
```suggestion
PUSH_OUTPUT=$(git push "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}" 2>&1)
PUSH_STATUS=$?
echo "${PUSH_OUTPUT}"
if [ $PUSH_STATUS -eq 0 ]; then
echo "Deployment successful!"
break
fi
if [ $PUSH_ATTEMPT -eq $MAX_PUSH_ATTEMPTS ]; then
echo "ERROR: Push failed after ${MAX_PUSH_ATTEMPTS} attempts." >&2
echo " Last push output:" >&2
echo "${PUSH_OUTPUT}" >&2
exit 1
fi
if echo "${PUSH_OUTPUT}" | grep -qiE 'rejected|non-fast-forward'; then
echo "Push rejected (non-fast-forward), pulling remote changes and
retrying..."
git pull --rebase "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}"
PUSH_ATTEMPT=$((PUSH_ATTEMPT + 1))
else
echo "ERROR: Push failed with a non-rejection error; not retrying." >&2
echo " Push output:" >&2
echo "${PUSH_OUTPUT}" >&2
exit 1
fi
```
##########
deploy-github-pages/entrypoint.sh:
##########
@@ -316,6 +316,21 @@ git status
echo "Committing changes."
git commit -m "Deploying to documentation branch - $(date +"%T")" --quiet
--allow-empty
-git push "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}"
-echo "Deployment successful!"
+
+MAX_PUSH_ATTEMPTS=5
+PUSH_ATTEMPT=1
+while [ $PUSH_ATTEMPT -le $MAX_PUSH_ATTEMPTS ]; do
+ echo "Push attempt ${PUSH_ATTEMPT}/${MAX_PUSH_ATTEMPTS}"
+ if git push "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}" 2>&1; then
+ echo "Deployment successful!"
+ break
+ fi
+ if [ $PUSH_ATTEMPT -eq $MAX_PUSH_ATTEMPTS ]; then
+ echo "ERROR: Push failed after ${MAX_PUSH_ATTEMPTS} attempts." >&2
+ exit 1
+ fi
+ echo "Push rejected, pulling remote changes and retrying..."
+ git pull --rebase "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}"
Review Comment:
To reduce repeated collisions when multiple jobs are retrying at the same
time, consider adding a short backoff (optionally with jitter) before the `git
pull --rebase`/next push attempt. Without a delay, concurrent runners can
remain in lockstep and continue to race on each retry.
```suggestion
git pull --rebase "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}"
# Add a backoff with jitter before the next push attempt to reduce
repeated collisions
BACKOFF_BASE_SECONDS=2
JITTER=$((RANDOM % 3)) # 0–2 seconds of jitter
BACKOFF_SECONDS=$((BACKOFF_BASE_SECONDS * PUSH_ATTEMPT + JITTER))
echo "Waiting ${BACKOFF_SECONDS}s before next push attempt..."
sleep "${BACKOFF_SECONDS}"
```
##########
deploy-github-pages/entrypoint.sh:
##########
@@ -316,6 +316,21 @@ git status
echo "Committing changes."
git commit -m "Deploying to documentation branch - $(date +"%T")" --quiet
--allow-empty
-git push "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}"
-echo "Deployment successful!"
+
+MAX_PUSH_ATTEMPTS=5
+PUSH_ATTEMPT=1
+while [ $PUSH_ATTEMPT -le $MAX_PUSH_ATTEMPTS ]; do
+ echo "Push attempt ${PUSH_ATTEMPT}/${MAX_PUSH_ATTEMPTS}"
+ if git push "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}" 2>&1; then
+ echo "Deployment successful!"
+ break
+ fi
Review Comment:
The retry logic currently treats any `git push` failure as a
concurrent-update rejection and immediately proceeds to pull/rebase. This can
mask non-retryable failures (auth/permission issues, network errors, wrong
refspec) and wastes up to 5 attempts before failing. Consider capturing the
push output and only retrying when it matches a non-fast-forward rejection
(e.g., "[rejected]" / "fetch first"); otherwise surface the error and exit
immediately.
##########
deploy-github-pages/entrypoint.sh:
##########
@@ -316,6 +316,21 @@ git status
echo "Committing changes."
git commit -m "Deploying to documentation branch - $(date +"%T")" --quiet
--allow-empty
-git push "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}"
-echo "Deployment successful!"
+
+MAX_PUSH_ATTEMPTS=5
+PUSH_ATTEMPT=1
+while [ $PUSH_ATTEMPT -le $MAX_PUSH_ATTEMPTS ]; do
+ echo "Push attempt ${PUSH_ATTEMPT}/${MAX_PUSH_ATTEMPTS}"
+ if git push "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}" 2>&1; then
Review Comment:
`2>&1` on the `git push` here doesn't capture output; it only redirects
stderr to stdout. If you intend to inspect the failure reason (to decide
whether to retry) or include diagnostics in the final error, store the output
in a variable and print it on failure; otherwise drop the redirection so errors
remain on stderr.
```suggestion
if git push "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}"; then
```
##########
deploy-github-pages/entrypoint.sh:
##########
@@ -316,6 +316,21 @@ git status
echo "Committing changes."
git commit -m "Deploying to documentation branch - $(date +"%T")" --quiet
--allow-empty
-git push "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}"
-echo "Deployment successful!"
+
+MAX_PUSH_ATTEMPTS=5
+PUSH_ATTEMPT=1
+while [ $PUSH_ATTEMPT -le $MAX_PUSH_ATTEMPTS ]; do
+ echo "Push attempt ${PUSH_ATTEMPT}/${MAX_PUSH_ATTEMPTS}"
+ if git push "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}" 2>&1; then
+ echo "Deployment successful!"
+ break
+ fi
+ if [ $PUSH_ATTEMPT -eq $MAX_PUSH_ATTEMPTS ]; then
Review Comment:
Minor: for numeric loop conditions, `(( PUSH_ATTEMPT <= MAX_PUSH_ATTEMPTS
))` avoids word-splitting issues and is more idiomatic in bash than `[
$PUSH_ATTEMPT -le $MAX_PUSH_ATTEMPTS ]`.
```suggestion
while (( PUSH_ATTEMPT <= MAX_PUSH_ATTEMPTS )); do
echo "Push attempt ${PUSH_ATTEMPT}/${MAX_PUSH_ATTEMPTS}"
if git push "${GIT_REPO_URL}" "${DOCUMENTATION_BRANCH}" 2>&1; then
echo "Deployment successful!"
break
fi
if (( PUSH_ATTEMPT == MAX_PUSH_ATTEMPTS )); then
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
