NO-JIRA: chore(workflows): refactor cache handling logic and consolidate make targets in build workflows #2790
Conversation
|
Skipping CI for Draft Pull Request. |
WalkthroughA GitHub Actions workflow template replaces separate event-specific build steps with a Python step that computes CONTAINER_BUILD_CACHE_ARGS and compiler flags, consolidates push/pr make steps into a single Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
github-actions
bot
added
the
review-requested
jiridanek
force-pushed
the
jd_version_updates
branch
from
07da201 to
9b8fbe0
Compare
jiridanek
force-pushed
the
jd_version_updates
branch
from
9b8fbe0 to
0f2ac13
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ide-developer 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-notebooks-TEMPLATE.yaml(2 hunks)
🔇 Additional comments (3)
.github/workflows/build-notebooks-TEMPLATE.yaml (3)
236-247: LGTM! Clean consolidation of duplicate build steps.The unification of
push-make-targetandpr-make-targetinto a singlemake-targetstep successfully eliminates code duplication. The conditionalPUSH_IMAGESlogic correctly handles bothpull_requestandpull_request_targetevents, and the step properly references the cache args computed by the previous step.
441-441: LGTM! Correctly updated to reference the new step ID.The Playwright test conditional properly references the unified
make-targetstep outcome instead of the previous separate step IDs.
213-213: Verify CFLAGS/CXXFLAGS flag order in s390x workaroundThe code sets
CFLAGSandCXXFLAGSvia--envthen immediately unsets them via--unsetenv. While this order appears counterintuitive, it references a specific libzmq workaround (zeromq/libzmq#4486). Confirm whether the current order is intentional or if it should be reversed to--unsetenv=CFLAGS --unsetenv=CXXFLAGS --env=CFLAGS=-Dundefined=64 --env=CXXFLAGS=-Dundefined=64per typical precedence patterns.
…lds and deduplicate make targets in build workflows
jiridanek
force-pushed
the
jd_version_updates
branch
from
0f2ac13 to
60251f5
Compare
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-notebooks-TEMPLATE.yaml(2 hunks)
🔇 Additional comments (3)
.github/workflows/build-notebooks-TEMPLATE.yaml (3)
215-235: LGTM! Event handling correctly addresses all workflow triggers.The fix for
pull_request_target(from the previous review) is properly implemented, and the cache strategy logic is sound for all event types. The approach of using different cache arguments based on event type (schedule regenerates, push updates, PRs read-only) aligns well with typical CI/CD cache patterns.
236-247: LGTM! Effective consolidation reduces duplication.The unified
make-targetstep successfully merges the previous event-specific steps, and the conditionalPUSH_IMAGESlogic correctly prevents pushing for PR events while allowing it for push/workflow_dispatch/schedule.
441-441: LGTM! Correctly references the consolidated step.The condition properly uses
steps.make-target.outcomefollowing the consolidation of the build steps.
| if "${{ inputs.platform }}" == "linux/s390x": | ||
| # workaround for known issue https://github.com/zeromq/libzmq/pull/4486 | ||
| # In qemu-user, CACHELINE_SIZE probe is undefined | ||
| # Compiling pyzmq through UV needs this in CFLAGS | ||
| extra_podman_build_args += '--env=CFLAGS=-Dundefined=64 --env=CXXFLAGS=-Dundefined=64 --unsetenv=CFLAGS --unsetenv=CXXFLAGS' |
There was a problem hiding this comment.
Critical: --unsetenv order will break the s390x workaround.
The current order sets CFLAGS/CXXFLAGS then immediately unsets them, leaving them undefined. Since the comment states "Compiling pyzmq through UV needs this in CFLAGS", this defeats the intended workaround for the CACHELINE_SIZE probe issue.
🔎 Proposed fix
Either remove the --unsetenv flags entirely (if overwriting is sufficient):
- extra_podman_build_args += '--env=CFLAGS=-Dundefined=64 --env=CXXFLAGS=-Dundefined=64 --unsetenv=CFLAGS --unsetenv=CXXFLAGS'
+ extra_podman_build_args = '--env=CFLAGS=-Dundefined=64 --env=CXXFLAGS=-Dundefined=64'Or reverse the order to unset first, then set:
- extra_podman_build_args += '--env=CFLAGS=-Dundefined=64 --env=CXXFLAGS=-Dundefined=64 --unsetenv=CFLAGS --unsetenv=CXXFLAGS'
+ extra_podman_build_args = '--unsetenv=CFLAGS --unsetenv=CXXFLAGS --env=CFLAGS=-Dundefined=64 --env=CXXFLAGS=-Dundefined=64'Note: Changed += to = since this is the only assignment to extra_podman_build_args.
📝 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.
| if "${{ inputs.platform }}" == "linux/s390x": | |
| # workaround for known issue https://github.com/zeromq/libzmq/pull/4486 | |
| # In qemu-user, CACHELINE_SIZE probe is undefined | |
| # Compiling pyzmq through UV needs this in CFLAGS | |
| extra_podman_build_args += '--env=CFLAGS=-Dundefined=64 --env=CXXFLAGS=-Dundefined=64 --unsetenv=CFLAGS --unsetenv=CXXFLAGS' | |
| if "${{ inputs.platform }}" == "linux/s390x": | |
| # workaround for known issue https://github.com/zeromq/libzmq/pull/4486 | |
| # In qemu-user, CACHELINE_SIZE probe is undefined | |
| # Compiling pyzmq through UV needs this in CFLAGS | |
| extra_podman_build_args = '--unsetenv=CFLAGS --unsetenv=CXXFLAGS --env=CFLAGS=-Dundefined=64 --env=CXXFLAGS=-Dundefined=64' |
🤖 Prompt for AI Agents
.github/workflows/build-notebooks-TEMPLATE.yaml around lines 209 to 213: the
current build-arg string appends CFLAGS/CXXFLAGS then immediately unsets them,
nullifying the s390x workaround; fix by (a) either removing the --unsetenv flags
entirely so the exported CFLAGS/CXXFLAGS remain set for the build, or (b) if
unset is required beforehand, construct the podman args so --unsetenv=CFLAGS
--unsetenv=CXXFLAGS appear before the --env=CFLAGS=... --env=CXXFLAGS=...
assignments (and use = instead of += if this is the only assignment to
extra_podman_build_args).
There was a problem hiding this comment.
no, this is not how --unsetenv in podman build works at all
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
podman build --unsetenv documentation
💡 Result:
The --unsetenv option removes an environment variable from the final image created by podman build. Use it as --unsetenv=VAR (repeatable to remove multiple variables). Example:
podman build --unsetenv HTTP_PROXY --unsetenv http_proxy -t myimage:latest .
Sources: Podman build documentation. [1] [2]
@jiridanek, you're absolutely right, and I apologize for the incorrect analysis.
I misunderstood how --unsetenv works in podman build. Based on the documentation, --unsetenv removes environment variables from the final image, not during the build process.
The sequence in your code is correct:
--env=CFLAGS=-Dundefined=64 --env=CXXFLAGS=-Dundefined=64sets these variables during the build (making them available when compiling pyzmq through UV)--unsetenv=CFLAGS --unsetenv=CXXFLAGSremoves them from the final image (cleanup, so they don't persist in the runtime environment)
This is the proper way to apply a build-time workaround without polluting the final image's environment. Thank you for the correction!
✏️ Learnings added
Learnt from: jiridanek
Repo: opendatahub-io/notebooks PR: 2790
File: .github/workflows/build-notebooks-TEMPLATE.yaml:209-213
Timestamp: 2025-12-19T15:54:11.014Z
Learning: In podman build, the --unsetenv flag removes environment variables from the final image, not during the build process. The pattern `--env=VAR=value ... --unsetenv=VAR` correctly sets variables during build (for compilation) while removing them from the final image (cleanup).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@coderabbitai create a github issue for this problem; my suggestion there is to store the command options in a python list, that way we avoid problems with spaces when concatting; using shlex.quote can also help
🧹 Nitpick comments (1)
.github/workflows/build-notebooks-TEMPLATE.yaml (1)
198-234: Python script uses type annotation but the type is enforced only at static analysis time.
The Literal type annotation on line 215 provides documentation but won't catch unexpected event names at runtime since Python doesn't enforce type hints. The else branch with ValueError handles this correctly. However, consider a minor improvement for robustness.
Also, on line 234, there's a potential issue: if extra_podman_build_args is empty, you'll have a leading space in CONTAINER_BUILD_CACHE_ARGS. This is unlikely to cause problems with shell parsing but is worth noting.
🔎 Optional: Avoid leading space when extra_podman_build_args is empty
-
with open(os.environ["GITHUB_OUTPUT"], "a") as f: -
f.write(f"CONTAINER_BUILD_CACHE_ARGS={extra_podman_build_args} {cache_args}\n")
-
with open(os.environ["GITHUB_OUTPUT"], "a") as f: -
args = f"{extra_podman_build_args} {cache_args}".strip() -
f.write(f"CONTAINER_BUILD_CACHE_ARGS={args}\n")
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the leading space problem in the workflow Python script
gh issue create \
--title "Improve command argument handling in build workflow Python script" \
--body "## Description
The Python script in \`.github/workflows/build-notebooks-TEMPLATE.yaml\` (lines 198-234) that computes \`CONTAINER_BUILD_CACHE_ARGS\` has a potential issue with string concatenation that can lead to a leading space in the output when \`extra_podman_build_args\` is empty.
## Problem
The current implementation concatenates strings directly:
\`\`\`
CONTAINER_BUILD_CACHE_ARGS={extra_podman_build_args} {cache_args}
\`\`\`
This can result in a leading space when \`extra_podman_build_args\` is empty, which while unlikely to cause problems with shell parsing, is not ideal.
## Suggested Approach
To improve robustness and avoid issues with spaces:
1. Store command options in a Python list instead of concatenating strings
2. Use \`shlex.quote\` to properly escape arguments when needed
3. Join the list elements appropriately to avoid leading/trailing spaces
## Context
- PR: https://github.com/opendatahub-io/notebooks/pull/2790
- Review comment: https://github.com/opendatahub-io/notebooks/pull/2790#discussion_r2635536051
- Requested by: @jiridanek" \
--assignee jiridanek
Length of output: 55
@jiridanek, I've created issue #2792 to track this improvement. The issue describes the leading space problem and includes your suggestion to use a Python list for command options along with shlex.quote for proper escaping. This approach will make the argument handling more robust and avoid potential issues with spaces in the concatenation.
2 participants
Description
Successfully unified the two make target steps into a single step in
.github/workflows/build-notebooks-TEMPLATE.yaml:Changes made:
push-make-targetandpr-make-targetsteps into unifiedmake-targetstepPUSH_IMAGESenvironment variable: sets to "no" for pull request events, "yes" otherwisemake-targetstep IDBenefits:
How Has This Been Tested?
Self checklist (all need to be checked):
make test(gmakeon macOS) before asking for reviewDockerfile.konfluxfiles should be done inodh/notebooksand automatically synced torhds/notebooks. For Konflux-specific changes, modifyDockerfile.konfluxfiles directly inrhds/notebooksas these require special attention in the downstream repository and flow to the upcoming RHOAI release.Merge criteria:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.