[WIP] RHOAIENG-9707 ci: dynamic testing of container images with pytest #629
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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Things to check or do * [ ] how this works with podman * [ ] how to test both kubernetes and docker with the same tests * [ ] how to run the notebook server and check that that works * [ ] generate test_* methods for every target in Makefile * [ ] commandline parameter to specify image to test * [ ] without parameter with image to test it can build the image as part of test For now it uses images from https://github.com/jiridanek/notebooks/actions/runs/10006075164/job/27658080202 and not from the current build.
jiridanek
force-pushed
the
jd_helpful_error
branch
from
a0163e2 to
4b32eb2
Compare
|
PR needs rebase. 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. |
|
@jiridanek: The following tests 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. |
…lux/component-updates/component-update-odh-workbench-jupyter-trustyai-cpu-py311-ubi9-n-v2-22 Update odh-workbench-jupyter-trustyai-cpu-py311-ubi9-n-v2-22 to 269b6f1
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes introduce containerized testing infrastructure using Dagger.io and Testcontainers, update development dependencies, and add new test utilities and test cases for validating container images and notebook execution. The README is updated with details about container self-tests, and new configuration files and helper scripts are added to support robust and flexible container-based test workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant Pytest
participant Testcontainers
participant Docker
participant Notebook
Tester->>Pytest: Run tests
Pytest->>Testcontainers: Start Docker container
Testcontainers->>Docker: Launch container
Pytest->>Docker: Copy test data into container
Pytest->>Docker: Install dependencies (e.g., papermill)
Pytest->>Docker: Execute notebook via papermill
Docker->>Notebook: Run notebook
Notebook-->>Docker: Execution complete
Docker-->>Pytest: Return results
Pytest->>Testcontainers: Stop and cleanup container
Suggested labels
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
tests/__init__.py (1)
9-10: Fix spacing around assignment operators.Missing spaces around the
=operators in environment variable assignments.-os.environ["DO_NOT_TRACK"]= "1" -os.environ["NOTHANKS"]= "1" +os.environ["DO_NOT_TRACK"] = "1" +os.environ["NOTHANKS"] = "1"README.md (1)
67-80: Consider refining the documentation for clarity.The container selftests section provides good context, but the prominent TODO and uncertainty about the approach might confuse users. Consider:
- Moving the TODO to a less prominent position or converting it to a GitHub issue
- Providing clearer guidance on the current recommended approach
- Adding examples of when each tool might be preferred
##### Container selftests -We're using [Dagger.io](https://dagger.io) to run containers from Python tests. +We currently support [Dagger.io](https://dagger.io) for running containers from Python tests. It has very nice verbose logging by default for every action that is running. -**TODO** need to decide which is more suitable; -ideally the tests should allow switching between Kubernetes and Docker/Podman. -That means tying it to this fancy Dagger thing may not be the smartest thing to do. +We are evaluating support for switching between Kubernetes and Docker/Podman environments. We also considered [Testcontainers.com](https://testcontainers.com/). The main difference between the two is that Dagger creates more abstractions over the container engine. Especially Dagger [does not allow bind-mounting local directories](https://docs.dagger.io/cookbook/#modify-a-copied-directory-or-remote-repository-in-a-container) directly to the container but always copies files in and out.tests/docker_utils.py (2)
42-56: Consider reducing function complexity.The
container_execfunction has 13 parameters, which makes it difficult to use and maintain. Consider using a configuration object or builder pattern.However, given this is a low-level utility wrapper around Docker's exec API, the current parameter list mirrors the underlying API and may be acceptable for this use case.
114-116: Clarify or remove uncertain error handling.The comment and exception suggest uncertainty about when this condition could occur. Either provide a clear explanation or handle this case more definitively.
- while self.poll() is None: - raise RuntimeError("Hm could that really happen?") + # Wait for the command to complete + while self.poll() is None: + raise RuntimeError("Command execution did not complete properly")tests/test_check_dagger.py (1)
3-6: Remove unused imports.The
osandsysmodules are imported but not used in the active code.-import os import pathlib -import sys import logging
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
README.md(1 hunks)pyproject.toml(1 hunks)tests/__init__.py(1 hunks)tests/conftest.py(1 hunks)tests/docker_utils.py(1 hunks)tests/logs/.gitignore(1 hunks)tests/test_check_dagger.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/docker_utils.py
21-21: Use a context manager for opening files
(SIM115)
tests/test_check_dagger.py
3-3: os imported but unused
Remove unused import: os
(F401)
5-5: sys imported but unused
Remove unused import: sys
(F401)
295-295: Use a context manager for opening files
(SIM115)
🪛 Pylint (3.3.7)
tests/docker_utils.py
[error] 25-25: function already defined line 23
(E0102)
[refactor] 21-21: Consider using 'with' for resource-allocating operations
(R1732)
[refactor] 42-42: Too many arguments (13/5)
(R0913)
[refactor] 42-42: Too many positional arguments (13/5)
(R0917)
tests/test_check_dagger.py
[refactor] 184-184: Redefining argument with the local name 'cmd'
(R1704)
[refactor] 295-295: Consider using 'with' for resource-allocating operations
(R1732)
🔇 Additional comments (6)
tests/__init__.py (1)
5-6: Good approach for determining project root path.Using
pathlib.Path(__file__).parent.parentis a clean and reliable way to establish the project root directory for test utilities.pyproject.toml (1)
13-17: Verify that the dependency versions are current and secure.The new testing dependencies look appropriate for the container testing infrastructure. Please ensure these versions are current and free from known vulnerabilities.
Are there any known security vulnerabilities in pytest 8.3.2, pytest-subtests 0.13.1, pytest-logger 1.1.1, testcontainers 4.7.2, or pyfakefs 5.6.0?tests/logs/.gitignore (1)
1-1: Good practice for ignoring test log files.Properly excluding pytest log files from version control is the right approach for test infrastructure.
tests/conftest.py (3)
16-18: Proper cleanup implementation.The
pytest_sessionfinishhook correctly ensures cleanup of testcontainers Reaper instances after test completion. This prevents resource leaks and follows testcontainers best practices.
1-9: Good use of type checking imports.Using
TYPE_CHECKINGto conditionally import pytest types is an efficient approach that avoids runtime imports while providing proper type hints.
11-13: Consider security implications of privileged containers.While setting
ryuk_privileged = Truesolves SELinux issues with Podman, it grants elevated privileges to the Ryuk container. Ensure this is acceptable for your testing environment and consider documenting any security trade-offs.What are the security implications of running testcontainers Ryuk in privileged mode?
| tar = tarfile.open(fileobj=fh, mode="w:gz") | ||
|
|
||
| tar_filter = None | ||
| if user or group: | ||
| def tar_filter(f: tarfile.TarInfo) -> tarfile.TarInfo: | ||
| if user: | ||
| f.uid = user | ||
| if group: | ||
| f.gid = group | ||
| return f | ||
|
|
||
| logging.debug(f"Adding {src=} to archive {dst=}") | ||
| try: | ||
| tar.add(src, arcname=os.path.basename(src), filter=tar_filter) | ||
| finally: | ||
| tar.close() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use context manager for tarfile resource management.
The tarfile should be properly closed using a context manager to ensure resource cleanup.
- fh = io.BytesIO()
- tar = tarfile.open(fileobj=fh, mode="w:gz")
+ fh = io.BytesIO()
+ with tarfile.open(fileobj=fh, mode="w:gz") as tar:
+ tar_filter = None
+ if user or group:
+ def tar_filter(f: tarfile.TarInfo) -> tarfile.TarInfo:
+ if user:
+ f.uid = user
+ if group:
+ f.gid = group
+ return f
- tar_filter = None
- if user or group:
- def tar_filter(f: tarfile.TarInfo) -> tarfile.TarInfo:
- if user:
- f.uid = user
- if group:
- f.gid = group
- return f
-
- logging.debug(f"Adding {src=} to archive {dst=}")
- try:
- tar.add(src, arcname=os.path.basename(src), filter=tar_filter)
- finally:
- tar.close()
+ logging.debug(f"Adding {src=} to archive {dst=}")
+ tar.add(src, arcname=os.path.basename(src), filter=tar_filter)📝 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.
| tar = tarfile.open(fileobj=fh, mode="w:gz") | |
| tar_filter = None | |
| if user or group: | |
| def tar_filter(f: tarfile.TarInfo) -> tarfile.TarInfo: | |
| if user: | |
| f.uid = user | |
| if group: | |
| f.gid = group | |
| return f | |
| logging.debug(f"Adding {src=} to archive {dst=}") | |
| try: | |
| tar.add(src, arcname=os.path.basename(src), filter=tar_filter) | |
| finally: | |
| tar.close() | |
| fh = io.BytesIO() | |
| with tarfile.open(fileobj=fh, mode="w:gz") as tar: | |
| tar_filter = None | |
| if user or group: | |
| def tar_filter(f: tarfile.TarInfo) -> tarfile.TarInfo: | |
| if user: | |
| f.uid = user | |
| if group: | |
| f.gid = group | |
| return f | |
| logging.debug(f"Adding {src=} to archive {dst=}") | |
| tar.add(src, arcname=os.path.basename(src), filter=tar_filter) |
🧰 Tools
🪛 Ruff (0.11.9)
21-21: Use a context manager for opening files
(SIM115)
🪛 Pylint (3.3.7)
[error] 25-25: function already defined line 23
(E0102)
[refactor] 21-21: Consider using 'with' for resource-allocating operations
(R1732)
🤖 Prompt for AI Agents
In tests/docker_utils.py around lines 21 to 36, the tarfile is opened and closed
manually, which risks resource leaks if exceptions occur. Replace the manual
open and close calls with a context manager (using the "with" statement) to
automatically handle closing the tarfile resource. This ensures proper cleanup
even if errors happen during the tarfile operations.
| uid = 123456 | ||
| gid = 0 | ||
| test_data_dir = str(ROOT_PATH / "jupyter" / notebook_name / f"{ubi_flavor}-{python_kernel}" / "test") | ||
| image = "ghcr.io/jiridanek/notebooks/workbench-images:jupyter-minimal-ubi9-python-3.9-jd_helpful_error_751147cd93fed327e940670edbc99c6f44a1ac24" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded image reference with parameterized value.
The hardcoded image name should be configurable to support testing different images and versions.
Consider adding a pytest fixture or parameter to make the image configurable:
@pytest.mark.parametrize("image", [
"ghcr.io/jiridanek/notebooks/workbench-images:jupyter-minimal-ubi9-python-3.9-jd_helpful_error_751147cd93fed327e940670edbc99c6f44a1ac24"
])
def test_something_with_papermill(image):
# ... rest of the test🤖 Prompt for AI Agents
In tests/test_check_dagger.py at line 33, the image reference is hardcoded,
limiting flexibility. Refactor the test to accept the image as a parameter by
adding a pytest fixture or using pytest.mark.parametrize to pass the image
string. Update the test function signature to accept this parameter and replace
the hardcoded image variable with the parameter value to allow testing with
different images and versions.
| # # run_kubectl(["wait", "--for=condition=ready", "pod", "runtime-pod", "--timeout=300s"]) | ||
| # # LOGGER.error("Usage: make validate-runtime-image image=<container-image-name>") | ||
| # # fail = False | ||
| image = "ghcr.io/jiridanek/notebooks/workbench-images:runtime-minimal-ubi9-python-3.9-jd_helpful_error_751147cd93fed327e940670edbc99c6f44a1ac24" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded image reference with parameterized value.
Similar to the other test, this hardcoded image name should be configurable.
Consider using the same parameterization approach as suggested for the papermill test.
🤖 Prompt for AI Agents
In tests/test_check_dagger.py at line 178, the Docker image reference is
hardcoded, which reduces flexibility. Replace the hardcoded image string with a
parameter or fixture that can be configured externally or passed into the test,
following the parameterization approach used in the papermill test. This will
make the test more adaptable and maintainable.
| LOGGER.info(f"=> Checking container image {image} for {cmd} command...") | ||
| # # r = run_kubectl(["exec", f"runtime-pod", "which {cmd} > /dev/null 2>&1"], check=False) | ||
| r = docker_utils.container_exec(container.get_wrapped_container(), | ||
| cmd=["/bin/bash", "-c", f"which {cmd} > /dev/null 2>&1"], | ||
| stream=True) | ||
| # # if r.returncode != 0: | ||
| # # LOGGER.error("ERROR: Container image $$image does not meet criteria for command: $$cmd") | ||
| # # fail = True | ||
| # # continue | ||
| assert r.communicate() == 0 | ||
| if cmd == "python3": | ||
| LOGGER.info("=> Checking notebook execution...") | ||
|
|
||
|
|
||
| # # await c.with_exec(use_entrypoint=True, args=[]) | ||
| # # print("default artgs", await c.default_args()) | ||
| # # TODO: I don't see elyra/ directory on the image | ||
| # # await c.with_exec(["/bin/bash", "-c", "python3 -m pip install -r /opt/app-root/elyra/requirements-elyra.txt" | ||
| # # " && curl https://github.com/nteract/papermill/main/papermill/tests/notebooks/simple_execute.ipynb --output simple_execute.ipynb" | ||
| # # " && python3 -m papermill simple_execute.ipynb output.ipynb > /dev/null"]) | ||
| # # r = run_kubectl(["exec", "runtime-pod", "/bin/sh", "-c", , check=False) | ||
| # # if r.returncode != 0: | ||
| # # LOGGER.error("ERROR: Image does not meet Python requirements criteria in requirements-elyra.txt") | ||
| # # fail = True | ||
| # # assert not fail, "=> ERROR: Container image $$image is not a suitable Elyra runtime image" | ||
| # # LOGGER.info(f"=> Container image {image} is a suitable Elyra runtime image") | ||
| # | ||
| # | ||
| # async def test_validate_codeserver_image(): | ||
| # # codeserver_pod_ready = run_kubectl( | ||
| # # ["wait", "--for=condition=ready", "pod", "codeserver-pod", "--timeout=300s"], check=False) | ||
| # # assert codeserver_pod_ready.returncode == 0, "Code-server pod did not become ready within expected time" | ||
| # | ||
| # # assert image, "Usage: make validate-codeserver-image image=<container-image-name>" | ||
| # | ||
| # image = "ghcr.io/jiridanek/notebooks/workbench-images:codeserver-ubi9-python-3.9-jd_helpful_error_751147cd93fed327e940670edbc99c6f44a1ac24" | ||
| # async with dagger.Connection(dagger.Config(log_output=sys.stderr)) as client: | ||
| # c = (client.container().from_(image)) | ||
| # for cmd in REQUIRED_CODE_SERVER_IMAGE_COMMANDS: | ||
| # await c.with_exec(["/bin/bash", "-c", f"which {cmd} > /dev/null 2>&1"]) | ||
| # # result = run_kubectl(["exec", "codeserver-pod", f"which {cmd} > /dev/null 2>&1"], check=False) | ||
| # # assert result.returncode == 0, f"ERROR: Container image {image} does not meet criteria for command: {cmd}" | ||
| # | ||
| # | ||
| # # async def validate_rstudio_image(client: dagger.Client, c: dagger.Container): | ||
| # async def test_validate_rstudio_image(): | ||
| # image = "ghcr.io/jiridanek/notebooks/workbench-images:rstudio-c9s-python-3.9-jd_helpful_error_751147cd93fed327e940670edbc99c6f44a1ac24" | ||
| # | ||
| # notebook_name = "" | ||
| # ubi_flavor = "c9s" | ||
| # python_kernel = "python-3.9" | ||
| # | ||
| # async with (dagger.Connection(dagger.Config(log_output=sys.stderr)) as client): | ||
| # c = (client.container() | ||
| # .from_(image)) | ||
| # | ||
| # # $(eval NOTEBOOK_NAME := $(subst .,-,$(subst cuda-,,$*))) | ||
| # LOGGER.info("# Running tests for $(NOTEBOOK_NAME) RStudio Server image...") | ||
| # # rstudo_pod_ready = run_kubectl(["wait", "--for=condition=ready", "pod", "rstudio-pod", "--timeout=300s"], check=False) | ||
| # # assert rstudo_pod_ready.returncode == 0, "Code-server pod did not become ready within expected time" | ||
| # # assert image, "Usage: make validate-rstudio-image image=<container-image-name>" | ||
| # | ||
| # LOGGER.info("=> Checking container image $$image for package intallation...") | ||
| # c = c.with_exec(["/bin/bash", "-c", "mkdir -p /opt/app-root/src/R/temp-library > /dev/null 2>&1"]) | ||
| # c = c.with_exec(["/bin/bash", "-c", | ||
| # '''R -e "install.packages('tinytex', lib='/opt/app-root/src/R/temp-library')" > /dev/null 2>&1''']) | ||
| # await c | ||
| # | ||
| # for cmd in REQUIRED_R_STUDIO_IMAGE_COMMANDS: | ||
| # LOGGER.info(f"=> Checking container image {image} for {cmd}...") | ||
| # # which_cmd = run_kubectl(["exec", "rstudio-pod", f"which {cmd} > /dev/null 2>&1"], check=False) | ||
| # await c.with_exec(["/bin/bash", "-c", f"which {cmd} > /dev/null 2>&1"]) | ||
| # # if which_cmd.returncode == 0: | ||
| # # LOGGER.info(f"{cmd} executed successfully!") | ||
| # # else: | ||
| # # LOGGER.error("ERROR: Container image {image} does not meet criteria for command: {cmd}") | ||
| # # fail = True | ||
| # # continue | ||
| # | ||
| # LOGGER.info("=> Fetching R script from URL and executing on the container...") | ||
| # # run_command(["curl", "-sSL", "-o", "test_script.R" f"{NOTEBOOK_REPO_BRANCH_BASE}/rstudio/c9s-python-3.9/test/test_script.R"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) | ||
| # # run_kubectl(["cp", "test_script.R", "rstudio-pod:/opt/app-root/src/test_script.R"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) | ||
| # # test_script = run_kubectl(["exec", "rstudio-pod", "--", "Rscript /opt/app-root/src/test_script.R > /dev/null 2>&1"]) | ||
| # r = client.host().directory(str(ROOT_PATH / "rstudio" / f"{ubi_flavor}-{python_kernel}" / "test")) | ||
| # d = (c | ||
| # .with_directory("/test", r) | ||
| # .with_workdir("/opt/app-data") | ||
| # .with_exec(["/bin/sh", "-c", "Rscript /test/test_script.R > /dev/null 2>&1"]) | ||
| # ) | ||
| # await d | ||
| # | ||
| # # if test_script.returncode == 0: | ||
| # # LOGGER.info("R script executed successfully!") | ||
| # # os.unlink("test_script.R") | ||
| # # else: | ||
| # # LOGGER.error("Error: R script failed.") | ||
| # # fail = True | ||
| # # | ||
| # # assert not fail | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up extensive commented code.
There's a large amount of commented-out code that makes the file difficult to navigate and maintain. Consider either:
- Implementing the functionality if it's needed
- Moving it to a separate development/draft file
- Removing it entirely if it's obsolete
The commented code appears to contain valuable implementations for Kubernetes-based testing and Dagger async patterns that could be useful for future development.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 184-184: Redefining argument with the local name 'cmd'
(R1704)
🤖 Prompt for AI Agents
In tests/test_check_dagger.py from lines 61 to 284, there is a large block of
commented-out code that clutters the file and reduces maintainability. Review
this commented code and decide whether to implement the needed functionality,
move it to a separate draft or development file for future reference, or remove
it entirely if it is obsolete. This cleanup will improve readability and
maintainability of the test file.
| with open("/config.txt", "wt") as fp: | ||
| fp.writelines(lines) |
There was a problem hiding this comment.
Fix hardcoded filename in blockinfile function.
The function writes to a hardcoded /config.txt instead of the provided filename parameter, which appears to be a bug.
- with open("/config.txt", "wt") as fp:
+ with open(filename, "wt") as fp:
fp.writelines(lines)📝 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.
| with open("/config.txt", "wt") as fp: | |
| fp.writelines(lines) | |
| with open(filename, "wt") as fp: | |
| fp.writelines(lines) |
🤖 Prompt for AI Agents
In tests/test_check_dagger.py around lines 310 to 311, the code writes to a
hardcoded filename "/config.txt" instead of using the provided filename
parameter. Replace the hardcoded string "/config.txt" with the variable filename
to ensure the function writes to the intended file.
| """ | ||
| begin = end = -1 | ||
|
|
||
| lines = open(filename, "rt").readlines() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use context manager for file operations.
The file should be opened using a context manager to ensure proper resource cleanup.
- lines = open(filename, "rt").readlines()
+ with open(filename, "rt") as f:
+ lines = f.readlines()📝 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.
| lines = open(filename, "rt").readlines() | |
| with open(filename, "rt") as f: | |
| lines = f.readlines() |
🧰 Tools
🪛 Ruff (0.11.9)
295-295: Use a context manager for opening files
(SIM115)
🪛 Pylint (3.3.7)
[refactor] 295-295: Consider using 'with' for resource-allocating operations
(R1732)
🤖 Prompt for AI Agents
In tests/test_check_dagger.py at line 295, the file is opened without a context
manager, which risks resource leaks. Replace the open call with a with statement
to open the file, read the lines inside the block, and ensure the file is
properly closed after reading.
| for cmd in REQUIRED_RUNTIME_IMAGE_COMMANDS: | ||
| LOGGER.info(f"=> Checking container image {image} for {cmd} command...") | ||
| # # r = run_kubectl(["exec", f"runtime-pod", "which {cmd} > /dev/null 2>&1"], check=False) | ||
| r = docker_utils.container_exec(container.get_wrapped_container(), | ||
| cmd=["/bin/bash", "-c", f"which {cmd} > /dev/null 2>&1"], | ||
| stream=True) | ||
| # # if r.returncode != 0: | ||
| # # LOGGER.error("ERROR: Container image $$image does not meet criteria for command: $$cmd") | ||
| # # fail = True | ||
| # # continue | ||
| assert r.communicate() == 0 |
There was a problem hiding this comment.
Fix variable name conflict in loop.
The loop variable cmd shadows the function parameter cmd, which is confusing and flagged by static analysis.
- for cmd in REQUIRED_RUNTIME_IMAGE_COMMANDS:
- LOGGER.info(f"=> Checking container image {image} for {cmd} command...")
+ for required_cmd in REQUIRED_RUNTIME_IMAGE_COMMANDS:
+ LOGGER.info(f"=> Checking container image {image} for {required_cmd} command...")
r = docker_utils.container_exec(container.get_wrapped_container(),
- cmd=["/bin/bash", "-c", f"which {cmd} > /dev/null 2>&1"],
+ cmd=["/bin/bash", "-c", f"which {required_cmd} > /dev/null 2>&1"],
stream=True)
assert r.communicate() == 0
- if cmd == "python3":
+ if required_cmd == "python3":
LOGGER.info("=> Checking notebook execution...")📝 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.
| for cmd in REQUIRED_RUNTIME_IMAGE_COMMANDS: | |
| LOGGER.info(f"=> Checking container image {image} for {cmd} command...") | |
| # # r = run_kubectl(["exec", f"runtime-pod", "which {cmd} > /dev/null 2>&1"], check=False) | |
| r = docker_utils.container_exec(container.get_wrapped_container(), | |
| cmd=["/bin/bash", "-c", f"which {cmd} > /dev/null 2>&1"], | |
| stream=True) | |
| # # if r.returncode != 0: | |
| # # LOGGER.error("ERROR: Container image $$image does not meet criteria for command: $$cmd") | |
| # # fail = True | |
| # # continue | |
| assert r.communicate() == 0 | |
| for required_cmd in REQUIRED_RUNTIME_IMAGE_COMMANDS: | |
| LOGGER.info(f"=> Checking container image {image} for {required_cmd} command...") | |
| # # r = run_kubectl(["exec", f"runtime-pod", "which {cmd} > /dev/null 2>&1"], check=False) | |
| r = docker_utils.container_exec(container.get_wrapped_container(), | |
| cmd=["/bin/bash", "-c", f"which {required_cmd} > /dev/null 2>&1"], | |
| stream=True) | |
| # # if r.returncode != 0: | |
| # # LOGGER.error("ERROR: Container image $$image does not meet criteria for command: $$cmd") | |
| # # fail = True | |
| # # continue | |
| assert r.communicate() == 0 | |
| if required_cmd == "python3": | |
| LOGGER.info("=> Checking notebook execution...") |
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 184-184: Redefining argument with the local name 'cmd'
(R1704)
🤖 Prompt for AI Agents
In tests/test_check_dagger.py around lines 184 to 194, the loop variable named
'cmd' conflicts with a function parameter of the same name, causing confusion
and static analysis warnings. Rename the loop variable to a distinct name such
as 'command' to avoid shadowing the function parameter. Update all references
inside the loop accordingly to use the new variable name.
Rebooted in
oc versionin the test, which can be run with ci testing #829This is a followup to
https://issues.redhat.com/browse/RHOAIENG-9707
Description
Example run
https://github.com/jiridanek/notebooks/actions/runs/10040438261/job/27746490803#step:9:19
Things to check or do
For now it uses images from
https://github.com/jiridanek/notebooks/actions/runs/10006075164/job/27658080202
and not from the current build.
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
New Features
Documentation
Chores
.gitignoreto exclude test log files from version control.