Skip to content

NO-JIRA: chore(workflows): refactor cache handling logic and consolidate make targets in build workflows #2790

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

Merged
jiridanek merged 1 commit into from
Dec 19, 2025
Merged

NO-JIRA: chore(workflows): refactor cache handling logic and consolidate make targets in build workflows #2790

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
77 changes: 43 additions & 34 deletions .github/workflows/build-notebooks-TEMPLATE.yaml
View file
Open in desktop
Copy link
Member Author

@jiridanek jiridanek Dec 19, 2025

Choose a reason for hiding this comment

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

@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")

Copy link
Contributor

@coderabbitai coderabbitai bot Dec 19, 2025

Choose a reason for hiding this comment

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

🧩 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.

Original file line number Diff line number Diff line change
Expand Up @@ -192,50 +192,59 @@ jobs:
fi
echo "${{ secrets.AIPCC_QUAY_BOT_PASSWORD }}" | podman login quay.io/aipcc -u "${{ secrets.AIPCC_QUAY_BOT_USERNAME }}" --password-stdin

- name: Compute extra podman build args
id: extra-podman-build-args
run: |
set -Eeuxo pipefail

EXTRA_PODMAN_BUILD_ARGS=""
if [[ "${{ inputs.platform }}" == "linux/s390x" ]]; then
# 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'
fi
echo "EXTRA_PODMAN_BUILD_ARGS=$EXTRA_PODMAN_BUILD_ARGS" >> $GITHUB_OUTPUT

# https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#push
# https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request

- name: "push|schedule|workflow_dispatch: make ${{ inputs.target }}"
id: push-make-target
# We use `CONTAINER_BUILD_CACHE_ARGS` to smuggle in various misc `podman build` args, not just cache-related ones
- name: Compute CONTAINER_BUILD_CACHE_ARGS
id: extra-container-build-args
shell: python
# language=python
run: |
# print running stats on disk occupancy
(while true; do df -h | grep "${HOME}/.local/share/containers"; sleep 30; done) &

make ${{ inputs.target }}
if: ${{ fromJson(inputs.github).event_name == 'push' ||
fromJson(inputs.github).event_name == 'schedule' ||
fromJson(inputs.github).event_name == 'workflow_dispatch' }}
env:
IMAGE_TAG: "${{ steps.calculated_vars.outputs.IMAGE_TAG }}"
CONTAINER_BUILD_CACHE_ARGS: "${{ steps.extra-podman-build-args.outputs.EXTRA_PODMAN_BUILD_ARGS }} --cache-from ${{ env.CACHE }} --cache-to ${{ env.CACHE }}"
- name: "pull_request: make ${{ inputs.target }}"
id: pr-make-target
import os
import sys
from typing import Literal

extra_podman_build_args = ""
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'
Comment on lines +209 to +213
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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).

Copy link
Member Author

@jiridanek jiridanek Dec 19, 2025

Choose a reason for hiding this comment

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

no, this is not how --unsetenv in podman build works at all

Copy link
Contributor

@coderabbitai coderabbitai bot Dec 19, 2025

Choose a reason for hiding this comment

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

🧩 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=64 sets these variables during the build (making them available when compiling pyzmq through UV)
  • --unsetenv=CFLAGS --unsetenv=CXXFLAGS removes 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.


event_name: Literal['push', 'pull_request', 'pull_request_target', 'schedule', 'workflow_dispatch'] = "${{ fromJson(inputs.github).event_name }}"
cache = "${{ env.CACHE }}"

if event_name == "schedule":
# For schedule events, skip --cache-from to regenerate cache
cache_args = f"--cache-to {cache}"
print("Regenerating cache (schedule event)", file=sys.stderr)
elif event_name == "push" or event_name == "workflow_dispatch":
# push or workflow_dispatch: use and update cache
cache_args = f"--cache-from {cache} --cache-to {cache}"
print("Using and updating cache", file=sys.stderr)
elif event_name == "pull_request" or event_name == "pull_request_target":
# pull_request/pull_request_target: use cache, but we don't have write access, so we can't update even if we wanted to
cache_args = f"--cache-from {cache}"
print("Using cache", file=sys.stderr)
else:
raise ValueError(f"Unexpected event name: {event_name}")

with open(os.environ["GITHUB_OUTPUT"], "a") as f:
f.write(f"CONTAINER_BUILD_CACHE_ARGS={extra_podman_build_args} {cache_args}\n")

- name: "Build: make ${{ inputs.target }}"
id: make-target
run: |
# print running stats on disk occupancy
(while true; do df -h | grep "${HOME}/.local/share/containers"; sleep 30; done) &

make ${{ inputs.target }}
if: "${{ fromJson(inputs.github).event_name == 'pull_request' ||
fromJson(inputs.github).event_name == 'pull_request_target' }}"
env:
IMAGE_TAG: "${{ steps.calculated_vars.outputs.IMAGE_TAG }}"
CONTAINER_BUILD_CACHE_ARGS: "${{ steps.extra-podman-build-args.outputs.EXTRA_PODMAN_BUILD_ARGS }} --cache-from ${{ env.CACHE }}"
# We don't have access to image registry, so disable pushing
PUSH_IMAGES: "no"
CONTAINER_BUILD_CACHE_ARGS: "${{ steps.extra-container-build-args.outputs.CONTAINER_BUILD_CACHE_ARGS }}"
# We don't have access to image registry for PRs, so disable pushing
PUSH_IMAGES: "${{ (fromJson(inputs.github).event_name == 'pull_request' || fromJson(inputs.github).event_name == 'pull_request_target') && 'no' || 'yes' }}"

- name: "Show podman images information"
run: podman images --digests
Expand Down Expand Up @@ -429,7 +438,7 @@ jobs:
# we leave little free disk space after we mount LVM for podman storage
# not enough to install playwright; running playwright in podman uses the space we have
- name: Run Playwright tests
if: ${{ !cancelled() && contains(inputs.target, 'codeserver') && (steps.push-make-target.outcome == 'success' || steps.pr-make-target.outcome == 'success') }}
if: ${{ !cancelled() && contains(inputs.target, 'codeserver') && steps.make-target.outcome == 'success' }}
# --ipc=host because Microsoft says so in Playwright docs
# --net=host because testcontainers connects to the Reaper container's exposed port
# we need to pass through the relevant environment variables
Expand Down
Loading