[rhoai-2.25] docs(lockfile): align custom flavor on Dockerfile.konflux.<flavor> - #2705
[rhoai-2.25] docs(lockfile): align custom flavor on Dockerfile.konflux.<flavor>#2705jiridanek wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
CI status [antigravity]Run: Build Notebooks (push) #30939029537 — 2/2 complete · 2 skipped Workflow completed with skipped jobs. |
📝 WalkthroughWalkthroughThe lockfile generators now resolve ChangesKonflux flavor resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
CI status [antigravity]Run: Build Notebooks (pr) #30939043570 — 2/2 complete · 1 passed · 1 skipped No workbench image jobs ran; all matrix jobs were skipped. |
📋 Review SummaryThe pull request successfully updates script documentation, default flavor handling, and Konflux Dockerfile validation ( 🔍 General Feedback
I did not find any issues requiring code modifications or inline comments. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/lockfile-generators/create-requirements-lockfile.sh`:
- Around line 114-115: Update the flavor resolution before constructing
KONFLUX_DOCKERFILE so projects without Dockerfile.konflux.cpu can select an
available non-CPU flavor (such as CUDA or ROCm), while still honoring an
explicitly provided --flavor; alternatively require --flavor when no suitable
default exists. Ensure the availability check uses the resolved flavor rather
than blindly assuming cpu.
In `@scripts/lockfile-generators/README.md`:
- Line 853: Update the --flavor option description in the README to state that
build-args/konflux.<flavor>.conf is required only for the rh-index flow, while
retaining the Dockerfile flavor matching and output filename details for all
flavors.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f1529506-c171-49bf-b99f-43a3ac1034f7
📒 Files selected for processing (2)
scripts/lockfile-generators/README.mdscripts/lockfile-generators/create-requirements-lockfile.sh
When --flavor is omitted and Dockerfile.konflux.cpu is absent, resolve
the flavor from the first available Dockerfile.konflux.{cpu,cuda,rocm}
in create-requirements-lockfile.sh and prefetch-all.sh.
Also scope konflux.conf requirement to rh-index in docs and --help.
Co-authored-by: Cursor <cursoragent@cursor.com>
CI status [antigravity]Run: Build Notebooks (push) #30943127627 — 2/2 complete · 2 skipped Workflow completed with skipped jobs. |
CI status [antigravity]Run: Build Notebooks (pr) #30943130271 — 2/2 complete · 1 passed · 1 skipped No workbench image jobs ran; all matrix jobs were skipped. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/lockfile-generators/prefetch-all.sh`:
- Around line 64-65: Update the --flavor option help text in prefetch-all.sh to
state that an explicitly supplied flavor must exactly match an available
Dockerfile.konflux.<flavor>; retain the existing documentation for the cpu
default and fallback behavior.
- Line 163: Update find_tekton_yaml to select the Tekton YAML whose Dockerfile
declaration matches Dockerfile.konflux.${FLAVOR} exactly, rather than relying on
head -1. Pass the resolved FLAVOR from the surrounding prefetch flow so CPU,
CUDA, and ROCm configurations resolve their corresponding YAML and inputs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3914949f-0dda-49a8-b710-68b8f6636935
📒 Files selected for processing (3)
scripts/lockfile-generators/README.mdscripts/lockfile-generators/create-requirements-lockfile.shscripts/lockfile-generators/prefetch-all.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/lockfile-generators/README.md
| --flavor NAME Lock file flavor (default: cpu, or first available | ||
| Dockerfile.konflux.{cpu,cuda,rocm} when cpu is absent) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document explicit flavor validation in prefetch-all.sh --help.
The resolver rejects an explicitly supplied flavor when Dockerfile.konflux.<flavor> is missing. The help text only documents the default fallback. Add the exact-match requirement so users can predict validation behavior.
Proposed help-text update
--flavor NAME Lock file flavor (default: cpu, or first available
Dockerfile.konflux.{cpu,cuda,rocm} when cpu is absent)
+ Must match Dockerfile.konflux.<flavor> in the
+ component directory.📝 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.
| --flavor NAME Lock file flavor (default: cpu, or first available | |
| Dockerfile.konflux.{cpu,cuda,rocm} when cpu is absent) | |
| --flavor NAME Lock file flavor (default: cpu, or first available | |
| Dockerfile.konflux.{cpu,cuda,rocm} when cpu is absent) | |
| Must match Dockerfile.konflux.<flavor> in the | |
| component directory. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lockfile-generators/prefetch-all.sh` around lines 64 - 65, Update the
--flavor option help text in prefetch-all.sh to state that an explicitly
supplied flavor must exactly match an available Dockerfile.konflux.<flavor>;
retain the existing documentation for the cpu default and fallback behavior.
|
|
||
| [[ -z "$COMPONENT_DIR" ]] && error_exit "--component-dir is required." | ||
| [[ -d "$COMPONENT_DIR" ]] || error_exit "Component directory not found: $COMPONENT_DIR" | ||
| FLAVOR="$(resolve_konflux_flavor "$COMPONENT_DIR" "$FLAVOR" "$FLAVOR_EXPLICIT")" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -u
rg -n -C 10 \
'find_tekton_yaml|TEKTON_YAML|FLAVOR=|dockerfile_path' \
scripts/lockfile-generators/prefetch-all.sh
printf '\nFlavor-specific Tekton references:\n'
rg -n -C 4 \
'Dockerfile\.konflux\.(cpu|cuda|rocm)|Dockerfile\.konflux\.' \
.tekton scripts --glob '*.yaml' --glob '*.sh'Repository: red-hat-data-services/notebooks
Length of output: 50387
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' 'Relevant implementation:'
sed -n '74,140p;292,325p' scripts/lockfile-generators/prefetch-all.sh
printf '\n%s\n' 'NPM downloader input handling:'
fd -i 'download-npm' scripts/lockfile-generators
sed -n '1,220p' scripts/lockfile-generators/download-npm.sh
printf '\n%s\n' 'Pull-request Tekton files grouped by component directory and flavor:'
python3 - <<'PY'
from pathlib import Path
import re
from collections import defaultdict
groups = defaultdict(list)
for path in sorted(Path(".tekton").glob("*pull-request*.yaml")):
text = path.read_text()
m = re.search(
r'(?m)^\s*value:\s*["\']?([^"\']*?/Dockerfile\.konflux\.([A-Za-z0-9_-]+))["\']?\s*$',
text,
)
if not m:
continue
dockerfile, flavor = m.groups()
component = dockerfile.rsplit("/Dockerfile.konflux.", 1)[0]
groups[component].append((flavor, str(path), dockerfile))
for component, entries in groups.items():
flavors = sorted({flavor for flavor, _, _ in entries})
if len(flavors) > 1:
print(component)
for flavor, path, dockerfile in entries:
print(f" {flavor}: {path} -> {dockerfile}")
PYRepository: red-hat-data-services/notebooks
Length of output: 12884
🏁 Script executed:
#!/usr/bin/env bash
set -u
python3 - <<'PY'
from pathlib import Path
from collections import defaultdict
import re
# Extract the scalar immediately following a dockerfile parameter.
records = []
for path in sorted(Path(".tekton").glob("*pull-request*.yaml")):
lines = path.read_text().splitlines()
for i, line in enumerate(lines):
if re.match(r"^\s*-\s*name:\s*dockerfile\s*$", line):
for value_line in lines[i + 1:i + 5]:
m = re.match(r"^\s*value:\s*[\"']?([^\"']+?)[\"']?\s*$", value_line)
if m:
dockerfile = m.group(1).strip()
if "/Dockerfile.konflux." in dockerfile:
component, flavor = dockerfile.rsplit("/Dockerfile.konflux.", 1)
records.append((component, flavor, str(path), dockerfile))
break
break
by_component = defaultdict(list)
for record in records:
by_component[record[0]].append(record)
print(f"pull-request YAMLs with Konflux dockerfiles: {len(records)}")
print(f"components represented: {len(by_component)}")
print("\nComponents with multiple matching pull-request YAMLs:")
collision_count = 0
for component in sorted(by_component):
entries = by_component[component]
if len(entries) > 1:
collision_count += 1
print(component)
for _, flavor, path, dockerfile in entries:
print(f" {flavor}\t{path}\t{dockerfile}")
if collision_count == 0:
print(" none")
print("\nFlavor distribution:")
flavors = defaultdict(int)
for _, flavor, _, _ in records:
flavors[flavor] += 1
for flavor, count in sorted(flavors.items()):
print(f" {flavor}: {count}")
print("\nFirst matching YAML for each component under lexical glob order:")
for component in sorted(by_component):
first = by_component[component][0]
print(f"{component}\t{first[1]}\t{first[2]}")
PY
printf '\nDockerfiles with multiple Konflux flavors:\n'
find . -type f -name 'Dockerfile.konflux.*' -print \
| sed 's#^\./##' \
| awk '
{
path=$0
sub(/\/Dockerfile\.konflux\.[^/]+$/, "", path)
count[path]++
files[path]=files[path] "\n " $0
}
END {
for (path in count)
if (count[path] > 1)
print path ":" files[path]
}
' | sortRepository: red-hat-data-services/notebooks
Length of output: 2887
🏁 Script executed:
#!/usr/bin/env bash
set -u
for f in \
.tekton/odh-workbench-jupyter-minimal-cpu-py312-pull-request.yaml \
.tekton/odh-workbench-jupyter-minimal-cuda-py312-pull-request.yaml \
.tekton/odh-workbench-jupyter-minimal-rocm-py312-pull-request.yaml
do
printf '\n--- %s ---\n' "$f"
rg -n -C 3 'name: dockerfile|name: prefetch-input|type: npm|path:|build-args-file' "$f"
done
printf '\nFlavor-specific Dockerfiles and package-lock locations:\n'
for f in \
jupyter/minimal/ubi9-python-3.12/Dockerfile.konflux.cpu \
jupyter/minimal/ubi9-python-3.12/Dockerfile.konflux.cuda \
jupyter/minimal/ubi9-python-3.12/Dockerfile.konflux.rocm
do
printf '\n--- %s ---\n' "$f"
rg -n '^(FROM|COPY|ADD)|package-lock|package\.json|prefetch-input|npm' "$f" || true
done
printf '\nNPM prefetch paths by YAML (textual YAML extraction):\n'
python3 - <<'PY'
from pathlib import Path
import re
files = [
Path(".tekton/odh-workbench-jupyter-minimal-cpu-py312-pull-request.yaml"),
Path(".tekton/odh-workbench-jupyter-minimal-cuda-py312-pull-request.yaml"),
Path(".tekton/odh-workbench-jupyter-minimal-rocm-py312-pull-request.yaml"),
]
for path in files:
lines = path.read_text().splitlines()
dockerfile = next(
(re.match(r"\s*value:\s*[\"']?([^\"']+?)[\"']?\s*$", lines[i + 1]).group(1)
for i, line in enumerate(lines[:-1])
if re.match(r"\s*-\s*name:\s*dockerfile\s*$", line)
and re.match(r"\s*value:", lines[i + 1])),
"<not found>",
)
npm_paths = []
in_npm = False
for i, line in enumerate(lines):
if re.match(r"\s*-\s*type:\s*npm\s*$", line):
in_npm = True
elif in_npm:
m = re.match(r"\s*path:\s*(\S+)\s*$", line)
if m:
npm_paths.append(m.group(1))
in_npm = False
elif re.match(r"\s*-\s*type:", line):
in_npm = False
print(path)
print(f" dockerfile: {dockerfile}")
print(f" npm paths: {npm_paths}")
PYRepository: red-hat-data-services/notebooks
Length of output: 3249
Make find_tekton_yaml flavor-aware.
jupyter/minimal/ubi9-python-3.12 has CPU, CUDA, and ROCm pull-request YAML files. head -1 selects the CPU file for every resolved flavor. The current files have no npm inputs, but this can select incorrect npm inputs when flavor-specific inputs exist. Match Dockerfile.konflux.${FLAVOR} exactly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lockfile-generators/prefetch-all.sh` at line 163, Update
find_tekton_yaml to select the Tekton YAML whose Dockerfile declaration matches
Dockerfile.konflux.${FLAVOR} exactly, rather than relying on head -1. Pass the
resolved FLAVOR from the surrounding prefetch flow so CPU, CUDA, and ROCm
configurations resolve their corresponding YAML and inputs.
1 participant
Summary
--flavordocs and validation onDockerfile.konflux.<flavor>after [rhoai-2.25] chore: remove orphan Dockerfiles and PDF install scripts #2694 removed legacyDockerfile.<flavor>workbench files.Dockerfile.konflux.${FLAVOR}increate-requirements-lockfile.shbefore lock generation.build-args/konflux.cuda.conf, jupyter/minimal).Fixes #2700
Test plan
bash -n scripts/lockfile-generators/create-requirements-lockfile.sh./scripts/lockfile-generators/create-requirements-lockfile.sh --helpshowsDockerfile.konflux.<flavor>--flavorexits with clear Dockerfile-not-found errorMade with Cursor
Summary by CodeRabbit
Documentation
Bug Fixes