Skip to content

Do not fail remove obsolete functions #420

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
phracek wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Do not fail remove obsolete functions #420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e98c807
Remove `set -e` flag from the script
phracek Mar 9, 2026
28044c9
Let's try to build container in case of RHEL variants
phracek Mar 9, 2026
0647c9e
Disable warning as `set -e` was removed
phracek Mar 9, 2026
ed2140c
The suite has to fail in case second build failed as well.
phracek Mar 10, 2026
644f611
Specify max_attempts instead of hard-coded value.
phracek Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 49 additions & 82 deletions build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
# SINGLE_VERSION - Specifies the image version - (must match with subdirectory in repo)
# VERSIONS - Must be set to a list with possible versions (subdirectories)

set -eE
set -E
[ -n "${DEBUG:-}" ] && set -x

# shellcheck shell=bash
Expand All @@ -21,53 +21,8 @@ error() { echo "ERROR: $*" ; false ; }

trap 'echo "errexit on line $LINENO, $0" >&2' ERR

MAX_BUILD_ATTEMPTS=2

# _parse_output_inner
# -------------------
# Helper function for 'parse_output'.
# We need to avoid case statements in $() for older Bash versions (per issue
# postgresql-container#35, mac ships with 3.2).
# Example of problematic statement: echo $(case i in i) echo i;; esac)
_parse_output_inner ()
{
set -o pipefail
{
case $stream in
stdout|1|"")
eval "$command" | tee >(cat - >&"$stdout_fd")
;;
stderr|2)
set +x # avoid stderr pollution
eval "$command" {free_fd}>&1 1>&"$stdout_fd" 2>&"$free_fd" | tee >(cat - >&"$stderr_fd")
;;
esac
# Inherit correct exit status.
(exit "${PIPESTATUS[0]}")
} | eval "$filter"
}


# parse_output COMMAND FILTER_COMMAND OUTVAR [STREAM={stderr|stdout}]
# -------------------------------------------------------------------
# Parse standard (error) output of COMMAND with FILTER_COMMAND and store the
# output into variable named OUTVAR. STREAM might be 'stdout' or 'stderr',
# defaults to 'stdout'. The filtered output stays (live) printed to terminal.
# This method doesn't create any explicit temporary files.
# Defines:
# ${$OUTVAR}: Set to FILTER_COMMAND output.
parse_output ()
{
local command=$1 filter=$2 var=$3 stream=$4
echo "-> building using $command"
local raw_output='' rc=0
{
# shellcheck disable=SC2034
raw_output=$(_parse_output_inner)
} {stdout_fd}>&1 {stderr_fd}>&2
rc=$?
eval "$var=\$raw_output"
(exit $rc)
}

# "best-effort" cleanup of image
function clean_image {
Expand Down Expand Up @@ -196,44 +151,54 @@ function docker_build_with_version {
if [[ "$SKIP_SQUASH" -eq 0 ]] && [[ "$is_podman" -eq 1 ]]; then
BUILD_OPTIONS+=" --squash"
fi

command="docker build ${BUILD_OPTIONS} -f $dockerfile ${DOCKER_BUILD_CONTEXT}"
echo "-> building using $command"
set +x -o pipefail
tmp_file=$(mktemp "/tmp/${dir}-${OS}.XXXXXX")
$command 2>&1 | tee "$tmp_file"
ret_code=$?
set -x +o pipefail
echo "Return code from docker build is '$ret_code'."
last_row=$(< "$tmp_file" tail -n 1)
if [[ $ret_code != "0" ]]; then
if [[ "${OS}" == "rhel8" ]] || [[ "${OS}" == "rhel9" ]] || [[ "${OS}" == "rhel10" ]]; then
# Do not fail in case of sending log to pastebin or logdetective fails.
set +e
analyze_logs_by_logdetective "${tmp_file}"
set -e
fi
else
# Structure of log build is as follows:
# COMMIT
# --> e191d12b5928
# e191d12b5928360dd6024fe80d31e08f994d42577f76b9b143e014749afc8ab4
# shellcheck disable=SC2016
if [[ "$last_row" =~ (^-->)?(Using cache )?[a-fA-F0-9]+$ ]]; then
IMAGE_ID="$last_row"
i=1
build_failed=1
while [ $i -le $MAX_BUILD_ATTEMPTS ]; do
command="docker build ${BUILD_OPTIONS} -f $dockerfile ${DOCKER_BUILD_CONTEXT}"
echo "-> building using $command"
set +x -o pipefail
tmp_file=$(mktemp "/tmp/${dir}-${OS}.XXXXXX")
$command 2>&1 | tee "$tmp_file"
ret_code=$?
set -x +o pipefail
echo "Return code from docker build is '$ret_code'."
last_row=$(< "$tmp_file" tail -n 1)
if [[ $ret_code != "0" ]]; then
# In case of failure, we want to analyze the logs and send them to pastebin or logdetective.
# The failure can be ethel issue like network failure or registry failure.
# Red Hat Enterprise Linux 9 for x86_64 - AppStre 0.0 B/s | 0 B 00:00
# Errors during downloading metadata for repository 'rhel-9-for-x86_64-appstream-rpms':
# - Curl error (56): Failure when receiving data from the peer for https:// [Received HTTP code 503 from proxy after CONNECT]
# Error: Failed to download metadata for repo 'rhel-9-for-x86_64-appstream-rpms':
if [[ "${OS}" == "rhel8" ]] || [[ "${OS}" == "rhel9" ]] || [[ "${OS}" == "rhel10" ]]; then
# Do not fail in case of sending log to pastebin or logdetective fails.
analyze_logs_by_logdetective "${tmp_file}"
fi
((i++))
sleep 5
echo "Retrying to build image for version $dir, attempt $i of $MAX_BUILD_ATTEMPTS."
Comment on lines +177 to +179
Copy link

@coderabbitai coderabbitai bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Skip the backoff once you've exhausted the attempts.

On the last failure this still increments i, waits 5 seconds, and logs attempt 3 of 2 before exiting. That makes permanent failures slower and the retry logs inaccurate.

Possible fix 
-      ((i++))
-      sleep 5
-      echo "Retrying to build image for version $dir, attempt $i of $MAX_BUILD_ATTEMPTS."
+      if [ $i -eq $MAX_BUILD_ATTEMPTS ]; then
+        break
+      fi
+      ((i++))
+      echo "Retrying to build image for version $dir, attempt $i of $MAX_BUILD_ATTEMPTS."
+      sleep 5
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
((i++))
sleep 5
echo "Retrying to build image for version $dir, attempt $i of $MAX_BUILD_ATTEMPTS."
if [ $i -eq $MAX_BUILD_ATTEMPTS ]; then
break
fi
((i++))
echo "Retrying to build image for version $dir, attempt $i of $MAX_BUILD_ATTEMPTS."
sleep 5
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@build.sh` around lines 177 - 179, The loop increments i, sleeps, and echoes a
retry even after reaching MAX_BUILD_ATTEMPTS causing incorrect logs and extra
delay; change the logic around i, MAX_BUILD_ATTEMPTS, sleep, echo and dir so
that you only increment, sleep 5 and print the "Retrying..." message when i is
still less than MAX_BUILD_ATTEMPTS (or break immediately when i >=
MAX_BUILD_ATTEMPTS) to avoid waiting and logging a nonexistent attempt number on
the final failure.

else
# Structure of log build is as follows:
# COMMIT
# --> e191d12b5928
# e191d12b5928360dd6024fe80d31e08f994d42577f76b9b143e014749afc8ab4
# shellcheck disable=SC2016
if [[ "$last_row" =~ (^-->)?(Using cache )?[a-fA-F0-9]+$ ]]; then
IMAGE_ID="$last_row"
fi
echo "$IMAGE_ID" > .image-id
tag_image
build_failed=0
break
Comment on lines +186 to +192
Copy link

@coderabbitai coderabbitai bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Don't declare success until tagging has succeeded.

After set -e was removed, this branch still sets build_failed=0 and breaks even when IMAGE_ID cannot be parsed or tag_image returns non-zero. That can turn a broken tag/push path into a green build.

Possible fix 
-      if [[ "$last_row" =~ (^-->)?(Using cache )?[a-fA-F0-9]+$ ]]; then
-        IMAGE_ID="$last_row"
-      fi
-      echo "$IMAGE_ID" > .image-id
-      tag_image
-      build_failed=0
+      if [[ "$last_row" =~ ([a-fA-F0-9]+)$ ]]; then
+        IMAGE_ID="${BASH_REMATCH[1]}"
+      else
+        echo "-> Failed to determine image ID for version $dir and OS $OS."
+        exit 1
+      fi
+      echo "$IMAGE_ID" > .image-id || exit 1
+      tag_image || exit 1
+      build_failed=0
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@build.sh` around lines 186 - 192, The branch assumes success
prematurely—ensure IMAGE_ID was actually parsed and that tag_image succeeded
before declaring success: after matching last_row, assign IMAGE_ID and write
.image-id, then call tag_image and check its exit status (or test its return
value); only if tag_image returns success set build_failed=0 and break;
otherwise leave build_failed as non-zero (or continue/exit appropriately) so
failures in tag_image or missing IMAGE_ID don't yield a green build. Reference
symbols: last_row, IMAGE_ID, .image-id, tag_image, build_failed.

fi
fi

rm -f "$tmp_file"

# shellcheck disable=SC2016
rm -f "$tmp_file"
done
if [[ $build_failed -ne 0 ]]; then
echo "-> Build failed for version $dir and OS $OS after 2 attempts, giving up."
exit 1
fi

# parse_output 'docker build '"$BUILD_OPTIONS"' -f "$dockerfile" "${DOCKER_BUILD_CONTEXT}"' \
# "tail -n 1 | awk '/Successfully built|(^--> )?(Using cache )?[a-fA-F0-9]+$/{print \$NF}'" \
# IMAGE_ID
# analyze_logs_by_logdetective "$?" "${tmp_file}"
echo "$IMAGE_ID" > .image-id
tag_image
}

function tag_image {
Expand Down Expand Up @@ -276,7 +241,9 @@ fi
echo "Built versions are: $dirs"

for dir in ${dirs}; do
# shellcheck disable=SC2164
pushd "${dir}" > /dev/null
docker_build_with_version Dockerfile."$OS"
# shellcheck disable=SC2164
popd > /dev/null
Comment on lines +244 to 248
Copy link

@coderabbitai coderabbitai bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Make directory changes explicit failures again.

With errexit gone, a failed pushd leaves the script in the previous directory and the next docker_build_with_version runs against the wrong tree. popd has the same problem with the directory stack, so suppressing SC2164 hides a real control-flow bug here.

Possible fix 
-  # shellcheck disable=SC2164
-  pushd "${dir}" > /dev/null
+  pushd "${dir}" > /dev/null || exit 1
   docker_build_with_version Dockerfile."$OS"
-  # shellcheck disable=SC2164
-  popd > /dev/null
+  popd > /dev/null || exit 1
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# shellcheck disable=SC2164
pushd "${dir}" > /dev/null
docker_build_with_version Dockerfile."$OS"
# shellcheck disable=SC2164
popd > /dev/null
pushd "${dir}" > /dev/null || exit 1
docker_build_with_version Dockerfile."$OS"
popd > /dev/null || exit 1
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@build.sh` around lines 244 - 248, The pushd/popd calls around
docker_build_with_version must abort on failure instead of suppressing SC2164;
replace the silent pushd "${dir}" > /dev/null and popd > /dev/null with explicit
failure checks (e.g., if ! pushd "${dir}" > /dev/null; then echo "pushd ${dir}
failed" >&2; exit 1; fi and similarly for popd) or add "|| exit 1" after each
call, remove the SC2164 suppression, and leave the docker_build_with_version
Dockerfile."$OS" invocation unchanged so it always runs only when pushd
succeeded.

done
2 changes: 1 addition & 1 deletion common.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ copy_md_files:
remove_md_files:
@for version in $(VERSIONS); do \
if ls $$version/*.md 1> /dev/null 2>&1; then \
rm -v $$version/root/*.md ; \
rm -fv $$version/root/*.md ; \
fi ; \
done

Expand Down
Loading