Skip to content

NO-JIRA: chore(docs/adr): document decision to implement Dockerfile templating to reduce duplication #2643

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

NO-JIRA: chore(docs/adr): document decision to implement Dockerfile templating to reduce duplication #2643

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae15bc6
NO-JIRA: chore(docs/adr): document decision to implement Dockerfile t…
jiridanek Nov 4, 2025
e3425bf
expand ADR to include references to prior deduplication efforts and r…
jiridanek Nov 4, 2025
796ad35
Update docs/architecture/decisions/0005-implement-dockerfile-templati…
jiridanek Nov 4, 2025
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
95 changes: 95 additions & 0 deletions ...tecture/decisions/0005-implement-dockerfile-templating-to-remove-duplication.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
# 5. Implement Dockerfile templating to remove duplication

Date: 2025-11-04

## Status

Accepted

## Context
The project contains multiple Dockerfiles/Containerfiles with significant duplication across different variants (CPU/GPU, different Python versions, different base distributions).
Maintaining these duplicated files is error-prone, as changes need to be manually propagated across multiple files, increasing the risk of inconsistencies, merge conflicts, and bugs.

We agreed to implement a templating solution to reduce duplication.

We are aware that introducing abstractions may make the project harder to understand for newcomers, and inappropriate abstractions would make it more difficult to make changes that used to be easy.
Therefore, we're committed to maintaining clarity and debuggability of the build process and to keeping the abstraction malleable so that it can be adjusted to future needs as the project evolves.

## Decision
We are already using the standard `${VARIABLE}` substitution via `podman build --build-arg-file`.

Implement Dockerfile/Containerfile templating using a phased approach:

### Phase 1: Containerfile.in with Podman/CPP

Begin using Podman's native `Containerfile.in` preprocessing feature (available since Podman 2.2.1).
Copy link
Member Author

@jiridanek jiridanek Nov 5, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 5, 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
# 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:

  1. Inline comments in RUN commands (especially with line continuations)
  2. Comment blocks as visual separators (#######...)
  3. 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:

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

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

  3. Reserve Containerfile.in for trivial cases like #include directives 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?

Copy link
Member Author

@jiridanek jiridanek Nov 5, 2025

Choose a reason for hiding this comment

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

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/

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 5, 2025

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

@jiridanek jiridanek Nov 6, 2025

Choose a reason for hiding this comment

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

@coderabbitai propose sample code to illustrate jinja2 templating

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 6, 2025

Choose a reason for hiding this comment

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

@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: true

3. 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: true

4. 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; \
	fi

6. 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-containerfiles

Key advantages demonstrated:

  1. Comments work perfectly - shell comments, Dockerfile comments, heredocs all preserved
  2. Clear separation - template logic vs. configuration data
  3. Type safety - StrictUndefined catches typos in variable names
  4. Easy to diff - generated files can be committed and reviewed
  5. CI validation - ensures templates stay in sync
  6. 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.

Copy link
Member Author

@jiridanek jiridanek Nov 6, 2025

Choose a reason for hiding this comment

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

@coderabbitai

  • 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

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 6, 2025

Choose a reason for hiding this comment

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

@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 cpp

Troubleshooting: 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 -->

Copy link
Member Author

@jiridanek jiridanek Nov 6, 2025

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 6, 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
# 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:

  1. 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
  2. Extract to separate script files - Enables reuse and testing in isolation
  3. Deduplication - Identify common patterns across variants
  4. 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:

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.


Containerfiles with `.in` suffix are automatically preprocessed via CPP(1), enabling:
- **C preprocessor directives**: `#include`, `#define`, `#ifdef`, `#if`, `#else`, `#endif`, etc.
- **File decomposition**: Break Containerfiles into reusable parts via `#include` directive
- **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
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 4, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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 mature

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


Note: Historical issues with comment handling (https://github.com/containers/buildah/issues/3229) were resolved in https://github.com/containers/buildah/pull/3241

### Phase 2: Python-based Templating
When Containerfile.in proves insufficient for complex templating needs, adopt a Python-based solution:

Options evaluated:
1. **Python f-strings**: Simple, native, no dependencies, good for basic interpolation
2. **stencils** (https://github.com/devcoons/stencils): Lightweight alternative, less mature
3. **Jinja2**: Industry-standard, powerful control structures (loops, conditionals), extensive ecosystem

Recommendation: Jinja2 for Phase 2 due to widespread adoption, maturity, and powerful templating capabilities.

### Generated Code Management Decision
Must decide whether to:
- **Option A: Commit generated Containerfiles to Git**
- Pros: Reproducible builds, easier debugging, clear history of what was built
- Cons: Potential for drift between templates and committed files, larger repo

- **Option B: Generate on-the-fly during build**
- Pros: Single source of truth, impossible for drift, cleaner repo
- Cons: Requires templating tooling in CI/CD, harder to debug build failures

Initial recommendation: Commit generated files (Option A) for traceability and easier debugging, with CI checks to ensure templates and generated files stay in sync.

## Consequences

### Positive
- Reduced duplication across Containerfiles
- Easier maintenance: changes to common patterns apply across all variants
- Lower risk of inconsistencies between variants
- Improved consistency in build configuration
- Clearer separation between variant-specific and common configuration

### Negative/Risks
- Additional complexity in the build process
- Learning curve for contributors unfamiliar with the templating system
- Risk of template bugs affecting multiple Containerfiles simultaneously
- Need to maintain tooling/scripts for template generation
- If committing generated files: need CI validation to catch drift
- If generating on-the-fly: build failures harder to debug without seeing the final Containerfile

### Mitigations
- Start with the simplest solution (Containerfile.in) and only move to more complex templating as needed
- Document templating approach clearly in the repository
- Implement CI checks to validate generated Containerfiles
- Provide clear error messages in templating scripts
- Keep templates as simple and readable as possible
- Consider a hybrid approach: commit generated files but validate against templates in CI

### Previous work

- <https://gitlab.cee.redhat.com/astonebe/notebook-utils>
- [RHOAIENG-16969 Remove specific instances of code duplication in odh/notebooks](https://issues.redhat.com/browse/RHOAIENG-16969)
- [RHOAIENG-19047 Deduplicate files that get included in workbench images](https://issues.redhat.com/browse/RHOAIENG-19047)
- [RHOAIENG-19046 Remove reliance on "chained builds" in notebooks repo](https://issues.redhat.com/browse/RHOAIENG-19046)
- [fix(makefile): standardized image targets #1015](https://github.com/opendatahub-io/notebooks/pull/1015)
- [RHOAIENG-16587: fix(test): ensure papermill tests run successfully for all supported notebooks #834](https://github.com/opendatahub-io/notebooks/pull/834)
- <https://redhat-internal.slack.com/archives/C060A5FJEAD/p1738786041914139>
- [📦 Consolidate duplicate bootstrapper implementations across Python 3.12 runtime environments #1349](https://github.com/opendatahub-io/notebooks/issues/1349)
- [improve and simplify docker multistage build in jupyter/datascience/ubi9-python-3.12/Dockerfile.cpu #2467](https://github.com/opendatahub-io/notebooks/issues/2467)
- [Refactor notebooks-release workflow to eliminate code duplication using reusable workflows #1185](https://github.com/opendatahub-io/notebooks/issues/1185)
Loading