Conversation
…tmanager#500 Resolves snapshotmanager#500. SnapmExistsError raise sites composed their own message strings, making the error format inconsistent across the codebase. Add a structured name parameter so all raise sites produce a uniform message and callers can programmatically inspect which snapshot set already existed without parsing the message string.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe change adds parameterized, resource-specific existence exceptions and updates manager duplicate checks to raise the matching exception for schedules, snapshot sets, boot entries, and revert entries. ChangesSnapshot existence error
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change is localized to structured duplicate-resource errors and does not introduce an actionable merge-blocking risk; it is merge-ready after normal checks and review. 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
Full details: No-Hardcoded-SecretsExplanation No hardcoded secret was introduced. The net PR diff changes only Full details: No-Weak-CryptoExplanation No weak-crypto issue was introduced. The PR changes only exception classes, exports, imports, and duplicate-resource raise sites in Full details: No-Injection-VectorsExplanation PASS — no injection vector was introduced. The PR only adds exception classes and changes raise sites. Added-line analysis found no SQL, shell=True, eval, exec, pickle.loads, yaml.load, or os.system. The new Full details: No-Sensitive-Data-In-LogsExplanation No sensitive-data logging was introduced. The pull request changes exception classes and four manager raise sites only; the diff adds no logger or print calls. The new messages contain validated snapshot-set or schedule names, not passwords, tokens, API keys, PII, session IDs, hostnames, or customer records. The duplicate-resource paths already logged equivalent names through the existing CLI error handler, so the pull request does not create a new exposure. Security risk: none identified.
✨ 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@snapm/snapm_exists_error.py`:
- Line 35: Update the __init__ method’s msg parameter annotation from str to
Optional[str], preserving its None default and existing constructor behavior.
- Around line 35-37: Update the SnapmExistsError call in the manager flow around
the existing caller to pass the raw snapshot name as the constructor’s name
argument, rather than a preformatted complete message. Preserve the structured
self.name contract and let SnapmExistsError.__init__ generate the default
message.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 239d1d63-51b6-48fa-afe0-619043cc1bb6
📒 Files selected for processing (1)
snapm/snapm_exists_error.py
|
|
||
| _default_template = "Snapshot set {name!r} already exists" | ||
|
|
||
| def __init__(self, name: str, msg: str = None) -> None: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ruff check snapm/snapm_exists_error.py --select RUF013Repository: snapshotmanager/snapm
Length of output: 872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,80p' snapm/snapm_exists_error.pyRepository: snapshotmanager/snapm
Length of output: 1379
Annotate the optional message as Optional[str].
msg can be None because it defaults to None and the constructor accepts omitted and custom messages. RUF013 flags msg: str = None; use Optional[str] so type checking accepts the documented API.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 35-35: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@snapm/snapm_exists_error.py` at line 35, Update the __init__ method’s msg
parameter annotation from str to Optional[str], preserving its None default and
existing constructor behavior.
Source: Linters/SAST tools
There was a problem hiding this comment.
This is valid: msg must have Optional[str] type since it can accept None.
There was a problem hiding this comment.
@bmr-cymru, confirmed. This is a low-severity type-correctness issue. The current annotation conflicts with the None default and can cause Ruff and static type-check failures. Update msg to Optional[str] and import Optional from typing.
You are interacting with an AI system.
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
|
Fixed whitespace in summary. |
There was a problem hiding this comment.
Hi @0x5t4l1n ! Thanks for your interest in contributing to snapm!
This PR is a good start, and the changes you have made to SnapmExistsError are going in the right direction.
There's a bit more work needed here to make this functional & compliant with our project standards: I'm happy to help if you're interested in revising this PR to include these changes.
First off: the patch is currently missing the DCO (Signed-off-by) and Assisted-by tags (from the summary it looks like you are using Claude Code). See the CONTRIBUTING.md.
As for the code, it looks good but so far your new exception class is not used (the patch just adds the new file, but does not import or use the new class anywhere). The new class also seems to be using Google style docstring formatting - the project uses Sphinx throughout. You can read more about this in the CONTRIBUTING.md (under "Coding Style"), or directly in the Sphinx Docs.
| """Structured SnapmExistsError raised when a snapshot set already exists.""" | ||
|
|
||
|
|
||
| class SnapmExistsError(Exception): |
There was a problem hiding this comment.
All exception classes in snapm should inherit from SnapmError (or one of its child classes), rather than bare Exception.
| @@ -0,0 +1,37 @@ | |||
| # Copyright Red Hat | |||
There was a problem hiding this comment.
I don't think there's currently a good justification for splitting this out into a separate file. If we did this, then we would do it for all the exception classes (e.g. creating snapm/_exceptions.py), and then import and re-export them in the base snapm package so that they still appear at the same import location for clients.
There was a problem hiding this comment.
Also, feel free to add your own copyright when making additions/changes: there's no need for you to assign copyright ownership to Red Hat when contributing.
| Raising with a custom message:: | ||
|
|
||
| raise SnapmExistsError("my-snapset", "my-snapset is taken; choose another name") | ||
| """ |
There was a problem hiding this comment.
Docstrings should use Sphinx notation: https://sphinx-rtd-tutorial.readthedocs.io/en/latest/docstrings.html
|
|
||
| _default_template = "Snapshot set {name!r} already exists" | ||
|
|
||
| def __init__(self, name: str, msg: str = None) -> None: |
There was a problem hiding this comment.
This is valid: msg must have Optional[str] type since it can accept None.
|
@0x5t4l1n any response to the review comments? |
… subclasses - SnapmExistsError now lives in snapm/_snapm.py (not a separate file) - Inherits from SnapmError instead of Exception - Adds SnapmSnapsetExistsError, SnapmScheduleExistsError, SnapmBootEntryExistsError, SnapmRevertEntryExistsError subclasses - Each subclass sets a resource-specific name attribute - Docstrings use Sphinx notation - All four Manager raise sites updated to use the appropriate subclass - New classes exported via __all__ Signed-off-by: Stalin <git@w4nn4d13.tech>
Replace all four SnapmExistsError raise sites with the appropriate subclass: SnapmSnapsetExistsError, SnapmScheduleExistsError, SnapmBootEntryExistsError, and SnapmRevertEntryExistsError. Signed-off-by: Stalin <git@w4nn4d13.tech>
bmr-cymru
left a comment
bmr-cymru
left a comment
There was a problem hiding this comment.
Thanks for updating the PR! This is looking better now - the only change I'd like to see (other than squashing the original commits) is to drop the msg argument from the subclasses.
|
|
||
| _default_template = "Snapshot set named {name!r} already exists" | ||
|
|
||
| def __init__(self, name: str, msg: Optional[str] = None) -> None: |
There was a problem hiding this comment.
I don't see a use for passing a custom msg here: let's simplify things and constrain the resource-specific subclasses to just reporting their own name with the provided template. I think this reduces the likelihood of misuse and confusing behaviour.
(Same comment for the other classes).
| @@ -1,37 +0,0 @@ | |||
| # Copyright Red Hat | |||
There was a problem hiding this comment.
Please squash/rebase to remove this from the history (rather than an incremental commit on the earlier changes).
2 participants
Summary
Fixes #500 — raise sites composed their own message strings, making the error format inconsistent and making it impossible for callers to programmatically inspect which snapshot set conflicted.
New file
snapm/snapm_exists_error.py:Test plan
SnapmExistsError("foo").name == "foo"str(SnapmExistsError("foo")) == "Snapshot set 'foo' already exists"SnapmExistsError("foo", "custom msg")Summary by CodeRabbit
Summary by CodeRabbit