-
Notifications
You must be signed in to change notification settings - Fork 52
LCORE-399: Update README how to create custom image #533
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.
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
Conversation
WalkthroughAdded a new "Custom Container Image" entry to README.md (ToC and section), providing step-by-step examples (pyproject.toml, Containerfile), optional config files, and a podman build command for creating library-mode lightspeed-stack custom images. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (6)
README.md (6)
54-54: Fix ToC indentation to satisfy markdownlint (MD007).Use 2-space indentation for this subitem.
- * [Custom Container Image](#custom-container-image) + * [Custom Container Image](#custom-container-image)
692-694: Typo and phrasing: “instructons” → “instructions”; clarify sentence.Minor docs polish.
-Follow these instructons when you need to bundle either additional configuration -files or more dependencies e.g. `lightspeed-stack-providers`. +Follow these instructions when you need to bundle additional configuration +files or extra dependencies (e.g., `lightspeed-stack-providers`).
741-742: Clean package manager caches to keep the image lean.Add cleanup after microdnf. Also consider making these RPMs optional in the sample.
-RUN microdnf install -y --nodocs --setopt=keepcache=0 --setopt=tsflags=nodocs TODO1 TODO2 +RUN microdnf install -y --nodocs --setopt=keepcache=0 --setopt=tsflags=nodocs TODO1 TODO2 \ + && microdnf clean all \ + && rm -rf /var/cache/dnf
699-709: Add language to fenced code block (MD040) and nudge to pin provider version.-``` +```toml [project] name = "my-customized-chatbot" @@ dependencies = [ - "lightspeed-stack-providers==TODO", + # Replace with a pinned version compatible with your base image + "lightspeed-stack-providers==0.1.0", ]--- `712-754`: **Add language to Containerfile fenced block (MD040) and make APP_ROOT used.** Minor clarity and robustness. ```diff -``` +```Dockerfile # Latest dev image built from the git main branch FROM quay.io/lightspeed-core/lightspeed-stack:dev-latest @@ -ARG APP_ROOT=/app-root -WORKDIR /app-root +ARG APP_ROOT=/app-root +WORKDIR ${APP_ROOT} @@ -ENV PATH="/app-root/.venv/bin:$PATH" +ENV PATH="${APP_ROOT}/.venv/bin:$PATH"--- `758-760`: **Add language to fenced code block (MD040).** ```diff -``` +```bash podman build -t "my-awesome-chatbot:latest" .</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 826dadb719c240c88533197b0fd66f4c3f604879 and e38796b90cc5f1d9a43ff5bbae1802525abedf53. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `README.md` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (1)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: matysek
PR: #292
File: pyproject.toml:0-0
Timestamp: 2025-08-18T10:56:55.349Z
Learning: The lightspeed-stack project intentionally uses a "generic image" approach, bundling many dependencies directly in the base runtime image to work for everyone, rather than using lean base images with optional dependency groups.</details> </details><details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>README.md</summary> 54-54: Unordered list indentation Expected: 2; Actual: 4 (MD007, ul-indent) --- 699-699: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 712-712: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 758-758: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)</summary> * GitHub Check: build-pr * GitHub Check: e2e_tests </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>README.md (1)</summary><blockquote> `687-761`: **Overall: solid addition; aligns with “generic image” strategy.** The guidance fits the project’s “generic image” approach (bundling common deps) while showing how to extend safely. After addressing the above nits/bugs, this section will be very usable. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| RUN python -c "import tomllib, sys; print(' '.join(tomllib.load(open('pyproject.toml','rb'))['project']['dependencies']))" \ | ||
| | xargs uv pip install --no-deps | ||
| # Install the project itself | ||
| RUN uv pip install . --no-deps && uv clean | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Dependency install via xargs is unsafe with environment markers/spaces.
Requirements like pkg; python_version < "3.13" get split by xargs. Write a requirements file and install from it.
-RUN python -c "import tomllib, sys; print(' '.join(tomllib.load(open('pyproject.toml','rb'))['project']['dependencies']))" \
- | xargs uv pip install --no-deps
-# Install the project itself
-RUN uv pip install . --no-deps && uv clean
+RUN python - <<'PY'
+import tomllib
+from pathlib import Path
+deps = tomllib.load(open('pyproject.toml','rb'))['project'].get('dependencies', [])
+Path('/tmp/deps.txt').write_text('\n'.join(deps) + '\n')
+PY
+RUN uv pip install --no-deps -r /tmp/deps.txt && rm -f /tmp/deps.txt
+# Install the project itself
+RUN uv pip install . --no-deps && uv clean📝 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.
| RUN python -c "import tomllib, sys; print(' '.join(tomllib.load(open('pyproject.toml','rb'))['project']['dependencies']))" \ | |
| | xargs uv pip install --no-deps | |
| # Install the project itself | |
| RUN uv pip install . --no-deps && uv clean | |
| RUN python - <<'PY' | |
| import tomllib | |
| from pathlib import Path | |
| deps = tomllib.load(open('pyproject.toml','rb'))['project'].get('dependencies', []) | |
| Path('/tmp/deps.txt').write_text('\n'.join(deps) + '\n') | |
| PY | |
| RUN uv pip install --no-deps -r /tmp/deps.txt && rm -f /tmp/deps.txt | |
| # Install the project itself | |
| RUN uv pip install . --no-deps && uv clean |
🤖 Prompt for AI Agents
In README.md around lines 733 to 737, the current approach pipes dependency
strings through xargs which breaks requirements that include environment markers
or spaces; instead, generate a temporary requirements file from pyproject.toml
using tomllib (writing each dependency line as-is to the file to preserve
markers/quoting), then call pip install -r <that-file> (and similarly pip
install . using --no-deps), and finally remove the temporary file; update the
Dockerfile steps to create the requirements file, use pip -r to install
dependencies safely, and clean up the file afterwards.
| ENV PATH="/app-root/.venv/bin:$PATH" | ||
| # Run the application | ||
| EXPOSE 8080 | ||
| ENTRYPOINT ["python3.12", "src/lightspeed_stack.py"] | ||
| USER 1001 | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ENTRYPOINT bypasses the uv venv; use venv’s python and module form.
python3.12 may not be the venv interpreter; use python -m lightspeed_stack.
-# Add executables from .venv to system PATH
-ENV PATH="/app-root/.venv/bin:$PATH"
+# Add executables from .venv to system PATH
+ENV PATH="/app-root/.venv/bin:$PATH"
@@
-ENTRYPOINT ["python3.12", "src/lightspeed_stack.py"]
+ENTRYPOINT ["python", "-m", "lightspeed_stack"]📝 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.
| ENV PATH="/app-root/.venv/bin:$PATH" | |
| # Run the application | |
| EXPOSE 8080 | |
| ENTRYPOINT ["python3.12", "src/lightspeed_stack.py"] | |
| USER 1001 | |
| ``` | |
| # Add executables from .venv to system PATH | |
| ENV PATH="/app-root/.venv/bin:$PATH" | |
| # Run the application | |
| EXPOSE 8080 | |
| ENTRYPOINT ["python", "-m", "lightspeed_stack"] | |
| USER 1001 |
🤖 Prompt for AI Agents
In README.md around lines 747 to 753, the Docker ENTRYPOINT currently calls a
system interpreter ("python3.12") which can bypass the virtualenv; change it to
invoke the venv's python from PATH and use module form: set ENTRYPOINT to run
"python -m lightspeed_stack" (array form) so the image uses the
/app-root/.venv/bin/python on PATH and runs your package as a module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (3)
README.md (3)
724-725: Optional configs copied unconditionally will break sample build when files are absent.Comment out by default and add guidance.
-# Bundle own configuration files -COPY lightspeed-stack.yaml run.yaml ./ +# Bundle configuration files (optional). Uncomment if present in your build context. +# COPY lightspeed-stack.yaml run.yaml ./
733-737: Installing deps via xargs breaks env markers/spaces; write a requirements file first.Environment markers (e.g., ; python_version < "3.13") get split by xargs.
-RUN python -c "import tomllib, sys; print(' '.join(tomllib.load(open('pyproject.toml','rb'))['project']['dependencies']))" \ - | xargs uv pip install --no-deps -# Install the project itself -RUN uv pip install . --no-deps && uv clean +RUN python - <<'PY' +import tomllib +from pathlib import Path +deps = tomllib.load(open('pyproject.toml','rb'))['project'].get('dependencies', []) +Path('/tmp/deps.txt').write_text('\n'.join(deps) + '\n') +PY +RUN uv pip install --no-deps -r /tmp/deps.txt && rm -f /tmp/deps.txt +# Install the project itself +RUN uv pip install . --no-deps && uv clean
749-753: ENTRYPOINT should use venv interpreter and module form.Calling python3.12 can bypass the uv-created venv; prefer python -m.
-ENTRYPOINT ["python3.12", "src/lightspeed_stack.py"] +ENTRYPOINT ["python", "-m", "lightspeed_stack"]
🧹 Nitpick comments (7)
README.md (7)
54-54: Fix ToC list indentation (markdownlint MD007).The new ToC item is indented inconsistently. Align with surrounding list style or regenerate ToC.
Apply one of the following minimal fixes (pick the one matching your intended hierarchy):
- * [Custom Container Image](#custom-container-image) + * [Custom Container Image](#custom-container-image)or, if it should be a child under “Building Container Images”:
- * [Custom Container Image](#custom-container-image) + * [Custom Container Image](#custom-container-image)
712-712: Add language to fenced code block (markdownlint MD040).Tag the Containerfile block for syntax highlighting.
-``` +```dockerfile
760-760: Add language to fenced code block (markdownlint MD040).Tag the build command block.
-``` +```bash
713-715: Base image tag now correct; consider pinning a digest for reproducibility.Using dev-latest is fine; pinning a digest avoids accidental updates.
Example:
-FROM quay.io/lightspeed-core/lightspeed-stack:dev-latest +# Consider digest pinning for reproducible builds: +# FROM quay.io/lightspeed-core/lightspeed-stack@sha256:<digest> +FROM quay.io/lightspeed-core/lightspeed-stack:dev-latest
716-718: Use APP_ROOT consistently.You define ARG APP_ROOT but hardcode /app-root elsewhere. Improves maintainability.
-ARG APP_ROOT=/app-root -WORKDIR /app-root +ARG APP_ROOT=/app-root +WORKDIR ${APP_ROOT} @@ -ENV PATH="/app-root/.venv/bin:$PATH" +ENV PATH="${APP_ROOT}/.venv/bin:$PATH"Also applies to: 749-749
741-744: Avoid TODO rpm placeholders that fail builds.Comment out or show as an example to prevent copy-paste build failures.
-# Bundle additional rpm packages -RUN microdnf install -y --nodocs --setopt=keepcache=0 --setopt=tsflags=nodocs TODO1 TODO2 \ - && microdnf clean all \ - && rm -rf /var/cache/dnf +# Bundle additional rpm packages (optional) +# RUN microdnf install -y --nodocs --setopt=keepcache=0 --setopt=tsflags=nodocs <pkg1> <pkg2> \ +# && microdnf clean all \ +# && rm -rf /var/cache/dnf
699-709: Replace placeholder dependency with a pinned version and guidance.Make the example copy/paste-safe with an explicit pin and a comment.
dependencies = [ - "lightspeed-stack-providers==TODO", + # Replace with a real pinned version to ensure reproducible builds, e.g.: + # "lightspeed-stack-providers==0.2.3", + "lightspeed-stack-providers==<PINNED_VERSION>", ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
54-54: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
712-712: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
760-760: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
| Follow these instructons when you need to bundle additional configuration | ||
| files or extra dependencies (e.g. `lightspeed-stack-providers`). | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: “instructons” → “instructions”.
Small spelling fix in the new section intro.
-Follow these instructons when you need to bundle additional configuration
+Follow these instructions when you need to bundle additional configuration📝 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.
| Follow these instructons when you need to bundle additional configuration | |
| files or extra dependencies (e.g. `lightspeed-stack-providers`). | |
| Follow these instructions when you need to bundle additional configuration | |
| files or extra dependencies (e.g. `lightspeed-stack-providers`). |
🤖 Prompt for AI Agents
In README.md around lines 692 to 694, there's a spelling mistake: "instructons"
should be corrected to "instructions"; update the word in that sentence to fix
the typo.
radofuchs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tisnik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
LCORE-399: Update README how to create custom image
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit