Include store disk usage stats in info output#2319
Open
olliewalsh wants to merge 1 commit into
Open
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
olliewalsh
force-pushed
the
konflux_tmp
branch
2 times, most recently
from
dbd8dd4 to
a5c4d5c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Member
|
Good idea on this feature. |
olliewalsh
force-pushed
the
konflux_tmp
branch
from
a5c4d5c to
414ab84
Compare
olliewalsh
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
olliewalsh
requested review from
bmahabirbu,
cgruver,
engelmi,
jhjaggars,
maxamillion,
rhatdan and
swarajpande5
as code owners
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
engelmi
reviewed
Feb 5, 2026
This comment was marked as resolved.
This comment was marked as resolved.
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded a function to compute disk usage for a store path with upward traversal on errors, exposed it in the CLI Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "ramalama.cli"
participant Common as "ramalama.common"
participant OS as "shutil / filesystem"
User->>CLI: run `ramalama info --store <path>`
CLI->>Common: store_disk_stats(Path(<path>))
Common->>OS: shutil.disk_usage(<path>)
alt disk_usage succeeds
OS-->>Common: (total, used, free)
else disk_usage raises OSError
Common->>OS: shutil.disk_usage(parent path) (repeat until root)
alt eventually succeeds
OS-->>Common: (total, used, free)
else reaches root without success
OS-->>Common: raises OSError / no usable path
Common-->>Common: return {"total": null, "used": null, "free": null}
end
end
Common-->>CLI: dict with disk stats or nulls
CLI-->>User: JSON output includes "StoreUsage"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/unit/test_common.py (1)
525-532: Consider asserting the traversal call sequence explicitly.These tests validate outputs, but they don’t assert that Line 526-532 actually walked
/tmp/store -> /tmp -> /. Addingassert_has_calls(...)would tighten regression detection for parent-walk logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/test_common.py` around lines 525 - 532, Add explicit assertions that the mocked disk usage function was called in the expected parent-traversal order: after calling store_disk_stats in tests test_store_disk_stats_no_store_dir and test_store_disk_stats_root_not_found, assert mock_disk_usage.assert_has_calls([call("/tmp/store"), call("/tmp"), call("/")]) (using unittest.mock.call) so the tests verify the walk sequence when store_disk_stats (the function under test) attempts disk_usage on the store path, its parent, and root.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ramalama/common.py`:
- Around line 793-799: The disk usage loop currently only catches
FileNotFoundError which misses other OSError subclasses (e.g., PermissionError);
update the exception handler around shutil.disk_usage(current_path) to catch
OSError instead of FileNotFoundError and keep the fallback/parent-walking logic,
and simplify the return by unpacking the tuple from shutil.disk_usage into
total, used, free and returning {"total": total, "used": used, "free": free}
rather than using dict(zip(...)); reference the shutil.disk_usage call and
current_path in the loop to locate the change.
---
Nitpick comments:
In `@test/unit/test_common.py`:
- Around line 525-532: Add explicit assertions that the mocked disk usage
function was called in the expected parent-traversal order: after calling
store_disk_stats in tests test_store_disk_stats_no_store_dir and
test_store_disk_stats_root_not_found, assert
mock_disk_usage.assert_has_calls([call("/tmp/store"), call("/tmp"), call("/")])
(using unittest.mock.call) so the tests verify the walk sequence when
store_disk_stats (the function under test) attempts disk_usage on the store
path, its parent, and root.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a772f485-47f8-4a3a-91c9-93ac9035a4e6
📒 Files selected for processing (3)
ramalama/cli.pyramalama/common.pytest/unit/test_common.py
This comment was marked as outdated.
This comment was marked as outdated.
olliewalsh
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
olliewalsh
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
olliewalsh
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ramalama/common.py (1)
805-825:⚠️ Potential issue | 🟠 MajorCatch broader OS errors in
store_disk_statsto preventinfocrashes.At Line 821, only
FileNotFoundErroris handled;shutil.disk_usage()can fail with otherOSErrorsubclasses (for examplePermissionError). Also, Line 820/Line 824 can avoidzip(...)entirely.🔧 Proposed fix
-def store_disk_stats(store_path: Path) -> dict[str, Optional[int]] | None: +def store_disk_stats(store_path: Path) -> dict[str, Optional[int]]: @@ current_path = store_path while True: try: - return dict(zip(("total", "used", "free"), shutil.disk_usage(current_path))) - except FileNotFoundError: + total, used, free = shutil.disk_usage(current_path) + return {"total": total, "used": used, "free": free} + except OSError: if current_path == current_path.parent: # Reached the root of the filesystem. - return dict(zip(("total", "used", "free"), (None, None, None))) + return {"total": None, "used": None, "free": None} current_path = current_path.parent#!/bin/bash # Verify current exception handling + test coverage for store_disk_stats. rg -n -C3 'def store_disk_stats|shutil\.disk_usage|except FileNotFoundError|except OSError|TestStoreDiskStats|PermissionError' \ ramalama/common.py test/unit/test_common.py # Verify exception hierarchy assumption used by the recommendation. python - <<'PY' print(issubclass(PermissionError, OSError)) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ramalama/common.py` around lines 805 - 825, The function store_disk_stats currently only catches FileNotFoundError when calling shutil.disk_usage(current_path); broaden the except to OSError to also cover PermissionError and similar OS-level failures, and simplify the return so you construct the dict directly (e.g., assign total, used, free = shutil.disk_usage(current_path) and return {"total": total, "used": used, "free": free} or return {"total": None, "used": None, "free": None} when at filesystem root) while preserving the upward traversal using current_path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ramalama/common.py`:
- Around line 805-825: The function store_disk_stats currently only catches
FileNotFoundError when calling shutil.disk_usage(current_path); broaden the
except to OSError to also cover PermissionError and similar OS-level failures,
and simplify the return so you construct the dict directly (e.g., assign total,
used, free = shutil.disk_usage(current_path) and return {"total": total, "used":
used, "free": free} or return {"total": None, "used": None, "free": None} when
at filesystem root) while preserving the upward traversal using current_path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9b4554f3-c390-41ee-9dc4-443f8fb867bb
📒 Files selected for processing (3)
ramalama/cli.pyramalama/common.pytest/unit/test_common.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ramalama/cli.py
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Oliver Walsh <owalsh@redhat.com>
olliewalsh
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ramalama/common.py (1)
805-826: Tighten the return type to match actual behavior.
store_disk_statsnever returnsNone; it always returns a dict (with nullable values on failure). Keeping| Nonein the signature makes callers handle an impossible case.Proposed type-signature fix
-def store_disk_stats(store_path: Path) -> dict[str, Optional[int]] | None: +def store_disk_stats(store_path: Path) -> dict[str, int | None]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ramalama/common.py` around lines 805 - 826, The function store_disk_stats always returns a dict with possibly-None values rather than None itself, so tighten the type signature by removing the `| None` union and use `dict[str, Optional[int]]` as the return type; update the function signature for store_disk_stats and adjust its docstring if necessary to state it returns a dict with nullable values on failure so callers no longer need to handle a None return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unit/test_common.py`:
- Around line 691-710: Tests in TestStoreDiskStats are using a hardcoded temp
path Path("/tmp/store") which triggers Ruff S108; replace those occurrences with
a synthetic non-filesystem path constant (e.g., TEST_STORE_PATH or a
Path("fake_store") defined in the test module) to avoid using real /tmp. Update
the four test methods (test_store_disk_stats,
test_store_disk_stats_no_store_dir, test_store_disk_stats_root_not_found,
test_store_disk_stats_permission_error) to reference the new constant instead of
Path("/tmp/store") and ensure imports/reference use pathlib.Path where needed so
assertions against store_disk_stats(Path(...)) remain unchanged.
---
Nitpick comments:
In `@ramalama/common.py`:
- Around line 805-826: The function store_disk_stats always returns a dict with
possibly-None values rather than None itself, so tighten the type signature by
removing the `| None` union and use `dict[str, Optional[int]]` as the return
type; update the function signature for store_disk_stats and adjust its
docstring if necessary to state it returns a dict with nullable values on
failure so callers no longer need to handle a None return.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b549344c-942e-4feb-af25-e90d9c213a9e
📒 Files selected for processing (3)
ramalama/cli.pyramalama/common.pytest/unit/test_common.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ramalama/cli.py
Comment on lines
+691
to
+710
| class TestStoreDiskStats: | ||
| @patch("ramalama.common.shutil.disk_usage") | ||
| def test_store_disk_stats(self, mock_disk_usage): | ||
| mock_disk_usage.return_value = (1000, 200, 800) | ||
| assert store_disk_stats(Path("/tmp/store")) == {"total": 1000, "used": 200, "free": 800} | ||
|
|
||
| @patch("ramalama.common.shutil.disk_usage") | ||
| def test_store_disk_stats_no_store_dir(self, mock_disk_usage): | ||
| mock_disk_usage.side_effect = [FileNotFoundError, FileNotFoundError, (1000, 200, 800)] | ||
| assert store_disk_stats(Path("/tmp/store")) == {"total": 1000, "used": 200, "free": 800} | ||
|
|
||
| @patch("ramalama.common.shutil.disk_usage") | ||
| def test_store_disk_stats_root_not_found(self, mock_disk_usage): | ||
| mock_disk_usage.side_effect = [FileNotFoundError, FileNotFoundError, FileNotFoundError] | ||
| assert store_disk_stats(Path("/tmp/store")) == {"total": None, "used": None, "free": None} | ||
|
|
||
| @patch("ramalama.common.shutil.disk_usage") | ||
| def test_store_disk_stats_permission_error(self, mock_disk_usage): | ||
| mock_disk_usage.side_effect = [PermissionError, (1000, 200, 800)] | ||
| assert store_disk_stats(Path("/tmp/store")) == {"total": 1000, "used": 200, "free": 800} |
There was a problem hiding this comment.
Replace hardcoded /tmp test path to unblock Ruff S108.
Lines 695, 700, 705, and 710 use Path("/tmp/store"), which triggers S108 and can block CI. Use a non-temp synthetic path constant instead.
Proposed fix
class TestStoreDiskStats:
+ TEST_STORE_PATH = Path("/ramalama/store")
+
`@patch`("ramalama.common.shutil.disk_usage")
def test_store_disk_stats(self, mock_disk_usage):
mock_disk_usage.return_value = (1000, 200, 800)
- assert store_disk_stats(Path("/tmp/store")) == {"total": 1000, "used": 200, "free": 800}
+ assert store_disk_stats(self.TEST_STORE_PATH) == {"total": 1000, "used": 200, "free": 800}
`@patch`("ramalama.common.shutil.disk_usage")
def test_store_disk_stats_no_store_dir(self, mock_disk_usage):
mock_disk_usage.side_effect = [FileNotFoundError, FileNotFoundError, (1000, 200, 800)]
- assert store_disk_stats(Path("/tmp/store")) == {"total": 1000, "used": 200, "free": 800}
+ assert store_disk_stats(self.TEST_STORE_PATH) == {"total": 1000, "used": 200, "free": 800}
`@patch`("ramalama.common.shutil.disk_usage")
def test_store_disk_stats_root_not_found(self, mock_disk_usage):
mock_disk_usage.side_effect = [FileNotFoundError, FileNotFoundError, FileNotFoundError]
- assert store_disk_stats(Path("/tmp/store")) == {"total": None, "used": None, "free": None}
+ assert store_disk_stats(self.TEST_STORE_PATH) == {"total": None, "used": None, "free": None}
`@patch`("ramalama.common.shutil.disk_usage")
def test_store_disk_stats_permission_error(self, mock_disk_usage):
mock_disk_usage.side_effect = [PermissionError, (1000, 200, 800)]
- assert store_disk_stats(Path("/tmp/store")) == {"total": 1000, "used": 200, "free": 800}
+ assert store_disk_stats(self.TEST_STORE_PATH) == {"total": 1000, "used": 200, "free": 800}🧰 Tools
🪛 Ruff (0.15.9)
[error] 695-695: Probable insecure usage of temporary file or directory: "/tmp/store"
(S108)
[error] 700-700: Probable insecure usage of temporary file or directory: "/tmp/store"
(S108)
[error] 705-705: Probable insecure usage of temporary file or directory: "/tmp/store"
(S108)
[error] 710-710: Probable insecure usage of temporary file or directory: "/tmp/store"
(S108)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/test_common.py` around lines 691 - 710, Tests in TestStoreDiskStats
are using a hardcoded temp path Path("/tmp/store") which triggers Ruff S108;
replace those occurrences with a synthetic non-filesystem path constant (e.g.,
TEST_STORE_PATH or a Path("fake_store") defined in the test module) to avoid
using real /tmp. Update the four test methods (test_store_disk_stats,
test_store_disk_stats_no_store_dir, test_store_disk_stats_root_not_found,
test_store_disk_stats_permission_error) to reference the new constant instead of
Path("/tmp/store") and ensure imports/reference use pathlib.Path where needed so
assertions against store_disk_stats(Path(...)) remain unchanged.
Collaborator
Author
There was a problem hiding this comment.
it's a unit-test, not an issue
There was a problem hiding this comment.
@olliewalsh, you're right — since shutil.disk_usage is fully mocked, the path Path("/tmp/store") is never accessed on the real filesystem, making this a false positive for S108. I'll disregard this flag in unit test contexts going forward.
✏️ Learnings added
Learnt from: olliewalsh
Repo: containers/ramalama PR: 2319
File: test/unit/test_common.py:691-710
Timestamp: 2026-04-10T13:20:26.718Z
Learning: In `test/unit/test_common.py`, Ruff S108 warnings on `/tmp/store` paths inside `TestStoreDiskStats` are false positives. The path is only passed to a fully mocked `shutil.disk_usage` and never touches the real filesystem, so S108 does not apply in this unit test context.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add reporting of disk usage statistics for the store path to the CLI info output.
New Features:
Tests: