OCPEDGE-2396: Add containerized resource-agents build tooling#60
Open
vimauro wants to merge 1 commit intoopenshift-eng:mainfrom
Open
OCPEDGE-2396: Add containerized resource-agents build tooling#60vimauro wants to merge 1 commit intoopenshift-eng:mainfrom
vimauro wants to merge 1 commit intoopenshift-eng:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vimauro: This pull request references OCPEDGE-2396 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vimauro The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughAdded containerized build validation tooling and documentation for resource-agents RPMs targeting CentOS Stream 9 and 10. Includes two Dockerfiles with build pipelines, a local Podman build orchestration script, and README documentation describing usage and build behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (4)
helpers/resource-agents-build/Dockerfile.stream10 (2)
17-19: Optional: Adddnf clean allfor consistency with best practices.Same consideration as
Dockerfile.stream9- addingdnf clean allwould reduce image size, though it's optional for a local build/test image.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helpers/resource-agents-build/Dockerfile.stream10` around lines 17 - 19, Add a cleanup step after the package installation RUN commands to reduce image size: locate the RUN lines that call "dnf install -y epel-release ..." and the long "dnf install -y git autoconf ..." and append a "dnf clean all" (or add a separate RUN dnf clean all) after those installs so cached metadata and package files are removed; ensure the cleanup is placed in the same layer or immediately following the install to maintain consistency with the Dockerfile.stream10 install steps.
22-25: Error suppression may hide legitimate failures.Redirecting stderr with
2>/dev/nullhides all errors, not just "package not found." Network issues or repo misconfigurations would silently fall through to the source build. Consider checking for the specific exit code or package availability first.Proposed improvement
# TODO: remove libqb source build once libqb-devel is available in EPEL/CRB for Stream 10 # Check: podman run --rm quay.io/centos/centos:stream10 dnf repoquery libqb-devel --available -RUN (dnf install -y libqb-devel 2>/dev/null) || ( \ +RUN dnf install -y libqb-devel || ( \ + echo "libqb-devel not available, building from source..." && \ git clone --depth 1 https://github.com/ClusterLabs/libqb /tmp/libqb-build \ && cd /tmp/libqb-build && autoreconf -fi && ./configure --prefix=/usr && make -j$(nproc) && make install \ && cd / && rm -rf /tmp/libqb-build )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helpers/resource-agents-build/Dockerfile.stream10` around lines 22 - 25, The RUN line currently swallows all dnf errors by redirecting stderr to /dev/null, which can hide real failures; change the logic so you first check package availability (e.g., run a query like dnf list available libqb-devel or similar check) and only fall back to the git clone/build when the package is truly unavailable, and when falling back propagate or log the original dnf error output instead of discarding it; update the RUN block that references libqb-devel and the git clone /tmp/libqb-build fallback to perform the availability check, branch on that result, and ensure any non-package-not-found errors are surfaced (logged or cause failure) rather than being ignored.helpers/resource-agents-build/Dockerfile.stream9 (2)
12-14: Optional: Adddnf clean allto reduce image size.Static analysis flags that
dnf clean allis missing after installs. While this is a local build/test container where size is less critical, adding cleanup could be beneficial if images are retained.Proposed change
RUN dnf config-manager --set-enabled crb || true -RUN dnf install -y epel-release || dnf install -y https://dl.fedoraproject.org/pub/epel/epel-release-latest-9.noarch.rpm -RUN dnf install -y git autoconf automake docbook-style-xsl glib2-devel libqb-devel make rpm-build perl python3-devel libnet-devel python3-pyroute2 libxslt systemd which +RUN dnf install -y epel-release || dnf install -y https://dl.fedoraproject.org/pub/epel/epel-release-latest-9.noarch.rpm \ + && dnf install -y git autoconf automake docbook-style-xsl glib2-devel libqb-devel make rpm-build perl python3-devel libnet-devel python3-pyroute2 libxslt systemd which \ + && dnf clean all🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helpers/resource-agents-build/Dockerfile.stream9` around lines 12 - 14, Add a cleanup step to the package-install RUN commands in Dockerfile.stream9 to reduce image size: after the dnf install lines (the RUN commands that call "dnf install -y epel-release ..." and "dnf install -y git autoconf ..."), append a cleanup command such as "dnf clean all && rm -rf /var/cache/dnf /var/cache/yum" (or combine into a single RUN so cache isn't persisted across layers) to remove metadata and caches.
25-26: Consider adding explicit error handling if no RPM is found.If
findreturns no matches,xargs -I{}executes nothing and the subsequenttest -ffails with an opaque error. Adding an explicit check would improve debuggability.Proposed improvement
-RUN find . -name 'resource-agents-*.rpm' ! -name '*debug*' ! -name '*.src.rpm' | head -1 | xargs -I{} cp {} /tmp/resource-agents.rpm -RUN test -f /tmp/resource-agents.rpm && rpm -qip /tmp/resource-agents.rpm +RUN RPM_PATH=$(find . -name 'resource-agents-*.rpm' ! -name '*debug*' ! -name '*.src.rpm' | head -n 1) \ + && if [ -z "$RPM_PATH" ]; then echo "ERROR: No RPM artifact found after build"; exit 1; fi \ + && cp "$RPM_PATH" /tmp/resource-agents.rpm \ + && rpm -qip /tmp/resource-agents.rpm🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helpers/resource-agents-build/Dockerfile.stream9` around lines 25 - 26, The Dockerfile's two RUN steps should explicitly fail with a clear error if no RPM is found: replace the pipeline that runs find | xargs -I{} cp {} /tmp/resource-agents.rpm with logic that captures the first match into a shell variable (e.g., FILE or RA_RPM), test if that variable is empty, and if so echo a descriptive error to stderr and exit 1; otherwise cp "$FILE" /tmp/resource-agents.rpm and then run rpm -qip /tmp/resource-agents.rpm. Ensure you reference the same target path /tmp/resource-agents.rpm so the subsequent rpm inspection only runs when the file exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helpers/resource-agents-build/Dockerfile.stream10`:
- Around line 28-30: The shallow clone uses "--depth 1" in the RUN command
cloning "${RESOURCE_AGENTS_REPO}" to /tmp/resource-agents-build which can fail
when checking out non-default refs; update the RUN line that contains git clone
--depth 1 "${RESOURCE_AGENTS_REPO}" /tmp/resource-agents-build so it does a full
clone (remove the "--depth 1" flag) or replace with an init/remote-add/fetch
sequence that fetches the specific "${RESOURCE_AGENTS_REF}" into
/tmp/resource-agents-build before running git -C /tmp/resource-agents-build
checkout FETCH_HEAD, ensuring RESOURCE_AGENTS_REPO and RESOURCE_AGENTS_REF are
used as in the existing commands.
In `@helpers/resource-agents-build/Dockerfile.stream9`:
- Around line 17-19: The shallow git clone using --depth 1 can prevent fetching
non-default branches/tags/SHAs; update the RUN git clone/fetch sequence so the
clone supports arbitrary refs referenced by RESOURCE_AGENTS_REF (e.g., remove
--depth 1 or add --no-single-branch to the git clone, or after cloning run git
fetch --unshallow || git fetch --depth=1 origin "${RESOURCE_AGENTS_REF}" before
git checkout FETCH_HEAD) so that the subsequent git -C
/tmp/resource-agents-build fetch origin "${RESOURCE_AGENTS_REF}" and git -C
/tmp/resource-agents-build checkout FETCH_HEAD succeed reliably for tags, SHAs,
and non-default branches.
In `@helpers/resource-agents-build/local-build-test.sh`:
- Around line 29-36: The argument parsing in the while/case block doesn't
validate that an option value exists for --repo and --ref and calls usage with a
zero exit on unknown options; update the case arms for --repo and --ref in the
while loop to first check that a next argument exists and is not another option
(i.e., test that "$2" is set and not starting with '-') and if missing print a
clear error referencing RESOURCE_AGENTS_REPO or RESOURCE_AGENTS_REF and exit
non‑zero; also change the default unknown option path (and any usage invocation
for invalid input) to print the error and call usage with a non‑zero exit code
so failures are signalled (reference the while loop, case pattern,
RESOURCE_AGENTS_REPO, RESOURCE_AGENTS_REF and usage).
---
Nitpick comments:
In `@helpers/resource-agents-build/Dockerfile.stream10`:
- Around line 17-19: Add a cleanup step after the package installation RUN
commands to reduce image size: locate the RUN lines that call "dnf install -y
epel-release ..." and the long "dnf install -y git autoconf ..." and append a
"dnf clean all" (or add a separate RUN dnf clean all) after those installs so
cached metadata and package files are removed; ensure the cleanup is placed in
the same layer or immediately following the install to maintain consistency with
the Dockerfile.stream10 install steps.
- Around line 22-25: The RUN line currently swallows all dnf errors by
redirecting stderr to /dev/null, which can hide real failures; change the logic
so you first check package availability (e.g., run a query like dnf list
available libqb-devel or similar check) and only fall back to the git
clone/build when the package is truly unavailable, and when falling back
propagate or log the original dnf error output instead of discarding it; update
the RUN block that references libqb-devel and the git clone /tmp/libqb-build
fallback to perform the availability check, branch on that result, and ensure
any non-package-not-found errors are surfaced (logged or cause failure) rather
than being ignored.
In `@helpers/resource-agents-build/Dockerfile.stream9`:
- Around line 12-14: Add a cleanup step to the package-install RUN commands in
Dockerfile.stream9 to reduce image size: after the dnf install lines (the RUN
commands that call "dnf install -y epel-release ..." and "dnf install -y git
autoconf ..."), append a cleanup command such as "dnf clean all && rm -rf
/var/cache/dnf /var/cache/yum" (or combine into a single RUN so cache isn't
persisted across layers) to remove metadata and caches.
- Around line 25-26: The Dockerfile's two RUN steps should explicitly fail with
a clear error if no RPM is found: replace the pipeline that runs find | xargs
-I{} cp {} /tmp/resource-agents.rpm with logic that captures the first match
into a shell variable (e.g., FILE or RA_RPM), test if that variable is empty,
and if so echo a descriptive error to stderr and exit 1; otherwise cp "$FILE"
/tmp/resource-agents.rpm and then run rpm -qip /tmp/resource-agents.rpm. Ensure
you reference the same target path /tmp/resource-agents.rpm so the subsequent
rpm inspection only runs when the file exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78ba7ecf-e4ec-498e-977b-3b774d0ef45b
📒 Files selected for processing (4)
helpers/README.mdhelpers/resource-agents-build/Dockerfile.stream10helpers/resource-agents-build/Dockerfile.stream9helpers/resource-agents-build/local-build-test.sh
Comment on lines
+28
to
+30
| RUN git clone --depth 1 "${RESOURCE_AGENTS_REPO}" /tmp/resource-agents-build \ | ||
| && git -C /tmp/resource-agents-build fetch origin "${RESOURCE_AGENTS_REF}" \ | ||
| && git -C /tmp/resource-agents-build checkout FETCH_HEAD |
There was a problem hiding this comment.
Same shallow clone concern as Stream 9.
This has the same issue as Dockerfile.stream9 - the shallow clone with --depth 1 may fail when fetching non-default refs. Consider applying the same fix here for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@helpers/resource-agents-build/Dockerfile.stream10` around lines 28 - 30, The
shallow clone uses "--depth 1" in the RUN command cloning
"${RESOURCE_AGENTS_REPO}" to /tmp/resource-agents-build which can fail when
checking out non-default refs; update the RUN line that contains git clone
--depth 1 "${RESOURCE_AGENTS_REPO}" /tmp/resource-agents-build so it does a full
clone (remove the "--depth 1" flag) or replace with an init/remote-add/fetch
sequence that fetches the specific "${RESOURCE_AGENTS_REF}" into
/tmp/resource-agents-build before running git -C /tmp/resource-agents-build
checkout FETCH_HEAD, ensuring RESOURCE_AGENTS_REPO and RESOURCE_AGENTS_REF are
used as in the existing commands.
Comment on lines
+17
to
+19
| RUN git clone --depth 1 "${RESOURCE_AGENTS_REPO}" /tmp/resource-agents-build \ | ||
| && git -C /tmp/resource-agents-build fetch origin "${RESOURCE_AGENTS_REF}" \ | ||
| && git -C /tmp/resource-agents-build checkout FETCH_HEAD |
There was a problem hiding this comment.
Shallow clone may fail for non-default branches or tags.
Using --depth 1 creates a shallow clone of only the default branch. The subsequent fetch origin "${RESOURCE_AGENTS_REF}" may fail if the ref is a tag, commit SHA, or branch that doesn't exist in the shallow history. Consider using --no-single-branch or removing --depth 1 for full ref flexibility.
Proposed fix
-RUN git clone --depth 1 "${RESOURCE_AGENTS_REPO}" /tmp/resource-agents-build \
+RUN git clone "${RESOURCE_AGENTS_REPO}" /tmp/resource-agents-build \
&& git -C /tmp/resource-agents-build fetch origin "${RESOURCE_AGENTS_REF}" \
&& git -C /tmp/resource-agents-build checkout FETCH_HEADOr alternatively, if you want to keep shallow clones for speed on the common case:
-RUN git clone --depth 1 "${RESOURCE_AGENTS_REPO}" /tmp/resource-agents-build \
- && git -C /tmp/resource-agents-build fetch origin "${RESOURCE_AGENTS_REF}" \
- && git -C /tmp/resource-agents-build checkout FETCH_HEAD
+RUN git clone --depth 1 --branch "${RESOURCE_AGENTS_REF}" "${RESOURCE_AGENTS_REPO}" /tmp/resource-agents-build \
+ || git clone "${RESOURCE_AGENTS_REPO}" /tmp/resource-agents-build && git -C /tmp/resource-agents-build checkout "${RESOURCE_AGENTS_REF}"📝 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
| RUN git clone --depth 1 "${RESOURCE_AGENTS_REPO}" /tmp/resource-agents-build \ | |
| && git -C /tmp/resource-agents-build fetch origin "${RESOURCE_AGENTS_REF}" \ | |
| && git -C /tmp/resource-agents-build checkout FETCH_HEAD | |
| RUN git clone "${RESOURCE_AGENTS_REPO}" /tmp/resource-agents-build \ | |
| && git -C /tmp/resource-agents-build fetch origin "${RESOURCE_AGENTS_REF}" \ | |
| && git -C /tmp/resource-agents-build checkout FETCH_HEAD |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@helpers/resource-agents-build/Dockerfile.stream9` around lines 17 - 19, The
shallow git clone using --depth 1 can prevent fetching non-default
branches/tags/SHAs; update the RUN git clone/fetch sequence so the clone
supports arbitrary refs referenced by RESOURCE_AGENTS_REF (e.g., remove --depth
1 or add --no-single-branch to the git clone, or after cloning run git fetch
--unshallow || git fetch --depth=1 origin "${RESOURCE_AGENTS_REF}" before git
checkout FETCH_HEAD) so that the subsequent git -C /tmp/resource-agents-build
fetch origin "${RESOURCE_AGENTS_REF}" and git -C /tmp/resource-agents-build
checkout FETCH_HEAD succeed reliably for tags, SHAs, and non-default branches.
Comment on lines
+29
to
+36
| while [[ $# -gt 0 ]]; do | ||
| case "$1" in | ||
| --repo) RESOURCE_AGENTS_REPO="$2"; shift 2 ;; | ||
| --ref) RESOURCE_AGENTS_REF="$2"; shift 2 ;; | ||
| -h|--help) usage ;; | ||
| *) echo "Unknown option: $1"; usage ;; | ||
| esac | ||
| done |
There was a problem hiding this comment.
Argument parsing lacks validation for missing option values.
If a user passes --repo without a following argument (e.g., ./local-build-test.sh --repo), shift 2 will fail or $2 will be unset, causing an error due to set -u. Additionally, calling usage on unknown options exits with code 0, which may be unexpected.
Proposed fix
while [[ $# -gt 0 ]]; do
case "$1" in
- --repo) RESOURCE_AGENTS_REPO="$2"; shift 2 ;;
- --ref) RESOURCE_AGENTS_REF="$2"; shift 2 ;;
+ --repo)
+ [[ $# -ge 2 ]] || { echo "Error: --repo requires an argument"; exit 1; }
+ RESOURCE_AGENTS_REPO="$2"; shift 2 ;;
+ --ref)
+ [[ $# -ge 2 ]] || { echo "Error: --ref requires an argument"; exit 1; }
+ RESOURCE_AGENTS_REF="$2"; shift 2 ;;
-h|--help) usage ;;
- *) echo "Unknown option: $1"; usage ;;
+ *) echo "Unknown option: $1" >&2; exit 1 ;;
esac
done📝 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
| while [[ $# -gt 0 ]]; do | |
| case "$1" in | |
| --repo) RESOURCE_AGENTS_REPO="$2"; shift 2 ;; | |
| --ref) RESOURCE_AGENTS_REF="$2"; shift 2 ;; | |
| -h|--help) usage ;; | |
| *) echo "Unknown option: $1"; usage ;; | |
| esac | |
| done | |
| while [[ $# -gt 0 ]]; do | |
| case "$1" in | |
| --repo) | |
| [[ $# -ge 2 ]] || { echo "Error: --repo requires an argument"; exit 1; } | |
| RESOURCE_AGENTS_REPO="$2"; shift 2 ;; | |
| --ref) | |
| [[ $# -ge 2 ]] || { echo "Error: --ref requires an argument"; exit 1; } | |
| RESOURCE_AGENTS_REF="$2"; shift 2 ;; | |
| -h|--help) usage ;; | |
| *) echo "Unknown option: $1" >&2; exit 1 ;; | |
| esac | |
| done |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@helpers/resource-agents-build/local-build-test.sh` around lines 29 - 36, The
argument parsing in the while/case block doesn't validate that an option value
exists for --repo and --ref and calls usage with a zero exit on unknown options;
update the case arms for --repo and --ref in the while loop to first check that
a next argument exists and is not another option (i.e., test that "$2" is set
and not starting with '-') and if missing print a clear error referencing
RESOURCE_AGENTS_REPO or RESOURCE_AGENTS_REF and exit non‑zero; also change the
default unknown option path (and any usage invocation for invalid input) to
print the error and call usage with a non‑zero exit code so failures are
signalled (reference the while loop, case pattern, RESOURCE_AGENTS_REPO,
RESOURCE_AGENTS_REF and usage).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
local-build-test.shvalidates that resource-agents compiles on CentOS Stream 9 and 10 locally via podman, with configurable repo/branchNote: I've moved and edited those scripts from openshift/release#75815 since I thought it could be more useful in this repo instead of
Release