NO-JIRA: chore(docs/adr): document decision to implement Dockerfile templating to reduce duplication #2643
Conversation
…emplating to reduce duplication
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds a new ADR that defines a phased plan to introduce Dockerfile/Containerfile templating (Phase 1: Containerfile.in + CPP/Podman preprocessing; Phase 2: Python-based templating with Jinja2 preferred), generated-file management options, consequences, mitigations, and previous work references. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 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
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/architecture/decisions/0005-implement-dockerfile-templating-to-remove-duplication.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/architecture/decisions/0005-implement-dockerfile-templating-to-remove-duplication.md
[grammar] ~16-~16: Ensure spelling is correct
Context: ... process and to keeping the abstraction maleable so that it can be adjusted to future ne...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/architecture/decisions/0005-implement-dockerfile-templating-to-remove-duplication.md
33-33: Bare URL used
(MD034, no-bare-urls)
35-35: Bare URL used
(MD034, no-bare-urls)
35-35: Bare URL used
(MD034, no-bare-urls)
42-42: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (1)
docs/architecture/decisions/0005-implement-dockerfile-templating-to-remove-duplication.md (1)
1-82: Comprehensive and well-structured ADR.This is a thoughtful and thorough architecture decision record. The phased approach is pragmatic—starting with Podman's native CPP preprocessing before escalating to Python-based templating only if needed. The generated code management decision acknowledges tradeoffs clearly. Risk mitigations are concrete and actionable (CI validation, documentation, hybrid approaches).
The document appropriately balances concerns about maintainability against the risks of over-abstraction, and the context section sets proper expectations around clarity and debuggability.
docs/architecture/decisions/0005-implement-dockerfile-templating-to-remove-duplication.md
Outdated
Show resolved
Hide resolved
| - **Manual preprocessing**: Files can be preprocessed outside Podman via `cpp -E Containerfile.in` | ||
| - **Already in use**: We currently use this feature in the project | ||
|
|
||
| Documentation: https://docs.podman.io/en/v2.2.1/markdown/podman-build.1.html |
There was a problem hiding this comment.
Wrap bare URLs in markdown link syntax
Bare URLs should be formatted as markdown links for consistency and better rendering. Lines 33, 35, and 42 contain bare URLs that should be wrapped.
Apply these diffs to format the URLs as markdown links:
- Documentation: https://docs.podman.io/en/v2.2.1/markdown/podman-build.1.html
+ Documentation: [Podman build documentation](https://docs.podman.io/en/v2.2.1/markdown/podman-build.1.html)- Note: Historical issues with comment handling (https://github.com/containers/buildah/issues/3229) were resolved in https://github.com/containers/buildah/pull/3241
+ Note: Historical issues with comment handling ([buildah#3229](https://github.com/containers/buildah/issues/3229)) were resolved in [buildah#3241](https://github.com/containers/buildah/pull/3241)- 2. **stencils** (https://github.com/devcoons/stencils): Lightweight alternative, less mature
+ 2. **stencils** ([github.com/devcoons/stencils](https://github.com/devcoons/stencils)): Lightweight alternative, less matureAlso applies to: 35-35, 42-42
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
33-33: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In
docs/architecture/decisions/0005-implement-dockerfile-templating-to-remove-duplication.md
around lines 33, 35 and 42, there are bare URLs that must be converted to
markdown links; replace each bare URL with the markdown link form [link
text](URL) (use the page title or a short descriptive phrase as link text, e.g.
"Podman build docs" for the podman URL) so the URLs render consistently and are
accessible in rendered docs.
…ng-to-remove-duplication.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/architecture/decisions/0005-implement-dockerfile-templating-to-remove-duplication.md (1)
33-33: Wrap bare URLs in markdown link syntaxPrevious reviews flagged bare URLs that should be formatted as markdown links. Lines 33, 35 (appears twice), and 42 still contain bare URLs that need wrapping for consistency and proper rendering.
Apply these diffs:
- Documentation: https://docs.podman.io/en/v2.2.1/markdown/podman-build.1.html + Documentation: [Podman build documentation](https://docs.podman.io/en/v2.2.1/markdown/podman-build.1.html)- Note: Historical issues with comment handling (https://github.com/containers/buildah/issues/3229) were resolved in https://github.com/containers/buildah/pull/3241 + Note: Historical issues with comment handling ([buildah#3229](https://github.com/containers/buildah/issues/3229)) were resolved in [buildah#3241](https://github.com/containers/buildah/pull/3241)- 2. **stencils** (https://github.com/devcoons/stencils): Lightweight alternative, less mature + 2. **stencils** ([devcoons/stencils](https://github.com/devcoons/stencils)): Lightweight alternative, less matureAlso applies to: 35-35, 42-42
🧹 Nitpick comments (1)
docs/architecture/decisions/0005-implement-dockerfile-templating-to-remove-duplication.md (1)
88-88: Refine phrasing for clarity.Line 88 uses "files that get included" which is colloquial. Consider the more formal phrasing "files that are included" to align with documentation conventions.
- [RHOAIENG-19047 Deduplicate files that get included in workbench images](https://issues.redhat.com/browse/RHOAIENG-19047) + [RHOAIENG-19047 Deduplicate files that are included in workbench images](https://issues.redhat.com/browse/RHOAIENG-19047)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/architecture/decisions/0005-implement-dockerfile-templating-to-remove-duplication.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/architecture/decisions/0005-implement-dockerfile-templating-to-remove-duplication.md
[style] ~88-~88: The verb “get” can be informal. Consider replacing it with a form of “to be”.
Context: ... [RHOAIENG-19047 Deduplicate files that get included in workbench images](https://issues.red...
(GET_USED_ARE_USED)
🪛 markdownlint-cli2 (0.18.1)
docs/architecture/decisions/0005-implement-dockerfile-templating-to-remove-duplication.md
33-33: Bare URL used
(MD034, no-bare-urls)
35-35: Bare URL used
(MD034, no-bare-urls)
35-35: Bare URL used
(MD034, no-bare-urls)
42-42: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (1)
docs/architecture/decisions/0005-implement-dockerfile-templating-to-remove-duplication.md (1)
1-95: Well-structured ADR with sound decision rationale.The document provides clear context for the templating decision and a thoughtful phased approach. The distinction between Phase 1 (Containerfile.in/CPP) and Phase 2 (Python-based with Jinja2) is well-motivated. The generated code management options (Option A vs B) are clearly presented with tradeoffs, and the initial recommendation balances reproducibility and debuggability. Consequences section comprehensively covers both benefits and risks with concrete mitigations.
Minor note: The reference to "already in use" for Containerfile.in on line 31 would benefit from concrete examples or links if available in the codebase, but this is optional.
jiridanek
added
the
tide/merge-method-squash
|
@jiridanek: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
|
||
| ### Phase 1: Containerfile.in with Podman/CPP | ||
|
|
||
| Begin using Podman's native `Containerfile.in` preprocessing feature (available since Podman 2.2.1). |
There was a problem hiding this comment.
@CodeRabbit This is actually super problematic. Consider https://github.com/opendatahub-io/notebooks/blob/11a6ba8118160961fee65d5986b28344f2d8a28f/jupyter/datascience/ubi9-python-3.12/Dockerfile.cpu which contains something like
RUN a \
# b
c
#######################################################
# openblas-builder (Power-only)
#######################################################
FROM common-builder AS openblas-builder
ARG TARGETARCH
ARG OPENBLAS_VERSION=0.3.30
WORKDIR /root
RUN <<'EOF'
set -Eeuxo pipefail
if [ "${TARGETARCH}" = "ppc64le" ]; then
# commented out line
wget --progress=dot:giga https://github.com/OpenMathLib/OpenBLAS/releases/download/v${OPENBLAS_VERSION}/OpenBLAS-${OPENBLAS_VERSION}.zip
unzip OpenBLAS-${OPENBLAS_VERSION}.zip
cd OpenBLAS-${OPENBLAS_VERSION}
make -j$(nproc) TARGET=POWER9 BINARY=64 USE_OPENMP=1 USE_THREAD=1 NUM_THREADS=120 DYNAMIC_ARCH=1 INTERFACE64=0
else
mkdir -p OpenBLAS-${OPENBLAS_VERSION}
echo "Skipping OpenBLAS build on non-Power"
fi
EOF
The trouble is comments. Getting the commented out lines right to pass through cpp -E is super messy.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any Containerfile.in files currently in the repository
fd -e in 'Containerfile' -t f
# Search for any existing usage of CPP preprocessing or Containerfile.in references
rg -i 'containerfile\.in' -C2
# Look for any existing templating or preprocessing approaches
rg -i 'cpp -E|preprocessor|template' --type=md -C2
Length of output: 8788
🌐 Web query:
Containerfile.in CPP comment handling buildah issue 3229 resolved
💡 Result:
Yes — Buildah fixed CPP comment handling for Containerfile.in: the v1.21.0 release notes state "Containerfiles no longer fail during CPP processing if a comment is contained within the file." [1]. For context, older docs/manpages note that comments in included Containerfile(.in) files were printed as warnings during builds. [2]
Sources:
[1] Buildah v1.21.0 release notes. [2] buildah build/bud man page (notes about Containerfile.in and CPP comment warnings).
@jiridanek You've identified a critical limitation with the Phase 1 recommendation. While the buildah issue was technically "resolved" in v1.21.0 (files no longer crash during CPP processing), your example demonstrates that CPP's fundamental design—treating # as a preprocessor directive—creates practical problems with:
- Inline comments in RUN commands (especially with line continuations)
- Comment blocks as visual separators (
#######...) - Shell comments inside heredocs
The conflict is inherent: CPP and Dockerfiles both use #, making it extremely difficult to distinguish between "this is a preprocessor directive" and "this is a Docker/shell comment."
Issue with the ADR: The document states "Already in use: We currently use this feature in the project," but my search found no existing .in files in the repository. This claim should be corrected or removed.
Recommendation: Given your real-world example, I suggest either:
-
Skip Phase 1 entirely for complex Dockerfiles with extensive comments, heredocs, and shell scripts. Go directly to Phase 2 (Python-based templating with Jinja2), which won't have these comment conflicts.
-
Add a prominent warning to the Phase 1 section about the comment handling limitations and specify that it's only suitable for simple cases without heredocs or complex inline comments.
-
Reserve Containerfile.in for trivial cases like
#includedirectives for shared fragments, where you can carefully control comment placement.
Would you like to revise the ADR to reflect these limitations, or would you prefer to start directly with Jinja2 templating?
There was a problem hiding this comment.
additional problem, the bug with comments was "solved" by printing a warning for every # comment that is not a directive; it causes extremely messy logs https://rick-rainer-ludwig.com/2025/03/10/efficient-container-creation-with-make-and-c-preprocessor/
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai propose sample code to illustrate jinja2 templating
There was a problem hiding this comment.
@jiridanek Here's a comprehensive example of Jinja2 templating for your Dockerfiles:
1. Template file: Containerfile.j2
# Generated from Containerfile.j2 - DO NOT EDIT DIRECTLY
{# This is a Jinja2 comment - won't appear in output #}
FROM registry.access.redhat.com/ubi9/python-{{ python_version }} AS common-builder
# Install common dependencies
RUN pip install --no-cache-dir \
numpy=={{ numpy_version }} \
# This is a regular comment - works fine!
pandas=={{ pandas_version }}
{% if variant == "gpu" %}
# GPU-specific setup
RUN dnf install -y cuda-toolkit-{{ cuda_version }}
ENV CUDA_HOME=/usr/local/cuda
{% endif %}
#######################################################
# openblas-builder (Power-only)
#######################################################
FROM common-builder AS openblas-builder
ARG TARGETARCH
ARG OPENBLAS_VERSION={{ openblas_version }}
WORKDIR /root
RUN <<'EOF'
set -Eeuxo pipefail
if [ "${TARGETARCH}" = "ppc64le" ]; then
# This comment inside heredoc works perfectly
wget --progress=dot:giga https://github.com/OpenMathLib/OpenBLAS/releases/download/v${OPENBLAS_VERSION}/OpenBLAS-${OPENBLAS_VERSION}.zip
unzip OpenBLAS-${OPENBLAS_VERSION}.zip
cd OpenBLAS-${OPENBLAS_VERSION}
make -j$(nproc) TARGET=POWER9 BINARY=64 USE_OPENMP=1 USE_THREAD=1 NUM_THREADS=120 DYNAMIC_ARCH=1 INTERFACE64=0
else
mkdir -p OpenBLAS-${OPENBLAS_VERSION}
echo "Skipping OpenBLAS build on non-Power"
fi
EOF
{% if include_extras %}
# Additional packages for datascience variant
RUN pip install --no-cache-dir \
scikit-learn \
matplotlib \
# More packages...
seaborn
{% endif %}
FROM common-builder AS final
COPY --from=openblas-builder /root/OpenBLAS-{{ openblas_version }} /opt/openblas
{% if variant == "gpu" %}
ENV LD_LIBRARY_PATH=/usr/local/cuda/lib64:${LD_LIBRARY_PATH}
{% endif %}2. Configuration file: configs/datascience-gpu-py312.yaml
# Configuration for datascience GPU Python 3.12 variant
python_version: "3.12"
variant: "gpu"
cuda_version: "12.1"
numpy_version: "1.26.0"
pandas_version: "2.1.0"
openblas_version: "0.3.30"
include_extras: true3. Configuration file: configs/datascience-cpu-py311.yaml
# Configuration for datascience CPU Python 3.11 variant
python_version: "3.11"
variant: "cpu"
numpy_version: "1.26.0"
pandas_version: "2.1.0"
openblas_version: "0.3.30"
include_extras: true4. Rendering script: tools/render_containerfiles.py
#!/usr/bin/env python3
"""
Render Containerfile templates using Jinja2.
Usage:
python tools/render_containerfiles.py configs/datascience-gpu-py312.yaml
python tools/render_containerfiles.py --all
"""
import argparse
import sys
from pathlib import Path
from typing import Dict, Any
import yaml
from jinja2 import Environment, FileSystemLoader, StrictUndefined
def render_template(template_path: Path, config: Dict[str, Any], output_path: Path) -> None:
"""Render a Jinja2 template with the given configuration."""
env = Environment(
loader=FileSystemLoader(template_path.parent),
undefined=StrictUndefined, # Fail on undefined variables
trim_blocks=True,
lstrip_blocks=True,
keep_trailing_newline=True,
)
template = env.get_template(template_path.name)
rendered = template.render(**config)
output_path.parent.mkdir(parents=True, exist_ok=True)
output_path.write_text(rendered)
print(f"✓ Rendered {output_path} from {template_path}")
def load_config(config_path: Path) -> Dict[str, Any]:
"""Load configuration from YAML file."""
with open(config_path) as f:
return yaml.safe_load(f)
def main():
parser = argparse.ArgumentParser(description="Render Containerfile templates")
parser.add_argument("config", nargs="?", help="Path to config YAML file")
parser.add_argument("--all", action="store_true", help="Render all configs")
parser.add_argument("--template", default="Containerfile.j2", help="Template file")
parser.add_argument("--check", action="store_true", help="Check if generated files match templates")
args = parser.parse_args()
template_path = Path(args.template)
if not template_path.exists():
print(f"Error: Template {template_path} not found", file=sys.stderr)
return 1
if args.all:
config_dir = Path("configs")
config_files = list(config_dir.glob("*.yaml"))
if not config_files:
print(f"Error: No config files found in {config_dir}", file=sys.stderr)
return 1
elif args.config:
config_files = [Path(args.config)]
else:
parser.print_help()
return 1
for config_path in config_files:
config = load_config(config_path)
# Determine output path from config
# e.g., datascience-gpu-py312.yaml -> jupyter/datascience/ubi9-python-3.12/Containerfile.gpu
variant = config.get("variant", "cpu")
python_version = config["python_version"]
output_path = Path(f"jupyter/datascience/ubi9-python-{python_version}/Containerfile.{variant}")
render_template(template_path, config, output_path)
return 0
if __name__ == "__main__":
sys.exit(main())5. Makefile integration: Makefile
.PHONY: generate-containerfiles
generate-containerfiles:
python tools/render_containerfiles.py --all
.PHONY: check-containerfiles
check-containerfiles: generate-containerfiles
@if git diff --exit-code jupyter/*/Containerfile.*; then \
echo "✓ All Containerfiles are up to date"; \
else \
echo "✗ Containerfiles are out of sync with templates"; \
echo "Run 'make generate-containerfiles' and commit the changes"; \
exit 1; \
fi6. CI validation: .github/workflows/validate-templates.yml
name: Validate Containerfile Templates
on: [pull_request]
jobs:
validate:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-version: '3.11'
- name: Install dependencies
run: |
pip install jinja2 pyyaml
- name: Check templates are in sync
run: make check-containerfilesKey advantages demonstrated:
- Comments work perfectly - shell comments, Dockerfile comments, heredocs all preserved
- Clear separation - template logic vs. configuration data
- Type safety -
StrictUndefinedcatches typos in variable names - Easy to diff - generated files can be committed and reviewed
- CI validation - ensures templates stay in sync
- No cryptic warnings - unlike CPP, Jinja2 doesn't complain about
#comments
This approach lets you maintain one template and generate multiple variants (CPU/GPU, Python 3.11/3.12, etc.) without the issues you encountered with CPP.
There was a problem hiding this comment.
- cpp (for podman to process *.in files; on macOS, install with:
podman machine ssh sudo dnf install -y cpp)
macOS
Install cpp using
podman machine ssh sudo dnf install -y cpp
If the above command fails due to lack of disk space, you can try to free up some space by running:
podman machine ssh sudo rpm-ostree usroverlay --hotfix
There was a problem hiding this comment.
@jiridanek Thanks for documenting the macOS setup instructions! These are helpful for anyone who wants to experiment with the Containerfile.in approach.
Suggested placement:
I'd recommend adding these instructions to a developer setup guide (e.g., docs/development/setup.md or CONTRIBUTING.md) rather than in the ADR itself, since the ADR focuses on architectural decisions rather than platform-specific tooling setup.
Enhanced version with context:
#### Installing cpp for Containerfile.in preprocessing (macOS)
If you want to experiment with Phase 1 templating (Containerfile.in with CPP), you'll need to install `cpp` in your Podman machine:
```commandline
podman machine ssh sudo dnf install -y cppTroubleshooting: If the above command fails due to lack of disk space, free up space first:
podman machine ssh sudo rpm-ostree usroverlay --hotfix
Then retry the cpp installation.
Note: Due to CPP's limitations with handling comments in Dockerfiles (especially in heredocs and complex scripts), this approach is recommended only for simple use cases. For production Dockerfiles, consider using Jinja2 templating (Phase 2) instead.
**Question:** Given the significant comment-handling issues we discussed earlier, are you planning to:
1. Document CPP setup for experimental/simple cases only, or
2. Actually use it for the complex Dockerfiles with heredocs?
If you're leaning toward option 2, we should revisit the ADR to add the warnings about comment handling we discussed.
<!-- This is an auto-generated reply by CodeRabbit -->
There was a problem hiding this comment.
@coderabbitai very well, I won't do Dockerfile.in templating, instead I'll start by turning inline bash to HEREDOC, and then extract it to separate files and deduplicating, as first step
- NO-JIRA: refactor(Dockerfile.cpu): wrap multiple RUN commands with bash for improved readability and error handling NO-JIRA: refactor(Dockerfile.cpu): wrap multiple RUN commands with bash for improved readability and error handling #2645
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch details about PR #2645 to understand the heredoc refactoring approach
gh pr view 2645 --json title,body,url,state,author
Length of output: 1770
@jiridanek That's an excellent pragmatic decision! 👍
Your phased approach makes a lot of sense:
- Inline bash → HEREDOC (PR NO-JIRA: refactor(Dockerfile.cpu): wrap multiple RUN commands with bash for improved readability and error handling #2645) - Improves readability and prepares for extraction
- Extract to separate script files - Enables reuse and testing in isolation
- Deduplication - Identify common patterns across variants
- Template with Jinja2 - Apply Phase 2 templating to both Dockerfiles and extracted scripts
This avoids the CPP comment-handling pitfalls entirely while achieving the same goals of reducing duplication and improving maintainability.
Recommendation for the ADR:
Since you're skipping Phase 1, I suggest updating the ADR in this PR to reflect your actual implementation plan:
- Add a section documenting why Phase 1 (Containerfile.in with CPP) was rejected due to comment handling issues
- Reframe the phased approach to start with heredoc conversion and script extraction
- Keep Phase 2 (Jinja2 templating) as the eventual goal
- Reference PR NO-JIRA: refactor(Dockerfile.cpu): wrap multiple RUN commands with bash for improved readability and error handling #2645 as the starting point
This way, the ADR will accurately document the decision-making process and help future contributors understand why CPP was avoided.
Would you like me to suggest specific wording for the ADR updates?
✏️ Learnings added
Learnt from: jiridanek
Repo: opendatahub-io/notebooks PR: 2643
File: docs/architecture/decisions/0005-implement-dockerfile-templating-to-remove-duplication.md:25-25
Timestamp: 2025-11-06T09:14:28.735Z
Learning: For the opendatahub-io/notebooks repository, Phase 1 (Containerfile.in with CPP preprocessing) was rejected due to CPP's inability to properly handle shell comments in heredocs and complex inline bash scripts. Instead, the approach is to first convert inline bash to heredocs, extract to separate script files for deduplication, and then apply Jinja2 templating (Phase 2) to both Dockerfiles and extracted scripts.
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.
1 participant
Summary by CodeRabbit