snapm: lift timeline classification logic up to SnapshotSet
#885
Conversation
bmr-cymru
added
enhancement
WalkthroughManager now computes timeline categories for SnapshotSet objects and propagates them to SnapshotSet.categories; scheduler, reporting, docs and tests consume this new categories field. Categorisation is recomputed after discovery and on snapshot-set mutations and used by GC evaluation and outputs. Changes
Sequence Diagram(s)sequenceDiagram
participant Manager
participant SnapshotSet
participant Scheduler
participant Reporter
rect rgb(232,244,255)
Note over Manager,SnapshotSet: Recompute & propagate categories
Manager->>Manager: _categorize_snapshot_sets(sets)
Manager->>SnapshotSet: set_categories(list)
SnapshotSet-->>Manager: ack
end
rect rgb(236,255,232)
Note over Scheduler,SnapshotSet: GC evaluation uses per-set categories
Scheduler->>SnapshotSet: read categories / to_dict()
SnapshotSet-->>Scheduler: return categories
Scheduler->>Scheduler: compute per-category kept/deleted decisions
end
rect rgb(255,246,232)
Note over Reporter,SnapshotSet: Reporting includes categories
Reporter->>SnapshotSet: request to_dict()/categories
SnapshotSet-->>Reporter: include categories in output
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. Comment |
|
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. |
bmr-cymru
force-pushed
the
bmr-timeline-classify-refactor
branch
from
fbd804d to
2867656
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
snapm/_snapm.py (1)
1068-1072: Fix syntax error inSnapshotSet.__str__The Categories line contains a syntax error due to conflicting quotation marks within the f-string:
f"{SNAPSET_CATEGORIES}: {", ".join(self.categories)}\n"The f-string uses double quotes to delimit the outer string, but the inner expression
", "also uses double quotes, which terminates the f-string prematurely. This causes an immediate SyntaxError when the module is imported.Change the inner string to single quotes to resolve:
Fix
f"{SNAPSET_CATEGORIES}: {', '.join(self.categories)}\n"
🧹 Nitpick comments (5)
snapm/_snapm.py (1)
128-160: Categories API and timeline constants look coherentDefining
SNAPSET_CATEGORIESas the human‑readable key,SNAPSET_TIMELINE_CATEGORIESas the canonical list of timeline buckets, initialisingself._categoriesinSnapshotSet.__init__, and exposingcategories/set_categoriesplus theto_dict()and__all__updates all fit together cleanly for both CLI and JSON output.If you expect callers to provide arbitrary lists, you might later consider normalising or validating
set_categories()againstSNAPSET_TIMELINE_CATEGORIES, but it is not essential for this change set.Also applies to: 1037-1045, 1104-1105, 1300-1321, 2366-2385
doc/user_guide.rst (1)
379-387: Documentation updates for timeline categories align with implementationThe new examples and field descriptions for
Categoriesand the"categories"report field are consistent with the addedSnapshotSet.__str__,SnapshotSet.to_dict(), andsnapset listfield wiring.One minor follow‑up for later: now that
to_dict()includesCategories, thesnapset show --jsonexamples will emit a"Categories"key as well; you may want to refresh those JSON snippets when convenient so that they reflect current output.Based on learnings, the distinction between fixed
showJSON keys and prefixedlistJSON report fields is still clearly documented.Also applies to: 404-409, 631-632, 794-802, 815-816, 1294-1314, 1341-1342
container_tests/tests/test_schedule.py (2)
33-33: Consider using an instance attribute orClassVarannotation forcategories.Using a mutable default (
[]) as a class attribute can lead to shared state between instances if modified in-place. Although the tests reassign this per-instance, the static analyser flags this as a potential issue (RUF012). Consider initialising it in__init__or annotating withtyping.ClassVar[List[str]]to clarify intent.🔎 Suggested fix
+from typing import ClassVar, List + class MockSnapshotSet: boot_entry = None revert_entry = None - categories = [] + categories: ClassVar[List[str]] = []Alternatively, initialise in
__init__:def __init__(self, name, timestamp): self.name = name self.timestamp = timestamp self.deleted = False self.categories = []
391-394: Use_for the unusedcategoriesvariable.The
categoriesdict returned by_categorize_snapshot_setsis unpacked but never used in these tests. Using_as the variable name signals intentional discard and silences the linter warning (RUF059). This pattern repeats across multiple test methods.🔎 Suggested fix for all occurrences
- categories, categorized = _categorize_snapshot_sets(eighteen_months) + _, categorized = _categorize_snapshot_sets(eighteen_months)snapm/manager/_manager.py (1)
1917-1924: Property returns mutable internal state directly.The
snapshot_set_categoriesproperty returns references toself._categoriesandself._categorizeddirectly. If callers modify these dictionaries, it could corrupt internal state. Consider returning copies if this is a public API, or document that callers must not modify the returned values.If immutability is desired:
@property def snapshot_set_categories( self, ) -> Tuple[Dict[str, List[SnapshotSet]], Dict[int, List[str]]]: """ Return the current snapshot set categorization dictionaries. """ return (dict(self._categories), dict(self._categorized))Alternatively, if performance is a concern, document the contract that callers must treat the result as read-only.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
container_tests/snapm.confcontainer_tests/tests/test_schedule.pydoc/user_guide.rstman/man8/snapm.8snapm/_snapm.pysnapm/command.pysnapm/manager/_manager.pysnapm/manager/_schedule.py
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
📚 Learning: 2025-09-09T16:06:19.848Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-09T16:06:19.848Z
Learning: In snapm CLI, `show` commands produce JSON with individual objects using fixed field names that match regular show command output, while `list` commands produce JSON arrays with keys derived from report field names that can be controlled by the caller (fields, ordering, sorting). This architectural difference is intentional - show format is fixed for predictability, list format is flexible for customizable reporting.
Applied to files:
doc/user_guide.rstman/man8/snapm.8
📚 Learning: 2025-09-09T19:26:01.116Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 454
File: man/man8/snapm.8:1434-1462
Timestamp: 2025-09-09T19:26:01.116Z
Learning: The `list` command JSON format in snapm uses a wrapper object structure with a type-specific key (e.g., "Snapsets", "Snapshots") that maps to an array of objects, rather than a direct JSON array. This design provides flexibility for future mixed-object-type reports and follows patterns similar to lvm2's JSON reports. For example: `{"Snapsets": [{"name": "backup", ...}, {"name": "before-upgrade", ...}]}`.
Applied to files:
doc/user_guide.rstman/man8/snapm.8
📚 Learning: 2025-09-09T19:56:30.429Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 454
File: man/man8/snapm.8:1433-1461
Timestamp: 2025-09-09T19:56:30.429Z
Learning: In snapm JSON output, field names always use full prefixed names (e.g., "snapset_name", "snapset_status") to avoid collisions between different object types, even though prefixes may be optional in other contexts when unambiguous. This is an intentional architectural decision for consistency and conflict prevention in JSON reports.
Applied to files:
doc/user_guide.rstsnapm/command.pyman/man8/snapm.8snapm/_snapm.py
📚 Learning: 2025-12-17T16:23:50.413Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 759
File: systemd/tmpfiles.d/snapm.conf:9-9
Timestamp: 2025-12-17T16:23:50.413Z
Learning: In systemd/tmpfiles.d configuration for snapm: use uppercase 'Z' (recursive SELinux context restoration) for /var/cache/snapm because it is a persistent cache directory tree (including subdirectories like /var/cache/snapm/diffcache) that should have proper labeling applied recursively. Unlike /run/snapm/mounts which contains mounted snapshot filesystems, /var/cache/snapm stores cache files and needs recursive context application.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-10-22T19:32:44.315Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T19:32:44.315Z
Learning: In systemd/tmpfiles.d configuration for snapm: use lowercase 'z' (non-recursive SELinux context restoration) instead of uppercase 'Z' (recursive) for /run/snapm and its subdirectories. Recursive context application is hazardous because /run/snapm/mounts contains mounted snapshot filesystems that have their own SELinux contexts which should not be modified. If a user runs systemd-tmpfiles --create, recursive relabeling would incorrectly change contexts on mounted content.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-08-27T20:27:47.299Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-27T20:27:47.299Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-08-27T21:01:32.324Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-27T21:01:32.324Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-12-22T10:31:24.641Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 773
File: snapm.spec:148-149
Timestamp: 2025-12-22T10:31:24.641Z
Learning: In the snapm project's snapm.spec file, do not flag typos, formatting issues, or other problems in the %changelog section, as these entries are auto-generated from git commit messages and represent the historical record. Modifying them would require rewriting git history, which the project avoids.
Applied to files:
container_tests/snapm.confman/man8/snapm.8
📚 Learning: 2025-09-25T02:09:51.734Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T02:09:51.734Z
Learning: In the snapm codebase, SnapshotSet UUIDs are v5 UUIDs derived from NAMESPACE_SNAPSHOT_SET, the snapshot set name, and timestamp. They are mutable and change when the name changes during rename operations, making them unsuitable as stable lock keys.
Applied to files:
snapm/command.pyman/man8/snapm.8
📚 Learning: 2025-08-22T19:34:08.396Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-22T19:34:08.396Z
Learning: The snapshotmanager organization (including snapm and boom projects) uses "SHORTMONTH DAY YEAR" format for man page headers. This format has been used by boom since 2017. Organizational consistency within snapshotmanager is prioritized over cross-project consistency when it comes to formatting standards.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-08-24T16:09:15.238Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 400
File: man/man8/snapm.8:496-505
Timestamp: 2025-08-24T16:09:15.238Z
Learning: In UNIX man pages for snapm, metavars (value placeholders) should be lowercase except for UUID which should be uppercase. For example, use `schedule_name` not `SCHEDULE_NAME`, but use `UUID` not `uuid`.
Applied to files:
man/man8/snapm.8snapm/_snapm.py
📚 Learning: 2025-09-10T23:42:35.992Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 357
File: snapm.spec:132-134
Timestamp: 2025-09-10T23:42:35.992Z
Learning: In the snapm project, changelog entries in snapm.spec are auto-generated from git log using `git log --oneline v0.4.0..v0.5.0 | sed -e 's/^[a-f0-9]* /- /' -e 's/%//'` and should not be manually edited to avoid rewriting git history.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-11-12T21:27:50.414Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 596
File: snapm/_snapm.py:36-44
Timestamp: 2025-11-12T21:27:50.414Z
Learning: In the snapm codebase, the numeric values of SNAPM_DEBUG_* constants (e.g., SNAPM_DEBUG_MANAGER, SNAPM_DEBUG_MOUNTS, SNAPM_DEBUG_SCHEDULE) are not guaranteed to remain stable across versions. Consumers should always use the symbolic constant names rather than persisting numeric masks. SNAPM_DEBUG_ALL is designed to change over time as new subsystems are added, and repurposing or reassigning bit positions for debug flags is acceptable and intentional.
Applied to files:
snapm/_snapm.pysnapm/manager/_manager.py
📚 Learning: 2025-09-03T15:21:15.412Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
Applied to files:
container_tests/tests/test_schedule.pysnapm/manager/_schedule.pysnapm/manager/_manager.py
🧬 Code graph analysis (3)
snapm/command.py (2)
snapm/report.py (2)
FieldType(232-286)report_str_list(456-471)snapm/_snapm.py (1)
categories(1301-1309)
container_tests/tests/test_schedule.py (3)
snapm/manager/_manager.py (1)
_categorize_snapshot_sets(537-617)snapm/_snapm.py (4)
categories(1301-1309)snapshot_set(1737-1741)snapshot_set(1744-1748)datetime(1176-1180)snapm/manager/_schedule.py (6)
evaluate(102-114)evaluate(136-151)evaluate(178-198)evaluate(260-281)evaluate(324-438)evaluate(685-697)
snapm/manager/_schedule.py (1)
snapm/_snapm.py (3)
SnapshotSet(1006-1563)set_categories(1311-1320)categories(1301-1309)
🪛 GitHub Actions: Snapshot Manager DBus Tests
snapm/manager/_schedule.py
[error] 357-357: KeyError: 'yearly' when accessing classified_sets in GcPolicyParamsTimeline.evaluate. Ensure SNAPSET_CATEGORIES covers all retention categories or initialize classified_sets with those keys.
🪛 Ruff (0.14.10)
snapm/_snapm.py
1068-1068: Cannot reuse outer quote character in f-strings on Python 3.9 (syntax was added in Python 3.12)
(invalid-syntax)
container_tests/tests/test_schedule.py
33-33: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
392-392: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
407-407: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
444-444: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
475-475: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
511-511: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
561-561: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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). (31)
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (5)
container_tests/snapm.conf (1)
2-9: Runtime directory tmpfiles configuration looks correctThe directory creation and SELinux relabelling flags match the intended design: non‑recursive
zfor/run/snapm(and subdirectories) and recursiveZfor/var/cache/snapm. No issues from a scheduling/categories perspective.Based on learnings, this aligns with the existing tmpfiles.d guidance.
snapm/command.py (1)
49-49: Categories report field wiring looks soundImporting
SNAPSET_CATEGORIESfor the header and exposing a"categories"field backed byREP_STR_LISTandd.categoriesintegrates cleanly with the existing reporting/JSON machinery and matches the documented"snapset_categories"JSON field naming pattern.Based on learnings, the prefixed JSON field name remains consistent with other snapset report fields.
Also applies to: 323-331
man/man8/snapm.8 (1)
1748-1753: LGTM! Documentation forcategoriesfield is comprehensive.The new
categoriesfield is well-documented with a clear description of the timeline categories and a reference to thetimelinegarbage collection policy documentation for further details. The updates to example outputs throughout the man page are consistent.snapm/manager/_manager.py (2)
537-617: LGTM! The categorization algorithm is well-designed.The implementation correctly handles:
- Multi-category membership (e.g., Jan 1 Monday can be yearly, quarterly, monthly, weekly, daily, and hourly)
- Boundary tracking to ensure only the first snapshot set in each calendar interval is categorised
- The nested
get_boundary()function cleanly encapsulates boundary calculation logicThe algorithm is consistent with the documentation and test expectations.
1536-1558: Good optimisation with therecategorizeparameter.The addition of the
recategorizeparameter todelete_snapshot_set()enables efficient batch deletions viadelete_snapshot_sets()by deferring recategorization until all deletions are complete. This avoids O(n²) behaviour when deleting multiple snapshot sets.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
doc/user_guide.rstman/man8/snapm.8snapm/_snapm.py
🚧 Files skipped from review as they are similar to previous changes (1)
- doc/user_guide.rst
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
📚 Learning: 2025-08-24T16:09:15.238Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 400
File: man/man8/snapm.8:496-505
Timestamp: 2025-08-24T16:09:15.238Z
Learning: In UNIX man pages for snapm, metavars (value placeholders) should be lowercase except for UUID which should be uppercase. For example, use `schedule_name` not `SCHEDULE_NAME`, but use `UUID` not `uuid`.
Applied to files:
man/man8/snapm.8snapm/_snapm.py
📚 Learning: 2025-09-09T19:56:30.429Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 454
File: man/man8/snapm.8:1433-1461
Timestamp: 2025-09-09T19:56:30.429Z
Learning: In snapm JSON output, field names always use full prefixed names (e.g., "snapset_name", "snapset_status") to avoid collisions between different object types, even though prefixes may be optional in other contexts when unambiguous. This is an intentional architectural decision for consistency and conflict prevention in JSON reports.
Applied to files:
man/man8/snapm.8snapm/_snapm.py
📚 Learning: 2025-08-22T19:34:08.396Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-22T19:34:08.396Z
Learning: The snapshotmanager organization (including snapm and boom projects) uses "SHORTMONTH DAY YEAR" format for man page headers. This format has been used by boom since 2017. Organizational consistency within snapshotmanager is prioritized over cross-project consistency when it comes to formatting standards.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-09-09T16:06:19.848Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-09T16:06:19.848Z
Learning: In snapm CLI, `show` commands produce JSON with individual objects using fixed field names that match regular show command output, while `list` commands produce JSON arrays with keys derived from report field names that can be controlled by the caller (fields, ordering, sorting). This architectural difference is intentional - show format is fixed for predictability, list format is flexible for customizable reporting.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-09-10T23:42:35.992Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 357
File: snapm.spec:132-134
Timestamp: 2025-09-10T23:42:35.992Z
Learning: In the snapm project, changelog entries in snapm.spec are auto-generated from git log using `git log --oneline v0.4.0..v0.5.0 | sed -e 's/^[a-f0-9]* /- /' -e 's/%//'` and should not be manually edited to avoid rewriting git history.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-09-09T19:26:01.116Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 454
File: man/man8/snapm.8:1434-1462
Timestamp: 2025-09-09T19:26:01.116Z
Learning: The `list` command JSON format in snapm uses a wrapper object structure with a type-specific key (e.g., "Snapsets", "Snapshots") that maps to an array of objects, rather than a direct JSON array. This design provides flexibility for future mixed-object-type reports and follows patterns similar to lvm2's JSON reports. For example: `{"Snapsets": [{"name": "backup", ...}, {"name": "before-upgrade", ...}]}`.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-12-22T10:31:24.641Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 773
File: snapm.spec:148-149
Timestamp: 2025-12-22T10:31:24.641Z
Learning: In the snapm project's snapm.spec file, do not flag typos, formatting issues, or other problems in the %changelog section, as these entries are auto-generated from git commit messages and represent the historical record. Modifying them would require rewriting git history, which the project avoids.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-09-25T02:09:51.734Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T02:09:51.734Z
Learning: In the snapm codebase, SnapshotSet UUIDs are v5 UUIDs derived from NAMESPACE_SNAPSHOT_SET, the snapshot set name, and timestamp. They are mutable and change when the name changes during rename operations, making them unsuitable as stable lock keys.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-11-12T21:27:50.414Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 596
File: snapm/_snapm.py:36-44
Timestamp: 2025-11-12T21:27:50.414Z
Learning: In the snapm codebase, the numeric values of SNAPM_DEBUG_* constants (e.g., SNAPM_DEBUG_MANAGER, SNAPM_DEBUG_MOUNTS, SNAPM_DEBUG_SCHEDULE) are not guaranteed to remain stable across versions. Consumers should always use the symbolic constant names rather than persisting numeric masks. SNAPM_DEBUG_ALL is designed to change over time as new subsystems are added, and repurposing or reassigning bit positions for debug flags is acceptable and intentional.
Applied to files:
snapm/_snapm.py
⏰ 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). (37)
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (11)
snapm/_snapm.py (6)
11-11: LGTM!The addition of
Listto the imports is appropriate for the new type hints on thecategoriesproperty andset_categoriesmethod.
128-128: LGTM!The new constants
SNAPSET_CATEGORIESandSNAPSET_TIMELINE_CATEGORIESare well-defined and appropriately named. The timeline categories list follows a logical ordering from coarsest (yearly) to finest (hourly) granularity.Also applies to: 152-160
1037-1037: LGTM!The initialization of
_categoriesto an empty list is correct and follows the established pattern for other internal fields in the class.
1068-1068: LGTM!The integration of categories into the string representation is consistent with other list fields in the class, using
', '.join()to format the output.
1104-1104: LGTM!The addition of categories to the dictionary representation is correct and consistent with the existing pattern for list fields.
2366-2366: LGTM!The new constants are correctly exported in
__all__, making them part of the public API as intended.Also applies to: 2384-2384
man/man8/snapm.8 (5)
1002-1002: LGTM!The example outputs consistently include the new Categories field with realistic values. The formatting and positioning are consistent across all examples in the man page.
Also applies to: 1024-1024, 1049-1049, 1994-1994, 2011-2011, 2083-2083
1016-1028: LGTM!The extended example properly demonstrates the Categories field within the complete command output, following man page formatting conventions.
1661-1679: LGTM!The help text properly documents the new categories field with appropriate type annotation
[strlist]and description. The lowercase field name follows the established convention for report fields, while the constant name uses title case.Based on learnings, this aligns with snapm's field naming conventions.
1748-1753: LGTM!The field documentation clearly explains the categories field, lists all possible values, and appropriately references the timeline garbage collection policy for additional context.
1920-1925: LGTM!The JSON output example correctly represents Categories as an array with the proper key name. This follows the established pattern where show commands use fixed field names matching the constant names.
Based on learnings, this aligns with snapm's JSON output conventions.
bmr-cymru
force-pushed
the
bmr-timeline-classify-refactor
branch
from
2867656 to
f4c981d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
man/man8/snapm.8 (1)
2024-2050: Consider updatingcreate --jsonexample to includeCategories
SnapshotSet.json()is now used by bothsnapset create --jsonandsnapset show --json, so thecreateexample here will actually include a"Categories"key at runtime. It would be less confusing to extend this example with the sameCategoriesarray shown in theshow --jsonexample above.
🧹 Nitpick comments (5)
snapm/manager/_schedule.py (1)
343-433: Timeline GC logic now matches multi‑category semantics, but unclassified sets are always deletedThe new
GcPolicyParamsTimeline.evaluate()correctly:
- Builds
classified_setskeyed bySNAPSET_TIMELINE_CATEGORIES.- Uses each
SnapshotSet.categoriesto populate those lists.- Computes per‑category “kept” tail sets based on the configured
keep_*values.- Deletes a set only when none of its categories wants to keep it, which matches the documented “ANY category keeps it” rule.
One behaviour worth sanity‑checking is:
sset_cats = snapshot_categories.get(id(sset), []) if not sset_cats: to_delete.append(sset) continueAny snapshot set with an empty
categorieslist (e.g. if categorisation has not been run or fails) will always be selected for deletion under TIMELINE, which is a fairly aggressive fail mode.If you want the system to fail “safe”, you might instead:
- Log a warning when a set has no categories under a TIMELINE policy, and
- Treat such sets as retained by default (or fall back to a single “hourly” category).
Possible defensive tweak
- for sset in sets: - sset_cats = snapshot_categories.get(id(sset), []) - - # If snapshot has no categories, delete it - if not sset_cats: - to_delete.append(sset) - continue + for sset in sets: + sset_cats = snapshot_categories.get(id(sset), []) + + # If snapshot has no categories under a TIMELINE policy, + # treat it as uncategorised and keep it by default. + if not sset_cats: + _log_debug_schedule( + "Snapshot set %s has no timeline categories; " + "retaining under TIMELINE GC policy", + sset.name, + ) + continueIf you intentionally want “no categories ⇒ always delete” that’s fine, but it is worth confirming this matches your upgrade and failure‑mode expectations.
snapm/_snapm.py (1)
1300-1322: Consider makingcategoriesharder to mutate from arbitrary callersAt the moment
categoriesreturns the underlying list andset_categories()blindly assigns whatever it is given. Given the docstring note that this is intended to be managed only byManager, you might want to:
- Store a shallow copy in
set_categories()(to prevent accidental aliasing), and/or- Return a
tupleor a copy from thecategoriesproperty instead of the original list.For example:
- def set_categories(self, categories: List[str]): + def set_categories(self, categories: List[str]): @@ - self._categories = categories + self._categories = list(categories)Not essential, but it would make misuse by external code less likely.
container_tests/tests/test_schedule.py (1)
21-36: TIMELINE GC tests are realistic; tidy upMockSnapshotSet.categoriesand unused localsThe new tests correctly:
- Import and use
_categorize_snapshot_sets(...)to assign per‑set categories before callingGcPolicyParamsTimeline.evaluate().- Exercise important edge cases (first snapshot bug, Jan 1 Monday, multiple weekly snapshots, progressive hourly retention).
Two small clean‑ups would make the test module friendlier to linters:
- Avoid mutable class attribute for
categories
MockSnapshotSet.categories = []is a mutable class attribute (flagged by Ruff RUF012). Since you always overwrite it per instance, it is simpler to make it an instance attribute:Suggested tweak
-class MockSnapshotSet: - - boot_entry = None - revert_entry = None - categories = [] - - def __init__(self, name, timestamp): - self.name = name - self.timestamp = timestamp - self.deleted = False +class MockSnapshotSet: + + boot_entry = None + revert_entry = None + + def __init__(self, name, timestamp): + self.name = name + self.timestamp = timestamp + self.deleted = False + self.categories = []
- Silence unused
categoriesunpack variablesIn several places you unpack
_categorize_snapshot_sets(...)intocategories, categorizedbut never usecategories(RUF059). Renaming to_categoriesor_keeps intent clear and satisfies linters, e.g.:Suggested tweak
- categories, categorized = _categorize_snapshot_sets(eighteen_months) + _categories, categorized = _categorize_snapshot_sets(eighteen_months) @@ - categories, categorized = _categorize_snapshot_sets([snapshot_set]) + _categories, categorized = _categorize_snapshot_sets([snapshot_set]) @@ - categories, categorized = _categorize_snapshot_sets([snapshot_set]) + _categories, categorized = _categorize_snapshot_sets([snapshot_set]) @@ - categories, categorized = _categorize_snapshot_sets(snapshot_sets) + _categories, categorized = _categorize_snapshot_sets(snapshot_sets) @@ - categories, categorized = _categorize_snapshot_sets(snapshot_sets) + _categories, categorized = _categorize_snapshot_sets(snapshot_sets)Functionally everything looks sound once these stylistic issues are addressed.
Also applies to: 386-416, 417-523, 525-565
snapm/manager/_manager.py (2)
1533-1533: Consider whether recategorization is needed after rename.Renaming a snapshot set doesn't change its timestamp, so its timeline categories shouldn't change. However, calling recategorization here ensures consistency and is a reasonable defensive approach.
1156-1217: Consider adding recategorization todiscover_snapshot_sets.The
discover_snapshot_setsmethod is public and rebuilds the entire snapshot set list. If called externally (e.g., to refresh after external changes), the timeline categories won't be updated automatically. Consider callingself._recategorize_snapshot_sets()at the end of this method to maintain consistency.🔎 Proposed fix
self.snapshot_sets.sort(key=lambda ss: ss.name) _log_debug("Discovered %d snapshot sets", len(self.snapshot_sets)) + self._recategorize_snapshot_sets()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
container_tests/snapm.confcontainer_tests/tests/test_schedule.pydoc/user_guide.rstman/man8/snapm.8snapm/_snapm.pysnapm/command.pysnapm/manager/_manager.pysnapm/manager/_schedule.py
🚧 Files skipped from review as they are similar to previous changes (1)
- snapm/command.py
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
📚 Learning: 2025-09-09T16:06:19.848Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-09T16:06:19.848Z
Learning: In snapm CLI, `show` commands produce JSON with individual objects using fixed field names that match regular show command output, while `list` commands produce JSON arrays with keys derived from report field names that can be controlled by the caller (fields, ordering, sorting). This architectural difference is intentional - show format is fixed for predictability, list format is flexible for customizable reporting.
Applied to files:
doc/user_guide.rstman/man8/snapm.8
📚 Learning: 2025-09-09T19:26:01.116Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 454
File: man/man8/snapm.8:1434-1462
Timestamp: 2025-09-09T19:26:01.116Z
Learning: The `list` command JSON format in snapm uses a wrapper object structure with a type-specific key (e.g., "Snapsets", "Snapshots") that maps to an array of objects, rather than a direct JSON array. This design provides flexibility for future mixed-object-type reports and follows patterns similar to lvm2's JSON reports. For example: `{"Snapsets": [{"name": "backup", ...}, {"name": "before-upgrade", ...}]}`.
Applied to files:
doc/user_guide.rstman/man8/snapm.8
📚 Learning: 2025-09-09T19:56:30.429Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 454
File: man/man8/snapm.8:1433-1461
Timestamp: 2025-09-09T19:56:30.429Z
Learning: In snapm JSON output, field names always use full prefixed names (e.g., "snapset_name", "snapset_status") to avoid collisions between different object types, even though prefixes may be optional in other contexts when unambiguous. This is an intentional architectural decision for consistency and conflict prevention in JSON reports.
Applied to files:
doc/user_guide.rstman/man8/snapm.8snapm/_snapm.py
📚 Learning: 2025-09-03T15:21:15.412Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
Applied to files:
snapm/manager/_schedule.pysnapm/manager/_manager.pycontainer_tests/tests/test_schedule.py
📚 Learning: 2025-08-24T16:09:15.238Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 400
File: man/man8/snapm.8:496-505
Timestamp: 2025-08-24T16:09:15.238Z
Learning: In UNIX man pages for snapm, metavars (value placeholders) should be lowercase except for UUID which should be uppercase. For example, use `schedule_name` not `SCHEDULE_NAME`, but use `UUID` not `uuid`.
Applied to files:
man/man8/snapm.8snapm/_snapm.py
📚 Learning: 2025-08-22T19:34:08.396Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-22T19:34:08.396Z
Learning: The snapshotmanager organization (including snapm and boom projects) uses "SHORTMONTH DAY YEAR" format for man page headers. This format has been used by boom since 2017. Organizational consistency within snapshotmanager is prioritized over cross-project consistency when it comes to formatting standards.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-09-10T23:42:35.992Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 357
File: snapm.spec:132-134
Timestamp: 2025-09-10T23:42:35.992Z
Learning: In the snapm project, changelog entries in snapm.spec are auto-generated from git log using `git log --oneline v0.4.0..v0.5.0 | sed -e 's/^[a-f0-9]* /- /' -e 's/%//'` and should not be manually edited to avoid rewriting git history.
Applied to files:
man/man8/snapm.8container_tests/snapm.conf
📚 Learning: 2025-12-22T10:31:24.641Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 773
File: snapm.spec:148-149
Timestamp: 2025-12-22T10:31:24.641Z
Learning: In the snapm project's snapm.spec file, do not flag typos, formatting issues, or other problems in the %changelog section, as these entries are auto-generated from git commit messages and represent the historical record. Modifying them would require rewriting git history, which the project avoids.
Applied to files:
man/man8/snapm.8snapm/_snapm.pycontainer_tests/snapm.conf
📚 Learning: 2025-09-25T02:09:51.734Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T02:09:51.734Z
Learning: In the snapm codebase, SnapshotSet UUIDs are v5 UUIDs derived from NAMESPACE_SNAPSHOT_SET, the snapshot set name, and timestamp. They are mutable and change when the name changes during rename operations, making them unsuitable as stable lock keys.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-12-02T16:08:02.588Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-02T16:08:02.588Z
Learning: In the snapshotmanager/snapm codebase, `FileTypeInfo` in `snapm/_fsdiff/filetypes.py` is not suitable for conversion to `dataclass` because it requires custom `__init__` logic to compute `is_text_like` from the `category` parameter, and has a custom `__str__` method that formats field names as human-readable strings (e.g., "MIME type:"). The project uses `dataclass` extensively for classes like `SnapmConfig` and `GcPolicyParams*`, but only where it doesn't interfere with custom initialization or formatting requirements.
Applied to files:
snapm/_snapm.py
📚 Learning: 2025-11-12T21:27:50.414Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 596
File: snapm/_snapm.py:36-44
Timestamp: 2025-11-12T21:27:50.414Z
Learning: In the snapm codebase, the numeric values of SNAPM_DEBUG_* constants (e.g., SNAPM_DEBUG_MANAGER, SNAPM_DEBUG_MOUNTS, SNAPM_DEBUG_SCHEDULE) are not guaranteed to remain stable across versions. Consumers should always use the symbolic constant names rather than persisting numeric masks. SNAPM_DEBUG_ALL is designed to change over time as new subsystems are added, and repurposing or reassigning bit positions for debug flags is acceptable and intentional.
Applied to files:
snapm/_snapm.pysnapm/manager/_manager.py
📚 Learning: 2025-12-17T16:23:50.413Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 759
File: systemd/tmpfiles.d/snapm.conf:9-9
Timestamp: 2025-12-17T16:23:50.413Z
Learning: In systemd/tmpfiles.d configuration for snapm: use uppercase 'Z' (recursive SELinux context restoration) for /var/cache/snapm because it is a persistent cache directory tree (including subdirectories like /var/cache/snapm/diffcache) that should have proper labeling applied recursively. Unlike /run/snapm/mounts which contains mounted snapshot filesystems, /var/cache/snapm stores cache files and needs recursive context application.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-10-22T19:32:44.315Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T19:32:44.315Z
Learning: In systemd/tmpfiles.d configuration for snapm: use lowercase 'z' (non-recursive SELinux context restoration) instead of uppercase 'Z' (recursive) for /run/snapm and its subdirectories. Recursive context application is hazardous because /run/snapm/mounts contains mounted snapshot filesystems that have their own SELinux contexts which should not be modified. If a user runs systemd-tmpfiles --create, recursive relabeling would incorrectly change contexts on mounted content.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-08-27T20:27:47.299Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-27T20:27:47.299Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-08-27T21:01:32.324Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-27T21:01:32.324Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
Applied to files:
container_tests/snapm.conf
🧬 Code graph analysis (3)
snapm/manager/_schedule.py (1)
snapm/_snapm.py (5)
SnapshotSet(1006-1565)snapshot_set(1739-1743)snapshot_set(1746-1750)set_categories(1311-1322)categories(1301-1309)
snapm/manager/_manager.py (1)
snapm/_snapm.py (8)
datetime(1176-1180)SnapshotSet(1006-1565)categories(1301-1309)set_categories(1311-1322)name(1140-1144)name(1659-1663)uuid(1162-1166)uuid(1666-1670)
container_tests/tests/test_schedule.py (2)
snapm/manager/_manager.py (1)
_categorize_snapshot_sets(537-617)snapm/_snapm.py (6)
categories(1301-1309)snapshot_set(1739-1743)snapshot_set(1746-1750)datetime(1176-1180)timestamp(1169-1173)timestamp(1710-1714)
🪛 Ruff (0.14.10)
container_tests/tests/test_schedule.py
33-33: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
407-407: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
444-444: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
475-475: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
511-511: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
561-561: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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). (34)
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (15)
container_tests/snapm.conf (1)
1-9: Runtime directory and SELinux relabel rules look correctThe directory creation and
z/Zflags for/run/snapm*vs/var/cache/snapmmatch the documented tmpfiles/SELinux expectations for this project (non‑recursive on/run, recursive on persistent cache). No issues spotted.man/man8/snapm.8 (4)
994-1006: Snapset textual examples now correctly surface timeline categoriesThe added
Categories:lines in the varioussnapset create/snapset showexamples are consistent with the newSnapshotSet.__str__output and help demonstrate how timeline classifications appear in normal CLI usage.Also applies to: 1016-1028, 1042-1053, 1987-2015, 2076-2087
1658-1680: Snapshot set field list aligns with reporting implementationThe updated “Snapshot set Fields” section (including
uuid,mounted,origin_mounted,mount_root, andcategories) matches the report field names exposed by the code and clarifies thatcategoriesis a[strlist]timeline field. This should keepsnapset list -o helpoutput and the man page in sync.
1748-1753:categoriesREPORT FIELD description matches TIMELINE GC semanticsDescribing
categoriesas a list drawn from"yearly", "quarterly", "monthly", "weekly", "daily", "hourly"and explicitly pointing back to the TIMELINE policy provides the right context for users interpreting this field. No issues.
1900-1933:snapset show --jsonexample correctly includesCategoriesThe JSON
snapset showexample now includes aCategoriesarray alongside the existing fields, matchingSnapshotSet.to_dict()/json()and the documented property names. This keeps show‑JSON shape consistent with the text output.doc/user_guide.rst (1)
373-387: User guide coverage ofcategoriesis consistent with the CLI and APIThe added
Categories:lines in the snapset examples, the new-o+categories/compact list examples, the extendedsnapset list -ohelpfield list, and theshow --jsonCategoriesarray all align with theSnapshotSet.categoriesproperty and the reporting field namecategories. This should give users a clear picture of how timeline classifications appear in both tabular and JSON formats.Also applies to: 397-405, 625-634, 787-799, 809-817, 1300-1320, 1326-1348, 1392-1415
snapm/manager/_schedule.py (1)
23-29: UsingSNAPSET_TIMELINE_CATEGORIEShere avoids the earlier misclassification bugImporting and driving the TIMELINE GC logic from
SNAPSET_TIMELINE_CATEGORIESrather than a display constant ensures that internal maps are keyed by real timeline categories, preventing theKeyError 'yearly'and single‑character key issues seen previously.snapm/_snapm.py (1)
108-129: CoreSnapshotSetcategory plumbing and exports look coherentThe new
SNAPSET_CATEGORIESlabel,SNAPSET_TIMELINE_CATEGORIESlist,_categoriesfield, inclusion in__str__/to_dict()/json(), and thecategories/set_categories()API together provide a clean, central representation of timeline categories. Exporting both constants via__all__makes them available to scheduling, reporting, and documentation without leaking implementation details. This matches how the manager and schedule modules are now consuming categories.Also applies to: 152-160, 1029-1038, 1063-1072, 1104-1104, 1300-1322, 2347-2387
snapm/manager/_manager.py (7)
13-13: LGTM!The new imports are appropriate for the categorization functionality.
Also applies to: 22-22, 32-32
537-617: Well-implemented categorization algorithm.The multi-category timeline classification logic is correct and well-documented. The use of
seen_boundariesto ensure only the first snapshot after each boundary gets that category is the right approach.
989-992: LGTM!Initializing the categorization state during
__init__and calling recategorization after snapshot sets are discovered is the correct approach.
1144-1153: LGTM!The method correctly recomputes categories and propagates them to each
SnapshotSetinstance.
1454-1454: LGTM!Recategorization after creating a snapshot set ensures the new set is included in timeline categories.
1538-1538: Excellent optimization for bulk deletions.The
recategorizeparameter allowsdelete_snapshot_setsto defer recategorization until after all deletions complete, avoiding redundant O(n) recategorizations. The default value ofTruemaintains backward compatibility.Also applies to: 1557-1558, 1574-1574, 1576-1576
1867-1867: LGTM!Recategorization after splitting a snapshot set ensures both the original and new sets have correct timeline categories.
|
@coderabbitai: re:
It always did that: the categorization is exhaustive, and is always run before garbage collection occurs. If is an error if uncategorized snapshot sets remain at this point. Leaving them lying around forever with a warning message that nobody will see unless they are checking logs is suboptimal. From the pre-refactored # If snapshot has no categories, delete it
if not sset_cats:
to_delete.append(sset)
continueThe same comment is present in the current code. |
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru: Thank you for the clarification! You're absolutely right—I mischaracterized the behavior. The deletion of unclassified sets was already present in the original implementation and is intentional. I appreciate you pointing out that categorization is exhaustive and that uncategorized sets at GC time indicate an error condition. The rationale makes sense, especially since warning messages in logs wouldn't be actionable. 🧠 Learnings used |
bmr-cymru
force-pushed
the
bmr-timeline-classify-refactor
branch
from
f4c981d to
c5c3a52
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
container_tests/snapm.confcontainer_tests/tests/test_schedule.pydoc/user_guide.rstman/man8/snapm.8snapm/_snapm.pysnapm/command.pysnapm/manager/_manager.pysnapm/manager/_schedule.pytests/test_manager.py
🚧 Files skipped from review as they are similar to previous changes (3)
- snapm/command.py
- doc/user_guide.rst
- snapm/_snapm.py
🧰 Additional context used
🧠 Learnings (22)
📓 Common learnings
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
📚 Learning: 2025-11-13T18:16:10.229Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-11-13T18:16:10.229Z
Learning: In snapshotmanager/snapm test code, timestamps in tests like `get_eighteen_months()` intentionally include jitter (non-zero seconds/microseconds) to mimic the behavior of systemd timer units with the default `AccuracySec` value of 1 minute, providing more realistic test coverage for Timeline GC logic.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-08-30T13:46:15.300Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-30T13:46:15.300Z
Learning: In snapm, boom is a hard requirement because snapm.manager._boot imports boom modules directly, and the Manager class imports and uses functionality from _boot including BootCache, create_snapset_boot_entry, and delete_snapset_boot_entry. The Manager class cannot be instantiated without boom being available.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-10-22T16:43:42.894Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T16:43:42.894Z
Learning: In snapm/manager/_manager.py _check_snapm_dir: the correct order for directory validation is: 1) check if path exists and validate it's a directory (not a symlink), 2) os.makedirs() to create if needed, 3) os.chmod() to fix permissions if directory existed with wrong perms, 4) validate final mode matches expected. Do not attempt chmod after makedirs for a newly created directory since makedirs(mode=...) already sets permissions. Do not log errors when chmod fails but the mode is already correct.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-10-20T21:12:52.344Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:372-377
Timestamp: 2025-10-20T21:12:52.344Z
Learning: In snapm/manager/_mounts.py mount operations: do not create missing mount point directories on snapshot filesystems; instead, check if the directory exists with isdir(), log a warning if missing, and skip that mount to avoid modifying the snapshot filesystem unnecessarily.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-09-03T15:21:15.412Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
Applied to files:
snapm/manager/_schedule.pycontainer_tests/tests/test_schedule.pysnapm/manager/_manager.py
📚 Learning: 2025-10-22T20:06:54.993Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Applied to files:
snapm/manager/_schedule.pysnapm/manager/_manager.py
📚 Learning: 2025-09-10T23:42:35.992Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 357
File: snapm.spec:132-134
Timestamp: 2025-09-10T23:42:35.992Z
Learning: The snapm project maintainer bmr-cymru prefers scripted version bumping as part of the snapm-x.y.z-release-prep workflow rather than automated regex-based parsing for version synchronization across files.
Applied to files:
snapm/manager/_schedule.pysnapm/manager/_manager.py
📚 Learning: 2025-08-24T16:09:15.238Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 400
File: man/man8/snapm.8:496-505
Timestamp: 2025-08-24T16:09:15.238Z
Learning: In UNIX man pages for snapm, metavars (value placeholders) should be lowercase except for UUID which should be uppercase. For example, use `schedule_name` not `SCHEDULE_NAME`, but use `UUID` not `uuid`.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-09-09T19:56:30.429Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 454
File: man/man8/snapm.8:1433-1461
Timestamp: 2025-09-09T19:56:30.429Z
Learning: In snapm JSON output, field names always use full prefixed names (e.g., "snapset_name", "snapset_status") to avoid collisions between different object types, even though prefixes may be optional in other contexts when unambiguous. This is an intentional architectural decision for consistency and conflict prevention in JSON reports.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-08-22T19:34:08.396Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-22T19:34:08.396Z
Learning: The snapshotmanager organization (including snapm and boom projects) uses "SHORTMONTH DAY YEAR" format for man page headers. This format has been used by boom since 2017. Organizational consistency within snapshotmanager is prioritized over cross-project consistency when it comes to formatting standards.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-09-10T23:42:35.992Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 357
File: snapm.spec:132-134
Timestamp: 2025-09-10T23:42:35.992Z
Learning: In the snapm project, changelog entries in snapm.spec are auto-generated from git log using `git log --oneline v0.4.0..v0.5.0 | sed -e 's/^[a-f0-9]* /- /' -e 's/%//'` and should not be manually edited to avoid rewriting git history.
Applied to files:
man/man8/snapm.8container_tests/snapm.conf
📚 Learning: 2025-09-09T16:06:19.848Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-09T16:06:19.848Z
Learning: In snapm CLI, `show` commands produce JSON with individual objects using fixed field names that match regular show command output, while `list` commands produce JSON arrays with keys derived from report field names that can be controlled by the caller (fields, ordering, sorting). This architectural difference is intentional - show format is fixed for predictability, list format is flexible for customizable reporting.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-12-22T10:31:24.641Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 773
File: snapm.spec:148-149
Timestamp: 2025-12-22T10:31:24.641Z
Learning: In the snapm project's snapm.spec file, do not flag typos, formatting issues, or other problems in the %changelog section, as these entries are auto-generated from git commit messages and represent the historical record. Modifying them would require rewriting git history, which the project avoids.
Applied to files:
man/man8/snapm.8container_tests/snapm.conf
📚 Learning: 2025-09-09T19:26:01.116Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 454
File: man/man8/snapm.8:1434-1462
Timestamp: 2025-09-09T19:26:01.116Z
Learning: The `list` command JSON format in snapm uses a wrapper object structure with a type-specific key (e.g., "Snapsets", "Snapshots") that maps to an array of objects, rather than a direct JSON array. This design provides flexibility for future mixed-object-type reports and follows patterns similar to lvm2's JSON reports. For example: `{"Snapsets": [{"name": "backup", ...}, {"name": "before-upgrade", ...}]}`.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-09-25T02:09:51.734Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T02:09:51.734Z
Learning: In the snapm codebase, SnapshotSet UUIDs are v5 UUIDs derived from NAMESPACE_SNAPSHOT_SET, the snapshot set name, and timestamp. They are mutable and change when the name changes during rename operations, making them unsuitable as stable lock keys.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-11-12T21:27:50.414Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 596
File: snapm/_snapm.py:36-44
Timestamp: 2025-11-12T21:27:50.414Z
Learning: In the snapm codebase, the numeric values of SNAPM_DEBUG_* constants (e.g., SNAPM_DEBUG_MANAGER, SNAPM_DEBUG_MOUNTS, SNAPM_DEBUG_SCHEDULE) are not guaranteed to remain stable across versions. Consumers should always use the symbolic constant names rather than persisting numeric masks. SNAPM_DEBUG_ALL is designed to change over time as new subsystems are added, and repurposing or reassigning bit positions for debug flags is acceptable and intentional.
Applied to files:
snapm/manager/_manager.py
📚 Learning: 2025-12-30T14:19:44.950Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-30T14:19:44.950Z
Learning: In snapshotmanager/snapm, ConfigParser construction for plugins is intentionally centralized in exactly one place within Manager._find_and_verify_plugins(). This architectural decision ensures maintainability, and duplicating this pattern elsewhere would create technical debt.
Applied to files:
snapm/manager/_manager.py
📚 Learning: 2025-12-17T16:23:50.413Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 759
File: systemd/tmpfiles.d/snapm.conf:9-9
Timestamp: 2025-12-17T16:23:50.413Z
Learning: In systemd/tmpfiles.d configuration for snapm: use uppercase 'Z' (recursive SELinux context restoration) for /var/cache/snapm because it is a persistent cache directory tree (including subdirectories like /var/cache/snapm/diffcache) that should have proper labeling applied recursively. Unlike /run/snapm/mounts which contains mounted snapshot filesystems, /var/cache/snapm stores cache files and needs recursive context application.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-10-22T19:32:44.315Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T19:32:44.315Z
Learning: In systemd/tmpfiles.d configuration for snapm: use lowercase 'z' (non-recursive SELinux context restoration) instead of uppercase 'Z' (recursive) for /run/snapm and its subdirectories. Recursive context application is hazardous because /run/snapm/mounts contains mounted snapshot filesystems that have their own SELinux contexts which should not be modified. If a user runs systemd-tmpfiles --create, recursive relabeling would incorrectly change contexts on mounted content.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-08-27T20:27:47.299Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-27T20:27:47.299Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-08-27T21:01:32.324Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-27T21:01:32.324Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
Applied to files:
container_tests/snapm.conf
🧬 Code graph analysis (3)
tests/test_manager.py (1)
snapm/manager/_manager.py (6)
_find_mount_point_for_devpath(314-325)_check_snapm_runtime_dir(328-356)_categorize_snapshot_sets(537-617)create(707-757)disable(896-906)delete(839-852)
snapm/manager/_schedule.py (1)
snapm/_snapm.py (5)
SnapshotSet(1006-1565)snapshot_set(1739-1743)snapshot_set(1746-1750)set_categories(1311-1322)categories(1301-1309)
container_tests/tests/test_schedule.py (2)
snapm/manager/_manager.py (1)
_categorize_snapshot_sets(537-617)snapm/_snapm.py (6)
categories(1301-1309)snapshot_set(1739-1743)snapshot_set(1746-1750)datetime(1176-1180)timestamp(1169-1173)timestamp(1710-1714)
🪛 Ruff (0.14.10)
tests/test_manager.py
155-155: Unused method argument: mock_exists
(ARG002)
155-155: Unused method argument: mock_open
(ARG002)
container_tests/tests/test_schedule.py
33-33: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
407-407: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
444-444: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
475-475: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
511-511: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
561-561: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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). (37)
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
🔇 Additional comments (11)
container_tests/snapm.conf (1)
1-9: LGTM! tmpfiles.d configuration follows project conventions.The runtime directory configuration correctly uses:
- Lowercase 'z' (non-recursive SELinux) for
/run/snapmpaths containing mounted snapshots- Uppercase 'Z' (recursive SELinux) for
/var/cache/snapmcache directory tree- Secure permissions (0700, root:root)
Based on learnings, this aligns with the established pattern for snapm runtime directories.
man/man8/snapm.8 (2)
1661-1680: LGTM! Field documentation is comprehensive and accurate.The snapshot set fields help text properly documents the new
categoriesfield as timeline categories with type[strlist], consistent with the implementation insnapm/_snapm.py.
1748-1753: LGTM! REPORT FIELDS documentation is accurate.The categories field documentation correctly describes it as containing timeline categories (yearly, quarterly, monthly, weekly, daily, hourly) and appropriately references the timeline garbage collection policy for more details.
container_tests/tests/test_schedule.py (1)
391-394: LGTM! Categorization setup follows correct pattern.The test correctly:
- Calls
_categorize_snapshot_sets()to compute classifications- Propagates categories to each
MockSnapshotSetvia thecategorizedmapping- Proceeds with evaluation
This mirrors the production workflow where Manager computes categorization and propagates it to SnapshotSet instances before GC evaluation.
Note: The static analysis hint about unused
categoriesvariable is a false positive - tests intentionally only use thecategorizedmapping.tests/test_manager.py (4)
152-165: LGTM! Comprehensive test for mount point resolution.The test correctly:
- Mocks
/proc/mountsfile reading with realistic data- Uses
samefilemock to verify device matching logic- Tests both successful match and no-match scenarios
167-195: LGTM! Security validation test is thorough.The test properly validates the runtime directory security checks:
- Correct case: 0700 permissions, root:root ownership
- Error cases: wrong permissions (0777) and non-root ownership
This ensures the runtime directory security validation works as intended.
1148-1212: LGTM! Comprehensive categorization logic test.This test thoroughly validates the
_categorize_snapshot_setsfunction:
- Multi-level categorization (yearly, quarterly, monthly, weekly, daily, hourly)
- Edge cases like Jan 1 (Sunday - yearly/quarterly/monthly/daily/hourly but NOT weekly)
- Monday categorization (weekly + daily + hourly)
- Quarter boundary detection (Apr 1)
- Reverse mapping validation (categorized dict keyed by id())
The test data and assertions correctly verify the complex multi-category membership logic.
1214-1314: LGTM! Comprehensive Scheduler unit tests.The test class thoroughly covers:
- Schedule creation with proper setup (write_config, enable, start)
- Duplicate detection
- Edit operations (cleanup of old, setup of new)
- Delete operations with proper cleanup
- Lifecycle proxy methods (enable/disable/start/stop/gc)
Good use of mocks to isolate Scheduler logic from file I/O and timer management.
snapm/manager/_schedule.py (2)
343-355: LGTM! Timeline categorization correctly uses pre-computed categories.The refactored logic properly:
- Initializes
classified_setskeyed bySNAPSET_TIMELINE_CATEGORIES(avoiding the pastKeyError: 'yearly'issue)- Reads pre-computed categories from
snapshot_set.categories(set by Manager)- Populates
classified_setsonly for recognized timeline categoriesThis addresses the past review comment and correctly implements the lifted categorization approach where Manager computes and propagates categories to SnapshotSet instances.
417-431: LGTM! Multi-category retention logic is correct.The deletion decision correctly implements the policy:
- Snapshot sets with no categories are deleted
- Snapshot sets are KEPT if ANY category wants to keep them
- Snapshot sets are DELETED only if ALL applicable categories agree to delete
As noted in PR objectives comments, uncategorized snapshot sets have always been deleted by the GC logic (not new behaviour). The categorization is exhaustive and run before GC, so any uncategorized sets indicate an error.
snapm/manager/_manager.py (1)
1538-1576: Well-designed optimization for bulk deletion.The addition of the
recategorizeparameter todelete_snapshot_set()and its use indelete_snapshot_sets()is a good optimization. By passingrecategorize=Falseduring the loop (line 1574) and performing a single recategorization after all deletions (line 1576), you avoid redundant recategorization overhead when deleting multiple snapshot sets.
bmr-cymru
force-pushed
the
bmr-timeline-classify-refactor
branch
2 times, most recently
from
10ebf31 to
cced502
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
man/man8/snapm.8 (1)
1900-1955: JSON examples forCategoriesarrays are now syntactically validThe updated JSON snippets for
snapset show --jsonandsnapset create --jsonnow terminate the"Categories"arrays without a trailing comma after"hourly", so the examples are valid JSON and match the new categories output from the code.
🧹 Nitpick comments (9)
snapm/_snapm.py (1)
128-160: SnapshotSet timeline categories wiring looks sound; consider making the category list immutable.The introduction of
SNAPSET_CATEGORIES/SNAPSET_TIMELINE_CATEGORIES, the_categoriesslot, and thecategories/set_categoriesAPI integrates cleanly with__str__,to_dict, JSON, and the public export surface; this gives a clear, single source of truth for time‑based classifications.If you want to reduce the risk of accidental mutation of the category universe, you could make
SNAPSET_TIMELINE_CATEGORIESa tuple (or at least treat it as read‑only) while continuing to use plain lists for per‑snapsetcategories. This keeps the public behaviour identical but makes it harder for future refactors to clobber the global category set by mistake.Also applies to: 1037-1105, 1300-1323, 2368-2387
snapm/manager/_manager.py (1)
537-619: Consider avoiding in‑place sorting of the input list in_categorize_snapshot_sets.The categorisation logic itself (boundary selection and multi‑category membership) looks correct, but
sets.sort(key=lambda x: x.timestamp)mutates the caller’s list in place. When called withManager.snapshot_sets, this silently changes the global ordering from the previous name‑sorted order to timestamp order, which can be surprising for any code that relied on the prior ordering.You could keep the categorisation behaviour identical while avoiding side‑effects by iterating over a sorted view instead:
Example tweak avoiding in‑place mutation
- sets.sort(key=lambda x: x.timestamp) - - _log_debug_manager("Classifying %d snapshot sets", len(sets)) + ordered_sets = sorted(sets, key=lambda x: x.timestamp) + + _log_debug_manager("Classifying %d snapshot sets", len(ordered_sets)) @@ - for snapshot_set in sets: + for snapshot_set in ordered_sets:This keeps the manager’s internal ordering decisions explicit and reduces the chance of subtle regressions elsewhere.
doc/user_guide.rst (2)
689-712: Keep the twosnapset list -ohelpfield listings in sync (addcategoriesto both).You’ve added
categoriesto the later “Snapshot set Fields” block (around the reporting section), but the earliersnapm snapset list -ohelpexample still omits it. For readers skimming only the first example, that can look like an inconsistency.It would be clearer to either update the earlier block to include
categoriesas well, or drop one of the duplicated-ohelpoutputs so there’s a single authoritative field list.Also applies to: 1347-1348
1300-1320: Align example category ordering with the implementation’s output order.The new
snapm snapset list -o+categoriesexamples show categories in orders likedaily, hourly, monthly, yearly, whereas the implementation buildsSnapshotSet.categoriesin timeline order (yearly,quarterly,monthly,weekly,daily,hourly) andSnapshotSet.__str__prints them with a simple", ".join(self.categories).To minimise confusion when users compare the docs with real output, consider adjusting the sample lines so the category ordering matches what the CLI actually prints (for example,
yearly, monthly, daily, hourlyfor the multi‑category cases).container_tests/tests/test_schedule.py (2)
29-38: Initialisecategoriesper instance rather than as a mutable class attribute.Defining
categories = []onMockSnapshotSetmakes it a shared mutable class attribute, which is exactly what Ruff is warning about and is slightly misleading given you always assign an instance‑specific list later.You can avoid both the linter warning and any potential confusion by initialising it on the instance instead:
Suggested tweak
-class MockSnapshotSet: - - boot_entry = None - revert_entry = None - categories = [] - - def __init__(self, name, timestamp): - self.name = name - self.timestamp = timestamp - self.deleted = False +class MockSnapshotSet: + + boot_entry = None + revert_entry = None + + def __init__(self, name, timestamp): + self.name = name + self.timestamp = timestamp + self.deleted = False + # Per-instance list of timeline categories (mirrors SnapshotSet.categories) + self.categories = []
391-395: Rename unusedcategoriesunpacking targets to_categories(or_) to satisfy Ruff.In the timeline tests you unpack
_categorize_snapshot_sets(...)into(categories, categorized)but only ever usecategorized. Ruff quite reasonably flagscategoriesas unused (RUF059).To keep the intent clear and silence the warning, you can rename the unused binding to
_categories(or_) in these tests:Example adjustments
- # Set up categorization. - _categories, categorized = _categorize_snapshot_sets(eighteen_months) + # Set up categorization. + _categories, categorized = _categorize_snapshot_sets(eighteen_months) @@ - # Set up categorization. - categories, categorized = _categorize_snapshot_sets(eighteen_months) + # Set up categorization. + _categories, categorized = _categorize_snapshot_sets(eighteen_months) @@ - # Set up categorization. - categories, categorized = _categorize_snapshot_sets([snapshot_set]) + # Set up categorization. + _categories, categorized = _categorize_snapshot_sets([snapshot_set]) @@ - # Set up categorization. - categories, categorized = _categorize_snapshot_sets([snapshot_set]) + # Set up categorization. + _categories, categorized = _categorize_snapshot_sets([snapshot_set]) @@ - # Set up categorization. - categories, categorized = _categorize_snapshot_sets(snapshot_sets) + # Set up categorization. + _categories, categorized = _categorize_snapshot_sets(snapshot_sets) @@ - # Set up categorization. - categories, categorized = _categorize_snapshot_sets(snapshot_sets) + # Set up categorization. + _categories, categorized = _categorize_snapshot_sets(snapshot_sets)(The exact spelling of the throwaway name doesn’t matter; the key point is to signal that the first return value is intentionally unused.)
Also applies to: 406-410, 444-446, 474-477, 510-514, 560-564
tests/test_manager.py (2)
152-195: Tighten mocks and address unused-argument warnings in new manager helper testsThe new tests for
_find_mount_point_for_devpathand_check_snapm_runtime_direxercise the helpers well, but a couple of small clean-ups would reduce noise:
test_find_mount_point_for_devpath’smock_existsandmock_openarguments are unused, which Ruff (ARG002) is already flagging. Renaming them to_mock_exists/_mock_open(or similar) will silence the warning while keeping the patch signatures intact.- In
test_check_snapm_runtime_dir, you patch bothos.lstatandos.statbut only_check_snapm_runtime_dir’sos.lstatresult is used. That’s fine for safety, but if you want to keep the test minimal you could drop the unusedos.statpatch and argument as an optional tidy-up.Functionally the tests look correct; these are just small maintainability tweaks.
1214-1314: Consider stubbing manager interactions forSchedulertests to avoid hidden environment couplingThe
SchedulerTestscorrectly verify creation, editing, deletion, lifecycle proxy, andgcdelegation using mocks, but they rely on a few implicit behaviours:
create/editstill call the real_parse_source_specs, so the tests depend on the host filesystem and plugin discovery for paths like/mntand/mnt/new.gcusesself._manager.find_snapshot_sets(Selection(...)); withself.manager = Mock(), this silently returns aMagicMockwhich is then passed toschedule.gc. That’s sufficient formock_sched.gc.assert_called(), but doesn’t clearly express the intent.To make the tests more robust and explicit, you could:
- Stub
_manager._parse_source_specsin these tests to return a trivial(sources, size_policies)tuple without touching the real environment.- Set
self.manager.find_snapshot_sets = Mock(return_value=[])insetUpso thegcpath clearly operates on a list and the test doesn’t depend onMagicMockbehaviour.This keeps the tests focused on Scheduler behaviour and avoids subtle dependencies on the runtime environment.
man/man8/snapm.8 (1)
1658-1680: Optional: alignuuidfield naming in REPORT FIELDS withlist -o helpoutputIn the “Snapshot set Fields” help example, the field is listed as
uuid, while in the REPORT FIELDS section the corresponding entry is documented under.B UUID. Since users typeuuidwith-o/--options, you may want to switch that REPORT FIELDS bullet to.B uuidfor consistency with the actual field name and the preceding help output. Functionally everything is correct; this is just a documentation polish.Also applies to: 1690-1707
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
container_tests/snapm.confcontainer_tests/tests/test_schedule.pydoc/user_guide.rstman/man8/snapm.8snapm/_snapm.pysnapm/command.pysnapm/manager/_manager.pysnapm/manager/_schedule.pytests/test_manager.py
🧰 Additional context used
🧠 Learnings (24)
📓 Common learnings
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
📚 Learning: 2025-09-09T16:06:19.848Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-09T16:06:19.848Z
Learning: In snapm CLI, `show` commands produce JSON with individual objects using fixed field names that match regular show command output, while `list` commands produce JSON arrays with keys derived from report field names that can be controlled by the caller (fields, ordering, sorting). This architectural difference is intentional - show format is fixed for predictability, list format is flexible for customizable reporting.
Applied to files:
doc/user_guide.rstman/man8/snapm.8
📚 Learning: 2025-09-09T19:26:01.116Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 454
File: man/man8/snapm.8:1434-1462
Timestamp: 2025-09-09T19:26:01.116Z
Learning: The `list` command JSON format in snapm uses a wrapper object structure with a type-specific key (e.g., "Snapsets", "Snapshots") that maps to an array of objects, rather than a direct JSON array. This design provides flexibility for future mixed-object-type reports and follows patterns similar to lvm2's JSON reports. For example: `{"Snapsets": [{"name": "backup", ...}, {"name": "before-upgrade", ...}]}`.
Applied to files:
doc/user_guide.rstman/man8/snapm.8
📚 Learning: 2025-09-09T19:56:30.429Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 454
File: man/man8/snapm.8:1433-1461
Timestamp: 2025-09-09T19:56:30.429Z
Learning: In snapm JSON output, field names always use full prefixed names (e.g., "snapset_name", "snapset_status") to avoid collisions between different object types, even though prefixes may be optional in other contexts when unambiguous. This is an intentional architectural decision for consistency and conflict prevention in JSON reports.
Applied to files:
doc/user_guide.rstman/man8/snapm.8snapm/command.pysnapm/_snapm.py
📚 Learning: 2025-08-24T16:09:15.238Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 400
File: man/man8/snapm.8:496-505
Timestamp: 2025-08-24T16:09:15.238Z
Learning: In UNIX man pages for snapm, metavars (value placeholders) should be lowercase except for UUID which should be uppercase. For example, use `schedule_name` not `SCHEDULE_NAME`, but use `UUID` not `uuid`.
Applied to files:
man/man8/snapm.8snapm/_snapm.py
📚 Learning: 2025-08-22T19:34:08.396Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-22T19:34:08.396Z
Learning: The snapshotmanager organization (including snapm and boom projects) uses "SHORTMONTH DAY YEAR" format for man page headers. This format has been used by boom since 2017. Organizational consistency within snapshotmanager is prioritized over cross-project consistency when it comes to formatting standards.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-09-10T23:42:35.992Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 357
File: snapm.spec:132-134
Timestamp: 2025-09-10T23:42:35.992Z
Learning: In the snapm project, changelog entries in snapm.spec are auto-generated from git log using `git log --oneline v0.4.0..v0.5.0 | sed -e 's/^[a-f0-9]* /- /' -e 's/%//'` and should not be manually edited to avoid rewriting git history.
Applied to files:
man/man8/snapm.8container_tests/snapm.conf
📚 Learning: 2025-12-22T10:31:24.641Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 773
File: snapm.spec:148-149
Timestamp: 2025-12-22T10:31:24.641Z
Learning: In the snapm project's snapm.spec file, do not flag typos, formatting issues, or other problems in the %changelog section, as these entries are auto-generated from git commit messages and represent the historical record. Modifying them would require rewriting git history, which the project avoids.
Applied to files:
man/man8/snapm.8container_tests/snapm.conf
📚 Learning: 2025-12-07T01:32:02.651Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-07T01:32:02.651Z
Learning: In the snapshotmanager/snapm project, files intentionally do not include trailing newlines at the end. This is a deliberate coding style convention and should not be flagged as missing or suggested for addition.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-08-26T13:22:28.829Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-26T13:22:28.829Z
Learning: In the snapm project's man pages, single `.` lines are intentional spacers used to separate and improve readability between macro definitions and section headers like `.SH`/`.SS`. These should not be flagged as formatting errors or suggested for removal.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-09-25T02:09:51.734Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T02:09:51.734Z
Learning: In the snapm codebase, SnapshotSet UUIDs are v5 UUIDs derived from NAMESPACE_SNAPSHOT_SET, the snapshot set name, and timestamp. They are mutable and change when the name changes during rename operations, making them unsuitable as stable lock keys.
Applied to files:
man/man8/snapm.8snapm/command.py
📚 Learning: 2025-09-03T15:21:15.412Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
Applied to files:
container_tests/tests/test_schedule.pysnapm/manager/_manager.pysnapm/manager/_schedule.pytests/test_manager.py
📚 Learning: 2025-10-22T20:06:54.993Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Applied to files:
snapm/manager/_manager.pysnapm/manager/_schedule.pysnapm/_snapm.py
📚 Learning: 2025-09-10T23:42:35.992Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 357
File: snapm.spec:132-134
Timestamp: 2025-09-10T23:42:35.992Z
Learning: The snapm project maintainer bmr-cymru prefers scripted version bumping as part of the snapm-x.y.z-release-prep workflow rather than automated regex-based parsing for version synchronization across files.
Applied to files:
snapm/manager/_manager.pysnapm/manager/_schedule.py
📚 Learning: 2025-11-12T21:27:50.414Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 596
File: snapm/_snapm.py:36-44
Timestamp: 2025-11-12T21:27:50.414Z
Learning: In the snapm codebase, the numeric values of SNAPM_DEBUG_* constants (e.g., SNAPM_DEBUG_MANAGER, SNAPM_DEBUG_MOUNTS, SNAPM_DEBUG_SCHEDULE) are not guaranteed to remain stable across versions. Consumers should always use the symbolic constant names rather than persisting numeric masks. SNAPM_DEBUG_ALL is designed to change over time as new subsystems are added, and repurposing or reassigning bit positions for debug flags is acceptable and intentional.
Applied to files:
snapm/manager/_manager.pysnapm/_snapm.py
📚 Learning: 2025-12-30T14:19:44.950Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-30T14:19:44.950Z
Learning: In snapshotmanager/snapm, ConfigParser construction for plugins is intentionally centralized in exactly one place within Manager._find_and_verify_plugins(). This architectural decision ensures maintainability, and duplicating this pattern elsewhere would create technical debt.
Applied to files:
snapm/manager/_manager.py
📚 Learning: 2025-12-17T16:23:50.413Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 759
File: systemd/tmpfiles.d/snapm.conf:9-9
Timestamp: 2025-12-17T16:23:50.413Z
Learning: In systemd/tmpfiles.d configuration for snapm: use uppercase 'Z' (recursive SELinux context restoration) for /var/cache/snapm because it is a persistent cache directory tree (including subdirectories like /var/cache/snapm/diffcache) that should have proper labeling applied recursively. Unlike /run/snapm/mounts which contains mounted snapshot filesystems, /var/cache/snapm stores cache files and needs recursive context application.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-10-22T19:32:44.315Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T19:32:44.315Z
Learning: In systemd/tmpfiles.d configuration for snapm: use lowercase 'z' (non-recursive SELinux context restoration) instead of uppercase 'Z' (recursive) for /run/snapm and its subdirectories. Recursive context application is hazardous because /run/snapm/mounts contains mounted snapshot filesystems that have their own SELinux contexts which should not be modified. If a user runs systemd-tmpfiles --create, recursive relabeling would incorrectly change contexts on mounted content.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-08-27T20:27:47.299Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-27T20:27:47.299Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-08-27T21:01:32.324Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-27T21:01:32.324Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-11-13T18:16:10.229Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-11-13T18:16:10.229Z
Learning: In snapshotmanager/snapm test code, timestamps in tests like `get_eighteen_months()` intentionally include jitter (non-zero seconds/microseconds) to mimic the behavior of systemd timer units with the default `AccuracySec` value of 1 minute, providing more realistic test coverage for Timeline GC logic.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-08-30T13:46:15.300Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-30T13:46:15.300Z
Learning: In snapm, boom is a hard requirement because snapm.manager._boot imports boom modules directly, and the Manager class imports and uses functionality from _boot including BootCache, create_snapset_boot_entry, and delete_snapset_boot_entry. The Manager class cannot be instantiated without boom being available.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-10-22T16:43:42.894Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T16:43:42.894Z
Learning: In snapm/manager/_manager.py _check_snapm_dir: the correct order for directory validation is: 1) check if path exists and validate it's a directory (not a symlink), 2) os.makedirs() to create if needed, 3) os.chmod() to fix permissions if directory existed with wrong perms, 4) validate final mode matches expected. Do not attempt chmod after makedirs for a newly created directory since makedirs(mode=...) already sets permissions. Do not log errors when chmod fails but the mode is already correct.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-10-20T21:12:52.344Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:372-377
Timestamp: 2025-10-20T21:12:52.344Z
Learning: In snapm/manager/_mounts.py mount operations: do not create missing mount point directories on snapshot filesystems; instead, check if the directory exists with isdir(), log a warning if missing, and skip that mount to avoid modifying the snapshot filesystem unnecessarily.
Applied to files:
tests/test_manager.py
🧬 Code graph analysis (4)
snapm/command.py (2)
snapm/report.py (2)
FieldType(232-286)report_str_list(456-471)snapm/_snapm.py (1)
categories(1301-1309)
snapm/manager/_manager.py (1)
snapm/_snapm.py (6)
datetime(1176-1180)SnapshotSet(1006-1565)categories(1301-1309)timestamp(1169-1173)timestamp(1710-1714)set_categories(1311-1322)
snapm/manager/_schedule.py (1)
snapm/_snapm.py (5)
SnapshotSet(1006-1565)snapshot_set(1739-1743)snapshot_set(1746-1750)set_categories(1311-1322)categories(1301-1309)
tests/test_manager.py (1)
snapm/manager/_manager.py (13)
_find_mount_point_for_devpath(314-325)_check_snapm_runtime_dir(328-356)_categorize_snapshot_sets(537-619)Scheduler(622-924)create(709-759)enable(857-869)start(872-882)_get_schedule_by_name(671-681)edit(762-838)stop(885-895)disable(898-908)delete(841-854)gc(911-924)
🪛 Ruff (0.14.10)
container_tests/tests/test_schedule.py
33-33: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
407-407: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
444-444: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
475-475: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
511-511: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
561-561: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
tests/test_manager.py
155-155: Unused method argument: mock_exists
(ARG002)
155-155: Unused method argument: mock_open
(ARG002)
⏰ 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). (37)
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
🔇 Additional comments (5)
snapm/manager/_manager.py (1)
987-990: Recategorisation lifecycle is well‑integrated after snapshot‑set mutations.Initialising
_categories/_categorizedinManager.__init__and centralising the recomputation in_recategorize_snapshot_sets()is a sensible design. Calling this helper after discovery, create, bulk and single delete, and split keepsSnapshotSet.categoriesin sync with the manager’s view, and batching thedelete_snapshot_sets()case by passingrecategorize=Falseintodelete_snapshot_set()is a nice optimisation.The new
recategorizeargument ondelete_snapshot_set()is backwards‑compatible with existing keyword/positional usage, so the API change is low risk.Also applies to: 1146-1155, 1218-1220, 1454-1458, 1540-1561, 1569-1579, 1755-1870
container_tests/snapm.conf (1)
1-9: Tmpfiles configuration matches SELinux z/Z expectations for runtime vs cache paths.The runtime and cache directories, ownership, and the use of
zfor/run/snapm*vsZfor/var/cache/snapmalign with the documented design (non‑recursive relabelling under/runto avoid touching mounted filesystems, recursive under the persistent cache). This should make the container tests accurately reflect the intended systemd tmpfiles behaviour.Based on learnings, this mirrors the project’s SELinux and tmpfiles conventions.
tests/test_manager.py (1)
1148-1212: Timeline categorisation test matches_categorize_snapshot_setssemanticsThe
TestTimelineCategorizationcase accurately reflects the behaviour of_categorize_snapshot_sets: it validates multi-category membership (yearly/quarterly/monthly/weekly/daily/hourly), ensures only the first set per boundary is selected, and checks the reversecategorized[id(...)]mapping. This gives good coverage of the refactored categorisation logic with simple, deterministic timestamps.snapm/command.py (1)
26-93: Newcategoriesreport field for snapshot sets is wired correctlyImporting
SNAPSET_CATEGORIESand adding aFieldTypethat drivesREP_STR_LISTfromd.categoriesintegrates the new snapshot-set timeline categories cleanly into bothsnapset listandsnapshot listreporting surfaces, with opt‑in exposure via-o categories. The implementation matches theSnapshotSet.categoriesAPI and existing reporting conventions.Also applies to: 323-332
snapm/manager/_schedule.py (1)
323-438: TIMELINE GC now correctly consumes precomputed snapshot-set categoriesThe refactored
GcPolicyParamsTimeline.evaluate()correctly:
- Builds
classified_setskeyed bySNAPSET_TIMELINE_CATEGORIESand usessnapshot_set.categoriesto populate them.- Computes per-category “kept” tails based on the
keep_*knobs, then logs selections per category.- Deletes only snapshot sets that have no categories or for which none of their categories keeps them, which matches the documented “any category keeps ⇒ retain, all vote delete ⇒ remove” behaviour and the existing GC semantics for uncategorised sets.
This cleanly decouples categorisation from retention and aligns with the new Manager-level categorisation pipeline.
|
@coderabbitai: full review |
|
@bmr-cymru: I'll conduct a full review of the PR. As context for the review: the deletion of unclassified snapshot sets is intentional behavior that was already present in the original implementation. Categorization is exhaustive and always runs before garbage collection, so uncategorized sets at GC time indicate an error condition and should be deleted rather than left with only log warnings. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
doc/user_guide.rst (1)
379-387: Documentation for snapshot‑set categories is consistent with the new APIThe added
Categories:lines in thesnapset create,split, andshowexamples, the JSONCategoriesarrays forshow --json, and the newcategoriesfield entry in the snapset field help all match the new publicSnapshotSet.categoriesand reporting behaviour, and clearly explain how to surface categories via-o+categoriesor a custom field list.One minor polish point: there are now two
snapm snapset list -ohelpfield-list examples; only the later one includes the newcategoriesfield. Consider updating the earlier field-list snippet to includecategoriesas well so the documentation stays self-consistent.Also applies to: 404-409, 631-632, 795-802, 815-822, 880-885, 1300-1320, 1347-1347, 1410-1415
tests/test_manager.py (1)
152-166: New manager helper, categorisation, and Scheduler tests look solid (with small test‑harness nits)The added tests for
_find_mount_point_for_devpath,_categorize_snapshot_sets, and theSchedulerlifecycle give useful unit coverage and mirror the intended categorisation semantics (multi-category membership, only first snapshot per boundary, and correct retention behaviour). The Scheduler tests correctly isolate themselves by patchingScheduleand using a temporary config directory, so they avoid touching real systemd state.Two minor clean-ups you might consider in the test code:
- In
test_find_mount_point_for_devpath,mock_existsandmock_openare unused; renaming them to_mock_exists/_mock_openor dropping them from the signature will quiet the RuffARG002hints.- In the new categorisation and GC tests you often write
categories, categorized = _categorize_snapshot_sets(...)but only usecategorized; assigning the first element to_(or_categories) would address theRUF059unused-variable warnings.These are purely cosmetic and don’t affect correctness.
Also applies to: 1148-1314
container_tests/tests/test_schedule.py (1)
21-34: Timeline GC tests now correctly reflect the new categorisation contractImporting
_categorize_snapshot_setsand updating the various TIMELINEGcPolicytests to compute categories up-front and attach them to eachMockSnapshotSetaligns the container tests with the refactored production logic (where categorisation is done once at the manager/scheduler layer andGcPolicyParamsTimeline.evaluateconsumessnapshot_set.categories). The progressive three‑month hourly test still accurately exercises the real-world “GC after each interval” behaviour.A couple of small test‑only refinements you might consider:
MockSnapshotSet.categoriesis defined as a mutable class attribute; given every test assignssnapshot_set.categories = ...on instances, this is safe in practice, but moving it into__init__(or annotating it as aClassVarif you keep it as a class attribute with type hints) would avoid the RUF012 warning and any accidental sharing.- In several places you unpack
_categorize_snapshot_sets(...)intocategories, categorizedbut only usecategorized. Binding the first element to_(or_categories) will address the RUF059 unused-variable hints without changing behaviour.These are optional tidy-ups; the functional intent and coverage look good.
Also applies to: 387-415, 401-416, 441-447, 471-477, 500-523, 535-568
snapm/manager/_schedule.py (1)
343-355: Defensive handling of non‑timeline categories in GC evaluationThe evaluation correctly drives from
SnapshotSet.categoriesandSNAPSET_TIMELINE_CATEGORIES, butshould_keepindexeskept_by_category[category]for every string insset_cats. If any non‑timeline category ever appears (e.g. from future extensions or client misuse ofset_categories()), this will raiseKeyErrorand abort GC.Consider filtering the categories you consult during GC:
- When computing
should_keep, ignore anycategorynot inkept_by_category.- Optionally also log or warn on unknown categories so they don’t silently affect retention.
This keeps the logic robust if additional snapshot‑set categories are introduced later while preserving current behaviour for the six recognised timeline classes.
Also applies to: 372-402, 417-432
snapm/_snapm.py (1)
128-130: MakeSnapshotSet.categorieseffectively read‑only to callersThe new
categoriesproperty andset_categories()hook are a good fit for exposing timeline classification, butcategoriescurrently returns the internal list, so external code can mutate it in place:snapset.categories.append("foo")This can break the Manager’s categorisation invariants and cause GC logic to see unexpected category values.
To keep categorisation Manager‑owned while still exposing it:
- Have
categoriesreturn a copy or a tuple (e.g.return tuple(self._categories)), and- Keep
set_categories()as the single mutator used by Manager.This preserves the public surface while preventing accidental external mutation of internal state.
Also applies to: 152-160, 1037-1038, 1068-1069, 1104-1105, 1300-1323, 2368-2387
snapm/manager/_manager.py (1)
13-14: Avoid mutating caller order in_categorize_snapshot_sets
_categorize_snapshot_sets()currently does an in‑placesets.sort(key=lambda x: x.timestamp). BecauseManager._recategorize_snapshot_sets()passesself.snapshot_setsinto this helper, this reorders the manager’s primary list from name‑sorted (as established indiscover_snapshot_sets()) to timestamp‑sorted as a side‑effect of recategorisation.GC and categorisation only require a chronological view for their own logic; they don’t need to change the manager’s global ordering. To keep behaviour local and preserve any existing expectations about
snapshot_setsordering:
- Use a local sorted view instead of mutating the input list, e.g.:
ordered_sets = sorted(sets, key=lambda s: s.timestamp) for snapshot_set in ordered_sets: ...and base both
categoriesandcategorizedcomputation onordered_sets.The rest of the recategorisation wiring (calling
_recategorize_snapshot_sets()after discovery/create/delete/split and propagating viaset_categories()) looks sound and keepsSnapshotSet.categoriesaligned with manager state.Also applies to: 22-22, 28-55, 537-620, 987-990, 1145-1155, 1217-1219, 1453-1457, 1539-1549, 1558-1559, 1568-1578, 1865-1869
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
container_tests/snapm.confcontainer_tests/tests/test_schedule.pydoc/user_guide.rstman/man8/snapm.8snapm/_snapm.pysnapm/command.pysnapm/manager/_manager.pysnapm/manager/_schedule.pytests/test_manager.py
🧰 Additional context used
🧠 Learnings (24)
📓 Common learnings
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
📚 Learning: 2025-09-03T15:21:15.412Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
Applied to files:
snapm/manager/_schedule.pytests/test_manager.pycontainer_tests/tests/test_schedule.pysnapm/manager/_manager.py
📚 Learning: 2025-10-22T20:06:54.993Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Applied to files:
snapm/manager/_schedule.pyman/man8/snapm.8snapm/_snapm.pysnapm/manager/_manager.py
📚 Learning: 2025-09-10T23:42:35.992Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 357
File: snapm.spec:132-134
Timestamp: 2025-09-10T23:42:35.992Z
Learning: The snapm project maintainer bmr-cymru prefers scripted version bumping as part of the snapm-x.y.z-release-prep workflow rather than automated regex-based parsing for version synchronization across files.
Applied to files:
snapm/manager/_schedule.pysnapm/manager/_manager.py
📚 Learning: 2025-09-09T16:06:19.848Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-09T16:06:19.848Z
Learning: In snapm CLI, `show` commands produce JSON with individual objects using fixed field names that match regular show command output, while `list` commands produce JSON arrays with keys derived from report field names that can be controlled by the caller (fields, ordering, sorting). This architectural difference is intentional - show format is fixed for predictability, list format is flexible for customizable reporting.
Applied to files:
doc/user_guide.rstman/man8/snapm.8
📚 Learning: 2025-09-09T19:26:01.116Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 454
File: man/man8/snapm.8:1434-1462
Timestamp: 2025-09-09T19:26:01.116Z
Learning: The `list` command JSON format in snapm uses a wrapper object structure with a type-specific key (e.g., "Snapsets", "Snapshots") that maps to an array of objects, rather than a direct JSON array. This design provides flexibility for future mixed-object-type reports and follows patterns similar to lvm2's JSON reports. For example: `{"Snapsets": [{"name": "backup", ...}, {"name": "before-upgrade", ...}]}`.
Applied to files:
doc/user_guide.rstman/man8/snapm.8
📚 Learning: 2025-09-09T19:56:30.429Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 454
File: man/man8/snapm.8:1433-1461
Timestamp: 2025-09-09T19:56:30.429Z
Learning: In snapm JSON output, field names always use full prefixed names (e.g., "snapset_name", "snapset_status") to avoid collisions between different object types, even though prefixes may be optional in other contexts when unambiguous. This is an intentional architectural decision for consistency and conflict prevention in JSON reports.
Applied to files:
doc/user_guide.rstman/man8/snapm.8snapm/command.pysnapm/_snapm.py
📚 Learning: 2025-11-13T18:16:10.229Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-11-13T18:16:10.229Z
Learning: In snapshotmanager/snapm test code, timestamps in tests like `get_eighteen_months()` intentionally include jitter (non-zero seconds/microseconds) to mimic the behavior of systemd timer units with the default `AccuracySec` value of 1 minute, providing more realistic test coverage for Timeline GC logic.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-08-30T13:46:15.300Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-30T13:46:15.300Z
Learning: In snapm, boom is a hard requirement because snapm.manager._boot imports boom modules directly, and the Manager class imports and uses functionality from _boot including BootCache, create_snapset_boot_entry, and delete_snapset_boot_entry. The Manager class cannot be instantiated without boom being available.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-10-22T16:43:42.894Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T16:43:42.894Z
Learning: In snapm/manager/_manager.py _check_snapm_dir: the correct order for directory validation is: 1) check if path exists and validate it's a directory (not a symlink), 2) os.makedirs() to create if needed, 3) os.chmod() to fix permissions if directory existed with wrong perms, 4) validate final mode matches expected. Do not attempt chmod after makedirs for a newly created directory since makedirs(mode=...) already sets permissions. Do not log errors when chmod fails but the mode is already correct.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-10-20T21:12:52.344Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:372-377
Timestamp: 2025-10-20T21:12:52.344Z
Learning: In snapm/manager/_mounts.py mount operations: do not create missing mount point directories on snapshot filesystems; instead, check if the directory exists with isdir(), log a warning if missing, and skip that mount to avoid modifying the snapshot filesystem unnecessarily.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-09-10T23:42:35.992Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 357
File: snapm.spec:132-134
Timestamp: 2025-09-10T23:42:35.992Z
Learning: In the snapm project, changelog entries in snapm.spec are auto-generated from git log using `git log --oneline v0.4.0..v0.5.0 | sed -e 's/^[a-f0-9]* /- /' -e 's/%//'` and should not be manually edited to avoid rewriting git history.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-12-22T10:31:24.641Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 773
File: snapm.spec:148-149
Timestamp: 2025-12-22T10:31:24.641Z
Learning: In the snapm project's snapm.spec file, do not flag typos, formatting issues, or other problems in the %changelog section, as these entries are auto-generated from git commit messages and represent the historical record. Modifying them would require rewriting git history, which the project avoids.
Applied to files:
man/man8/snapm.8container_tests/snapm.conf
📚 Learning: 2025-08-22T19:34:08.396Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-22T19:34:08.396Z
Learning: The snapshotmanager organization (including snapm and boom projects) uses "SHORTMONTH DAY YEAR" format for man page headers. This format has been used by boom since 2017. Organizational consistency within snapshotmanager is prioritized over cross-project consistency when it comes to formatting standards.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-08-24T16:09:15.238Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 400
File: man/man8/snapm.8:496-505
Timestamp: 2025-08-24T16:09:15.238Z
Learning: In UNIX man pages for snapm, metavars (value placeholders) should be lowercase except for UUID which should be uppercase. For example, use `schedule_name` not `SCHEDULE_NAME`, but use `UUID` not `uuid`.
Applied to files:
man/man8/snapm.8snapm/_snapm.py
📚 Learning: 2025-08-25T17:19:19.563Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-25T17:19:19.563Z
Learning: The man page changes mentioned in the AI summary for PR #383 were rebasing artifacts and should be ignored when reviewing this branch - there are no actual man page changes on the bmr-vm-tests branch.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-12-07T01:32:02.651Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-07T01:32:02.651Z
Learning: In the snapshotmanager/snapm project, files intentionally do not include trailing newlines at the end. This is a deliberate coding style convention and should not be flagged as missing or suggested for addition.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-08-26T13:22:28.829Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-26T13:22:28.829Z
Learning: In the snapm project's man pages, single `.` lines are intentional spacers used to separate and improve readability between macro definitions and section headers like `.SH`/`.SS`. These should not be flagged as formatting errors or suggested for removal.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-09-25T02:09:51.734Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T02:09:51.734Z
Learning: In the snapm codebase, SnapshotSet UUIDs are v5 UUIDs derived from NAMESPACE_SNAPSHOT_SET, the snapshot set name, and timestamp. They are mutable and change when the name changes during rename operations, making them unsuitable as stable lock keys.
Applied to files:
man/man8/snapm.8snapm/command.py
📚 Learning: 2025-12-17T16:23:50.413Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 759
File: systemd/tmpfiles.d/snapm.conf:9-9
Timestamp: 2025-12-17T16:23:50.413Z
Learning: In systemd/tmpfiles.d configuration for snapm: use uppercase 'Z' (recursive SELinux context restoration) for /var/cache/snapm because it is a persistent cache directory tree (including subdirectories like /var/cache/snapm/diffcache) that should have proper labeling applied recursively. Unlike /run/snapm/mounts which contains mounted snapshot filesystems, /var/cache/snapm stores cache files and needs recursive context application.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-10-22T19:32:44.315Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T19:32:44.315Z
Learning: In systemd/tmpfiles.d configuration for snapm: use lowercase 'z' (non-recursive SELinux context restoration) instead of uppercase 'Z' (recursive) for /run/snapm and its subdirectories. Recursive context application is hazardous because /run/snapm/mounts contains mounted snapshot filesystems that have their own SELinux contexts which should not be modified. If a user runs systemd-tmpfiles --create, recursive relabeling would incorrectly change contexts on mounted content.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-08-27T20:27:47.299Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-27T20:27:47.299Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-08-27T21:01:32.324Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-27T21:01:32.324Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
Applied to files:
container_tests/snapm.conf
📚 Learning: 2025-11-12T21:27:50.414Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 596
File: snapm/_snapm.py:36-44
Timestamp: 2025-11-12T21:27:50.414Z
Learning: In the snapm codebase, the numeric values of SNAPM_DEBUG_* constants (e.g., SNAPM_DEBUG_MANAGER, SNAPM_DEBUG_MOUNTS, SNAPM_DEBUG_SCHEDULE) are not guaranteed to remain stable across versions. Consumers should always use the symbolic constant names rather than persisting numeric masks. SNAPM_DEBUG_ALL is designed to change over time as new subsystems are added, and repurposing or reassigning bit positions for debug flags is acceptable and intentional.
Applied to files:
snapm/_snapm.pysnapm/manager/_manager.py
🧬 Code graph analysis (3)
tests/test_manager.py (2)
snapm/manager/_manager.py (6)
_find_mount_point_for_devpath(314-325)_check_snapm_runtime_dir(328-356)_categorize_snapshot_sets(537-619)create(709-759)disable(898-908)delete(841-854)snapm/manager/_schedule.py (4)
name(552-556)name(837-841)disable(606-610)disable(939-946)
snapm/command.py (2)
snapm/report.py (2)
FieldType(232-286)report_str_list(456-471)snapm/_snapm.py (1)
categories(1301-1309)
container_tests/tests/test_schedule.py (3)
snapm/manager/_manager.py (1)
_categorize_snapshot_sets(537-619)snapm/_snapm.py (6)
categories(1301-1309)snapshot_set(1739-1743)snapshot_set(1746-1750)datetime(1176-1180)timestamp(1169-1173)timestamp(1710-1714)snapm/manager/_schedule.py (6)
evaluate(102-114)evaluate(136-151)evaluate(178-198)evaluate(260-281)evaluate(324-438)evaluate(685-697)
🪛 Ruff (0.14.10)
tests/test_manager.py
155-155: Unused method argument: mock_exists
(ARG002)
155-155: Unused method argument: mock_open
(ARG002)
container_tests/tests/test_schedule.py
33-33: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
407-407: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
444-444: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
475-475: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
511-511: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
561-561: Unpacked variable categories is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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). (24)
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
🔇 Additional comments (3)
container_tests/snapm.conf (1)
2-9: Runtime directory and SELinux labelling look correctThe tmpfiles configuration matches the runtime-dir security expectations: 0700 root:root on all snapm runtime paths, non-recursive
zfor/run/snapm*, and recursiveZfor/var/cache/snapm, which is exactly the split required to avoid relabelling mounted filesystems while still fixing labels under the persistent cache tree.Based on learnings, this follows the documented
z/Zusage for/run/snapmvs/var/cache/snapm.snapm/command.py (1)
26-50: Newcategoriesreport field is wired correctlyImporting
SNAPSET_CATEGORIESand adding aFieldTypethat maps"categories"tod.categorieswithREP_STR_LISTis consistent with the existing reporting model and keeps the default snapset field set unchanged, while allowing-o+categoriesand JSON to exposesnapset_categorieswhen requested.Based on learnings, this also respects the JSON naming convention that uses fully prefixed field names in report output.
Also applies to: 160-332
man/man8/snapm.8 (1)
1000-1053: Categories field documentation and examples look consistentThe new
Categorieslines in text and JSON examples, the addedcategoriesentry in thesnapset list -o helpoutput, and the REPORT FIELDS description all align with the six recognised timeline categories and the new SnapshotSet API. JSON snippets are syntactically valid and match the described output format.Also applies to: 1660-1680, 1748-1753, 1918-1925, 2039-2046, 2081-2088
|
@coderabbitai: re:
That's not correct. Those examples are taken from actual tool output. You seem to have overlooked the fact that the 456 def report_str_list(self, value):
457 """
458 Report a string list value for this Field object.
459
460 Set the value for this field to the strings contained in ``value``.
461
462 :param value: A list of strings
463 :rtype: None
464 """
465 if not isinstance(value, list):
466 raise TypeError("Value for report_str_list() must be a list type.")
467 if not all(isinstance(v, string_types) for v in value):
468 raise TypeError("Value for report_str_list() must be a list of strings.")
469 value = sorted(value)
470 list_value = ", ".join(value)
471 self.set_value(list_value, sort_value=list_value) |
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru: You're absolutely right—I apologize for that incorrect suggestion. I missed that 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru: You're absolutely right—I apologize for that incorrect suggestion. I missed that ✏️ Learnings added
🧠 Learnings used |
|
@coderabbitai: re:
We want that to blow up. That would be a severe bug or misuse of the |
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru: You're absolutely right—thank you for the clarification! The fail-fast behavior is the correct design here. If invalid categories appear in ✏️ Learnings added
🧠 Learnings used |
bmr-cymru
force-pushed
the
bmr-timeline-classify-refactor
branch
from
cced502 to
37aa3a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_manager.py (2)
152-166: Tidy up unused mocks intest_find_mount_point_for_devpathThe patches and behaviour look correct, but
mock_existsandmock_openare not used in the body, which triggers static-analysis noise. You can keep the patches while silencing the warnings by prefixing the unused parameters with_:Proposed adjustment
- @patch("builtins.open", new_callable=unittest.mock.mock_open, read_data="/dev/sda1 /boot xfs rw 0 0\n/dev/mapper/vg-lv /home ext4 rw 0 0\n") - @patch("snapm.manager._manager.exists", return_value=True) - @patch("snapm.manager._manager.samefile", return_value=True) - def test_find_mount_point_for_devpath(self, mock_samefile, mock_exists, mock_open): + @patch("builtins.open", new_callable=unittest.mock.mock_open, read_data="/dev/sda1 /boot xfs rw 0 0\n/dev/mapper/vg-lv /home ext4 rw 0 0\n") + @patch("snapm.manager._manager.exists", return_value=True) + @patch("snapm.manager._manager.samefile", return_value=True) + def test_find_mount_point_for_devpath(self, mock_samefile, _mock_exists, _mock_open):
167-195:test_check_snapm_runtime_direxercises the right paths;os.statpatch is optionalThe test correctly:
- Patches
SNAPM_RUNTIME_DIRandexistsinsnapm.manager._managerso_check_snapm_runtime_dir()operates on a fake path.- Uses
os.lstatreturn values to simulate secure vs insecure directory cases and asserts thatSnapmSystemErroris raised appropriately.
os.statis not used by_check_snapm_runtime_dir()(onlyos.lstatis), so the@patch("os.stat")andmock_statassignments are redundant. You can drop them to simplify the test, but it is not functionally harmful as-is.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
container_tests/snapm.confcontainer_tests/tests/test_schedule.pydoc/user_guide.rstman/man8/snapm.8snapm/_snapm.pysnapm/command.pysnapm/manager/_manager.pysnapm/manager/_schedule.pytests/test_manager.py
🚧 Files skipped from review as they are similar to previous changes (3)
- container_tests/snapm.conf
- snapm/command.py
- container_tests/tests/test_schedule.py
🧰 Additional context used
🧠 Learnings (22)
📓 Common learnings
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2026-01-03T17:05:40.850Z
Learning: In the snapm codebase, the GC evaluation logic in `snapm/manager/_schedule.py` intentionally allows KeyError when indexing `kept_by_category[category]` if invalid categories appear in `SnapshotSet.categories`. This fail-fast behavior is by design: invalid categories indicate severe bugs or API misuse, and should blow up rather than fail silently. The `set_categories()` method is forbidden for client code per its docstring.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
📚 Learning: 2025-10-20T21:12:52.344Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:372-377
Timestamp: 2025-10-20T21:12:52.344Z
Learning: In snapm/manager/_mounts.py mount operations: do not create missing mount point directories on snapshot filesystems; instead, check if the directory exists with isdir(), log a warning if missing, and skip that mount to avoid modifying the snapshot filesystem unnecessarily.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-10-22T16:43:42.894Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T16:43:42.894Z
Learning: In snapm/manager/_manager.py _check_snapm_dir: the correct order for directory validation is: 1) check if path exists and validate it's a directory (not a symlink), 2) os.makedirs() to create if needed, 3) os.chmod() to fix permissions if directory existed with wrong perms, 4) validate final mode matches expected. Do not attempt chmod after makedirs for a newly created directory since makedirs(mode=...) already sets permissions. Do not log errors when chmod fails but the mode is already correct.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-08-30T13:46:15.300Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-30T13:46:15.300Z
Learning: In snapm, boom is a hard requirement because snapm.manager._boot imports boom modules directly, and the Manager class imports and uses functionality from _boot including BootCache, create_snapset_boot_entry, and delete_snapset_boot_entry. The Manager class cannot be instantiated without boom being available.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-09-09T16:06:19.848Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-09T16:06:19.848Z
Learning: In snapm CLI, `show` commands produce JSON with individual objects using fixed field names that match regular show command output, while `list` commands produce JSON arrays with keys derived from report field names that can be controlled by the caller (fields, ordering, sorting). This architectural difference is intentional - show format is fixed for predictability, list format is flexible for customizable reporting.
Applied to files:
doc/user_guide.rstman/man8/snapm.8
📚 Learning: 2025-09-09T19:26:01.116Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 454
File: man/man8/snapm.8:1434-1462
Timestamp: 2025-09-09T19:26:01.116Z
Learning: The `list` command JSON format in snapm uses a wrapper object structure with a type-specific key (e.g., "Snapsets", "Snapshots") that maps to an array of objects, rather than a direct JSON array. This design provides flexibility for future mixed-object-type reports and follows patterns similar to lvm2's JSON reports. For example: `{"Snapsets": [{"name": "backup", ...}, {"name": "before-upgrade", ...}]}`.
Applied to files:
doc/user_guide.rstman/man8/snapm.8
📚 Learning: 2025-09-09T19:56:30.429Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 454
File: man/man8/snapm.8:1433-1461
Timestamp: 2025-09-09T19:56:30.429Z
Learning: In snapm JSON output, field names always use full prefixed names (e.g., "snapset_name", "snapset_status") to avoid collisions between different object types, even though prefixes may be optional in other contexts when unambiguous. This is an intentional architectural decision for consistency and conflict prevention in JSON reports.
Applied to files:
doc/user_guide.rstsnapm/_snapm.pyman/man8/snapm.8
📚 Learning: 2026-01-03T16:49:13.418Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2026-01-03T16:49:13.418Z
Learning: In the snapm codebase, the `report_str_list()` method in `snapm/command.py` automatically sorts string lists (using `sorted(value)`) before outputting them. Documentation examples showing category ordering (e.g., "daily, hourly") reflect this sorted output behavior and should not be changed.
Applied to files:
doc/user_guide.rstsnapm/_snapm.pyman/man8/snapm.8snapm/manager/_manager.py
📚 Learning: 2026-01-03T17:05:40.850Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2026-01-03T17:05:40.850Z
Learning: In the snapm codebase, the GC evaluation logic in `snapm/manager/_schedule.py` intentionally allows KeyError when indexing `kept_by_category[category]` if invalid categories appear in `SnapshotSet.categories`. This fail-fast behavior is by design: invalid categories indicate severe bugs or API misuse, and should blow up rather than fail silently. The `set_categories()` method is forbidden for client code per its docstring.
Applied to files:
snapm/_snapm.pysnapm/manager/_schedule.pysnapm/manager/_manager.py
📚 Learning: 2025-10-22T20:06:54.993Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Applied to files:
snapm/_snapm.pysnapm/manager/_schedule.pyman/man8/snapm.8snapm/manager/_manager.py
📚 Learning: 2025-08-24T16:09:15.238Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 400
File: man/man8/snapm.8:496-505
Timestamp: 2025-08-24T16:09:15.238Z
Learning: In UNIX man pages for snapm, metavars (value placeholders) should be lowercase except for UUID which should be uppercase. For example, use `schedule_name` not `SCHEDULE_NAME`, but use `UUID` not `uuid`.
Applied to files:
snapm/_snapm.pyman/man8/snapm.8
📚 Learning: 2025-11-12T21:27:50.414Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 596
File: snapm/_snapm.py:36-44
Timestamp: 2025-11-12T21:27:50.414Z
Learning: In the snapm codebase, the numeric values of SNAPM_DEBUG_* constants (e.g., SNAPM_DEBUG_MANAGER, SNAPM_DEBUG_MOUNTS, SNAPM_DEBUG_SCHEDULE) are not guaranteed to remain stable across versions. Consumers should always use the symbolic constant names rather than persisting numeric masks. SNAPM_DEBUG_ALL is designed to change over time as new subsystems are added, and repurposing or reassigning bit positions for debug flags is acceptable and intentional.
Applied to files:
snapm/_snapm.pysnapm/manager/_manager.py
📚 Learning: 2025-09-03T15:21:15.412Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
Applied to files:
snapm/manager/_schedule.pysnapm/manager/_manager.py
📚 Learning: 2025-09-10T23:42:35.992Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 357
File: snapm.spec:132-134
Timestamp: 2025-09-10T23:42:35.992Z
Learning: The snapm project maintainer bmr-cymru prefers scripted version bumping as part of the snapm-x.y.z-release-prep workflow rather than automated regex-based parsing for version synchronization across files.
Applied to files:
snapm/manager/_schedule.pysnapm/manager/_manager.py
📚 Learning: 2025-08-22T19:34:08.396Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-22T19:34:08.396Z
Learning: The snapshotmanager organization (including snapm and boom projects) uses "SHORTMONTH DAY YEAR" format for man page headers. This format has been used by boom since 2017. Organizational consistency within snapshotmanager is prioritized over cross-project consistency when it comes to formatting standards.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-09-10T23:42:35.992Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 357
File: snapm.spec:132-134
Timestamp: 2025-09-10T23:42:35.992Z
Learning: In the snapm project, changelog entries in snapm.spec are auto-generated from git log using `git log --oneline v0.4.0..v0.5.0 | sed -e 's/^[a-f0-9]* /- /' -e 's/%//'` and should not be manually edited to avoid rewriting git history.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-12-22T10:31:24.641Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 773
File: snapm.spec:148-149
Timestamp: 2025-12-22T10:31:24.641Z
Learning: In the snapm project's snapm.spec file, do not flag typos, formatting issues, or other problems in the %changelog section, as these entries are auto-generated from git commit messages and represent the historical record. Modifying them would require rewriting git history, which the project avoids.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-12-07T01:32:02.651Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-07T01:32:02.651Z
Learning: In the snapshotmanager/snapm project, files intentionally do not include trailing newlines at the end. This is a deliberate coding style convention and should not be flagged as missing or suggested for addition.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-08-26T13:22:28.829Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-26T13:22:28.829Z
Learning: In the snapm project's man pages, single `.` lines are intentional spacers used to separate and improve readability between macro definitions and section headers like `.SH`/`.SS`. These should not be flagged as formatting errors or suggested for removal.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-09-25T02:09:51.734Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T02:09:51.734Z
Learning: In the snapm codebase, SnapshotSet UUIDs are v5 UUIDs derived from NAMESPACE_SNAPSHOT_SET, the snapshot set name, and timestamp. They are mutable and change when the name changes during rename operations, making them unsuitable as stable lock keys.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-08-22T16:15:49.986Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 400
File: man/man8/snapm.8:128-133
Timestamp: 2025-08-22T16:15:49.986Z
Learning: In UNIX systems, command line arguments should remain lowercase (e.g., --uuid, --snapshot-uuid) following standard UNIX conventions. Only the metavar (value placeholder) should be uppercase (e.g., UUID). This is different from Windows conventions. Command line option names themselves should never be changed to uppercase.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-10-20T12:46:58.210Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/_snapm.py:1823-1845
Timestamp: 2025-10-20T12:46:58.210Z
Learning: The snapm project prefers using long command-line options (e.g., `--uuid`, `--label`) over short options (e.g., `-U`, `-L`) for clarity in source code, process listings, and debug output.
Applied to files:
man/man8/snapm.8
🧬 Code graph analysis (3)
tests/test_manager.py (1)
snapm/manager/_manager.py (13)
_find_mount_point_for_devpath(314-325)_check_snapm_runtime_dir(328-356)_categorize_snapshot_sets(537-615)Scheduler(618-920)create(705-755)enable(853-865)start(868-878)_get_schedule_by_name(667-677)edit(758-834)stop(881-891)disable(894-904)delete(837-850)gc(907-920)
snapm/manager/_schedule.py (1)
snapm/_snapm.py (3)
SnapshotSet(1006-1565)set_categories(1311-1322)categories(1301-1309)
snapm/manager/_manager.py (1)
snapm/_snapm.py (7)
datetime(1176-1180)SnapshotSet(1006-1565)timestamp(1169-1173)timestamp(1710-1714)set_categories(1311-1322)name(1140-1144)name(1659-1663)
🪛 Ruff (0.14.10)
tests/test_manager.py
155-155: Unused method argument: mock_exists
(ARG002)
155-155: Unused method argument: mock_open
(ARG002)
1184-1184: Undefined name categories
(F821)
1185-1185: Undefined name categories
(F821)
1186-1186: Undefined name categories
(F821)
1187-1187: Undefined name categories
(F821)
1188-1188: Undefined name categories
(F821)
1189-1189: Undefined name categories
(F821)
1192-1192: Undefined name categories
(F821)
1193-1193: Undefined name categories
(F821)
1196-1196: Undefined name categories
(F821)
1197-1197: Undefined name categories
(F821)
1198-1198: Undefined name categories
(F821)
1201-1201: Undefined name categories
(F821)
1202-1202: Undefined name categories
(F821)
1205-1205: Undefined name categories
(F821)
1206-1206: Undefined name categories
(F821)
⏰ 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). (37)
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (19)
doc/user_guide.rst (1)
379-405: Timeline categories documentation looks consistent and correctly integratedThe added
Categoriesfield, examples forsnapset create/list/show, field help entries, and JSON snippets are coherent with the newSnapshotSet.categoriesinterface and sorted list behaviour for reports. I do not see any discrepancies between these docs and the described CLI semantics.Also applies to: 631-632, 712-712, 795-817, 880-885, 1301-1320, 1348-1349, 1411-1416
snapm/manager/_schedule.py (1)
23-29: Timeline GC now correctly driven bySNAPSET_TIMELINE_CATEGORIESandSnapshotSet.categoriesThe refactored
GcPolicyParamsTimeline.evaluate():
- Uses
SNAPSET_TIMELINE_CATEGORIESconsistently for bucket initialisation, logging, and retention computation.- Consumes
SnapshotSet.categoriesto build both per-category lists and an id→categories map.- Retains the intentional fail-fast behaviour: any invalid category in
SnapshotSet.categoriesstill propagates tokept_by_category[category]lookups and raisesKeyError, surfacing bugs promptly.- Implements the documented rule that a set is deleted only if all applicable categories vote for deletion, with uncategorised sets always deleted.
This aligns with the manager-side categorisation helper and the documented timeline policy semantics.
Also applies to: 343-355, 364-371, 372-402, 404-433
tests/test_manager.py (1)
1214-1314: Scheduler tests exercise core lifecycle paths via mocks appropriatelyThe new
SchedulerTestsclass:
- Constructs
Schedulerwith a temporary directory and a mocked manager to isolate on-disk effects.- Uses a patched
Scheduleclass forcreate/edittests, asserting configuration writing, enable/start calls, and internal schedule tracking.- Verifies
deleteand lifecycle proxy methods (enable,stop,disable,gc) delegate correctly to the underlyingScheduleinstances.This provides good coverage of the scheduler’s orchestration logic without depending on real schedules on disk.
man/man8/snapm.8 (1)
1002-1053: Man-page updates forcategoriesfield and JSON output are consistent and validThe man page now:
- Documents
categoriesin the snapshot-set field list and REPORT FIELDS with a clear description of permissible values.- Shows
Categories:in text examples forsnapset create/show, matching the newSnapshotSet.categoriesoutput.- Extends JSON examples to include a
"Categories"array with valid JSON syntax (no trailing commas), aligned with the user-guide examples and reporting behaviour.These changes accurately reflect the new public interface without introducing formatting or schema inconsistencies.
Also applies to: 1661-1679, 1696-1697, 1748-1753, 1920-1925, 2043-2046, 2087-2088
snapm/_snapm.py (7)
11-11: LGTM!The addition of
Listto the typing imports is necessary for the newcategoriesproperty andset_categoriesmethod type hints.
128-128: LGTM!The new constants follow existing naming conventions and provide a clear, canonical definition of timeline categories. The ordering from yearly to hourly is logical and consistent with the categorization algorithm.
Also applies to: 152-160
1037-1037: LGTM!The initialisation of
_categoriesas an empty list is correct and follows the pattern of other instance variables inSnapshotSet.__init__.
1068-1068: LGTM!The addition of categories to the string representation is consistent with other fields and uses the standard
', '.join()pattern for formatting lists.
1104-1104: LGTM!The addition of categories to the dictionary representation is consistent with other fields and correctly uses the constant
SNAPSET_CATEGORIESas the key.
1300-1323: LGTM!The
categoriesproperty andset_categoriesmethod are well-implemented with proper defensive copying. The docstring correctly warns thatset_categoriesis intended forManageruse only, which aligns with the design decision that categorisation is managed centrally.
2368-2368: LGTM!The new constants are correctly added to
__all__for public export, following the pattern of other exported constants.Also applies to: 2386-2386
snapm/manager/_manager.py (8)
13-13: LGTM!The import additions are necessary and correctly support the new categorisation functionality:
datetimeandtimedeltafor boundary calculations,DictandListfor type hints, andSNAPSET_TIMELINE_CATEGORIESfor the canonical category definitions.Also applies to: 22-22, 32-32
537-616: LGTM!The
_categorize_snapshot_setsfunction correctly implements the timeline classification algorithm. The logic properly identifies the "first snapshot set after each boundary" by:
- Sorting input by timestamp to ensure chronological processing (addressing the previous review concern)
- Computing appropriate boundaries for each category
- Filtering categories based on datetime properties (quarterly months, weekly Monday)
- Tracking seen boundaries to ensure only the first snapshot after each boundary is categorised
The function returns a mapping from snapshot set ID to category list, which is then applied by
_recategorize_snapshot_sets.
1137-1144: LGTM!The
_recategorize_snapshot_setsmethod provides a clean interface for updating categorisation. It delegates the algorithm to_categorize_snapshot_setsand propagates the results to eachSnapshotSetinstance viaset_categories.
1208-1208: LGTM!The call to
_recategorize_snapshot_setsis correctly placed after snapshot set discovery and indexing, ensuring that categories are computed during initialisation.
1446-1446: LGTM!The call to
_recategorize_snapshot_setsafter creating a snapshot set is necessary, as the new snapshot set may affect categorisation (e.g., becoming the "first after boundary" for some categories).
1529-1529: LGTM!The addition of the
recategorizeparameter todelete_snapshot_setis a good optimisation that allows batching of deletions whilst deferring recategorisation until after all deletions complete. The default value ofTruemaintains backward compatibility.Also applies to: 1537-1538, 1548-1549
1565-1567: LGTM!The optimisation in
delete_snapshot_setsis well-implemented: it defers recategorisation until after all deletions by passingrecategorize=Falseto each call, then recategorises once at the end. This avoids O(N²) behaviour when deleting multiple snapshot sets.
1858-1858: LGTM!The call to
_recategorize_snapshot_setsafter splitting a snapshot set is necessary, as the operation creates a new snapshot set and modifies an existing one, both of which may affect categorisation.
Move the timeline classification algorithm from GcPolicyParams.timeline() to a new snapm.manager._manager function, _categorize_snapshot_sets(), then extend Managerto update categorizations on individualSnapshotSet` objects every time the list of snapshot sets changes. Resolves: #882 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #883 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #884 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
bmr-cymru
force-pushed
the
bmr-timeline-classify-refactor
branch
from
37aa3a5 to
1d66e8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
snapm/manager/_manager.py (1)
537-615: Centralised snapshot-set categorisation in Manager is coherent and wired through all mutating paths
_categorize_snapshot_sets()now sorts sets bytimestamp, usesSNAPSET_TIMELINE_CATEGORIESas the single source of truth, and enforces “first snapshot after each boundary per category” viaseen_boundaries._recategorize_snapshot_sets()then pushes those per-set category lists intoSnapshotSet.set_categories(), and is invoked after discovery, create, bulk delete (once, viarecategorize=Falseon individual deletions), and split operations. This keepsSnapshotSet.categoriesin sync with Manager state and leaves GC’sGcPolicyParamsTimelinefree to consume the categories without duplicating classification logic; any unexpected category still triggers aKeyErrorwhen GC indexeskept_by_category[...], preserving the intended fail-fast semantics. Based on learnings, this centralisesset_categories()exactly where it should live and aligns Manager and GC expectations.Also applies to: 1137-1145, 1207-1209, 1443-1447, 1529-1550, 1565-1568, 1855-1858
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
container_tests/snapm.confcontainer_tests/tests/test_schedule.pydoc/user_guide.rstman/man8/snapm.8snapm/_snapm.pysnapm/command.pysnapm/manager/_manager.pysnapm/manager/_schedule.pytests/test_manager.py
✅ Files skipped from review due to trivial changes (2)
- container_tests/snapm.conf
- doc/user_guide.rst
🚧 Files skipped from review as they are similar to previous changes (3)
- container_tests/tests/test_schedule.py
- man/man8/snapm.8
- snapm/command.py
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2026-01-03T17:05:40.850Z
Learning: In the snapm codebase, the GC evaluation logic in `snapm/manager/_schedule.py` intentionally allows KeyError when indexing `kept_by_category[category]` if invalid categories appear in `SnapshotSet.categories`. This fail-fast behavior is by design: invalid categories indicate severe bugs or API misuse, and should blow up rather than fail silently. The `set_categories()` method is forbidden for client code per its docstring.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
📚 Learning: 2026-01-03T17:05:40.850Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2026-01-03T17:05:40.850Z
Learning: In the snapm codebase, the GC evaluation logic in `snapm/manager/_schedule.py` intentionally allows KeyError when indexing `kept_by_category[category]` if invalid categories appear in `SnapshotSet.categories`. This fail-fast behavior is by design: invalid categories indicate severe bugs or API misuse, and should blow up rather than fail silently. The `set_categories()` method is forbidden for client code per its docstring.
Applied to files:
snapm/manager/_schedule.pysnapm/manager/_manager.pytests/test_manager.pysnapm/_snapm.py
📚 Learning: 2025-09-03T15:21:15.412Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
Applied to files:
snapm/manager/_schedule.pysnapm/manager/_manager.py
📚 Learning: 2025-10-22T20:06:54.993Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Applied to files:
snapm/manager/_schedule.pysnapm/manager/_manager.pytests/test_manager.pysnapm/_snapm.py
📚 Learning: 2025-09-10T23:42:35.992Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 357
File: snapm.spec:132-134
Timestamp: 2025-09-10T23:42:35.992Z
Learning: The snapm project maintainer bmr-cymru prefers scripted version bumping as part of the snapm-x.y.z-release-prep workflow rather than automated regex-based parsing for version synchronization across files.
Applied to files:
snapm/manager/_schedule.pysnapm/manager/_manager.py
📚 Learning: 2026-01-03T16:49:13.418Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2026-01-03T16:49:13.418Z
Learning: In the snapm codebase, the `report_str_list()` method in `snapm/command.py` automatically sorts string lists (using `sorted(value)`) before outputting them. Documentation examples showing category ordering (e.g., "daily, hourly") reflect this sorted output behavior and should not be changed.
Applied to files:
snapm/manager/_manager.pysnapm/_snapm.py
📚 Learning: 2025-11-12T21:27:50.414Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 596
File: snapm/_snapm.py:36-44
Timestamp: 2025-11-12T21:27:50.414Z
Learning: In the snapm codebase, the numeric values of SNAPM_DEBUG_* constants (e.g., SNAPM_DEBUG_MANAGER, SNAPM_DEBUG_MOUNTS, SNAPM_DEBUG_SCHEDULE) are not guaranteed to remain stable across versions. Consumers should always use the symbolic constant names rather than persisting numeric masks. SNAPM_DEBUG_ALL is designed to change over time as new subsystems are added, and repurposing or reassigning bit positions for debug flags is acceptable and intentional.
Applied to files:
snapm/manager/_manager.pysnapm/_snapm.py
📚 Learning: 2025-10-20T21:12:52.344Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:372-377
Timestamp: 2025-10-20T21:12:52.344Z
Learning: In snapm/manager/_mounts.py mount operations: do not create missing mount point directories on snapshot filesystems; instead, check if the directory exists with isdir(), log a warning if missing, and skip that mount to avoid modifying the snapshot filesystem unnecessarily.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-11-13T18:16:10.229Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-11-13T18:16:10.229Z
Learning: In snapshotmanager/snapm test code, timestamps in tests like `get_eighteen_months()` intentionally include jitter (non-zero seconds/microseconds) to mimic the behavior of systemd timer units with the default `AccuracySec` value of 1 minute, providing more realistic test coverage for Timeline GC logic.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-10-22T16:43:42.894Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T16:43:42.894Z
Learning: In snapm/manager/_manager.py _check_snapm_dir: the correct order for directory validation is: 1) check if path exists and validate it's a directory (not a symlink), 2) os.makedirs() to create if needed, 3) os.chmod() to fix permissions if directory existed with wrong perms, 4) validate final mode matches expected. Do not attempt chmod after makedirs for a newly created directory since makedirs(mode=...) already sets permissions. Do not log errors when chmod fails but the mode is already correct.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-09-14T02:14:30.316Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-14T02:14:30.316Z
Learning: PR #478 in snapshotmanager/snapm fixes a regression in the LVM2 plugin where `can_snapshot()` method throws `SnapmCalloutError` when probing non-device-mapper devices. The fix adds device major number validation to prevent calling dmsetup on non-dm devices, allowing proper "no provider found" error messages instead of confusing dmsetup errors.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-08-25T17:19:19.563Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-25T17:19:19.563Z
Learning: The man page changes mentioned in the AI summary for PR #383 were rebasing artifacts and should be ignored when reviewing this branch - there are no actual man page changes on the bmr-vm-tests branch.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-08-26T19:07:42.290Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-26T19:07:42.290Z
Learning: In the snapm virt_tests module, verify_lvm_setup() is intended as a lightweight sanity check for essential volumes, not comprehensive validation for every storage layout. The responsibility for testing new layout kickstart fragments lies with the developer submitting them. Adding extensive validation for each layout would be redundant and create clutter.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-08-30T13:46:15.300Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-30T13:46:15.300Z
Learning: In snapm, boom is a hard requirement because snapm.manager._boot imports boom modules directly, and the Manager class imports and uses functionality from _boot including BootCache, create_snapset_boot_entry, and delete_snapset_boot_entry. The Manager class cannot be instantiated without boom being available.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-09-09T19:56:30.429Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 454
File: man/man8/snapm.8:1433-1461
Timestamp: 2025-09-09T19:56:30.429Z
Learning: In snapm JSON output, field names always use full prefixed names (e.g., "snapset_name", "snapset_status") to avoid collisions between different object types, even though prefixes may be optional in other contexts when unambiguous. This is an intentional architectural decision for consistency and conflict prevention in JSON reports.
Applied to files:
snapm/_snapm.py
📚 Learning: 2025-08-24T16:09:15.238Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 400
File: man/man8/snapm.8:496-505
Timestamp: 2025-08-24T16:09:15.238Z
Learning: In UNIX man pages for snapm, metavars (value placeholders) should be lowercase except for UUID which should be uppercase. For example, use `schedule_name` not `SCHEDULE_NAME`, but use `UUID` not `uuid`.
Applied to files:
snapm/_snapm.py
🧬 Code graph analysis (3)
snapm/manager/_schedule.py (1)
snapm/_snapm.py (3)
SnapshotSet(1006-1565)set_categories(1311-1322)categories(1301-1309)
snapm/manager/_manager.py (1)
snapm/_snapm.py (11)
datetime(1176-1180)SnapshotSet(1006-1565)timestamp(1169-1173)timestamp(1710-1714)snapshot_set(1739-1743)snapshot_set(1746-1750)set_categories(1311-1322)name(1140-1144)name(1659-1663)uuid(1162-1166)uuid(1666-1670)
tests/test_manager.py (1)
snapm/manager/_manager.py (13)
_find_mount_point_for_devpath(314-325)_check_snapm_runtime_dir(328-356)_categorize_snapshot_sets(537-615)Scheduler(618-920)create(705-755)enable(853-865)start(868-878)_get_schedule_by_name(667-677)edit(758-834)stop(881-891)disable(894-904)delete(837-850)gc(907-920)
⏰ 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). (32)
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: python-basic-tests
🔇 Additional comments (8)
snapm/_snapm.py (1)
11-11: SnapshotSet category constants and API look consistent and safe to exposeThe new
SNAPSET_CATEGORIES/SNAPSET_TIMELINE_CATEGORIESconstants, backing field_categories, thecategoriesread-only copy, andset_categories()mutator are wired cleanly into__str__,to_dict(), and__all__. Returning a list copy prevents external mutation, and centralising writes throughset_categories()keeps the Manager as the sole categorisation authority, which matches the intended “manager-only” contract for categories used by GC. Based on learnings, this aligns with the fail-fast GC behaviour around invalid categories.Also applies to: 128-129, 152-160, 1037-1045, 1063-1072, 1090-1108, 1300-1323, 2368-2387
snapm/manager/_schedule.py (1)
11-11: Timeline GC now correctly consumesSnapshotSet.categoriesand canonical category setUsing
SNAPSET_TIMELINE_CATEGORIESto buildclassified_setsand to drive logging avoids the earlier misuse ofSNAPSET_CATEGORIES, and folding insnapshot_set.categoriespreserves the intended “keep if any category votes keep, delete only if all vote delete” semantics. Becausekept_by_categoryonly defines known timeline keys, the comprehension oversset_catswill still raiseKeyErroron any unknown category, so the desired fail-fast behaviour for invalidSnapshotSet.categoriesis intact. Based on learnings, this matches the GC design.Also applies to: 23-29, 343-355, 364-371, 372-402, 404-410, 417-433
tests/test_manager.py (5)
10-22: New imports for datetime and_managerare appropriateImporting
datetimeand aliasingsnapm.manager._managerfor direct helper access keeps the new tests focused on the internal functions without over-reaching into higher-level APIs. No issues here.
152-166: Mount-point resolution test is well-isolatedPatching
builtins.open,_manager.exists, and_manager.samefilegives a deterministic harness for_find_mount_point_for_devpath, and the two assertions cover both “match found” and “no match” paths without hitting the real/proc/mountsor filesystem. This is a good, focused unit test for the helper.
1146-1209: Timeline categorisation test exercises key boundary cases
TestTimelineCategorization.test__categorize_snapshot_setscovers the important boundaries (year start, quarter start, month start, Monday weekly boundary, same-day hourly classification) and asserts the multi-category membership and non-membership expectations directly against the reverse map returned by_categorize_snapshot_sets. This aligns well with the documented rules for yearly/quarterly/monthly/weekly/daily/hourly and gives good regression protection around the boundary logic.
1211-1311: SchedulerTests provide solid coverage of schedule lifecycle and integration pointsThe new
SchedulerTestsclass validates creation (including duplicate detection), editing, deletion, and the enable/disable/start/stop/gc proxies using a temporary directory and patchedScheduleclass, so no real on-disk schedules are touched. This is a sensible way to exerciseSchedulerbehaviour in isolation while still driving parameter validation via_parse_source_specsand the Manager mock.
167-193: Fix decorator argument ordering fortest_check_snapm_runtime_dirWith three
@patch(...)decorators, the test function currently only declares two mock parameters, so unittest will call it with too many arguments and the test will error before exercising_check_snapm_runtime_dir(). You should either drop the constant patch from the decorator stack or accept the extra argument.For example, if you want to keep all three decorators, adjust the signature to match the patch order (bottom decorator first argument, then upwards):
Suggested fix
- @patch("snapm.manager._manager.SNAPM_RUNTIME_DIR", "/run/snapm_test") - @patch("snapm.manager._manager.exists") - @patch("os.lstat") - def test_check_snapm_runtime_dir(self, mock_lstat, mock_exists): + @patch("snapm.manager._manager.SNAPM_RUNTIME_DIR", "/run/snapm_test") + @patch("snapm.manager._manager.exists") + @patch("os.lstat") + def test_check_snapm_runtime_dir(self, mock_lstat, mock_exists, _mock_runtime_dir): """Test validation of the runtime directory security."""Alternatively, you can patch
SNAPM_RUNTIME_DIRinside the test body with a context manager and keep only the two mocks in the signature.⛔ Skipped due to learnings
Learnt from: bmr-cymru Repo: snapshotmanager/snapm PR: 0 File: :0-0 Timestamp: 2025-12-21T21:15:22.354Z Learning: In Python test files using patch decorators, mock parameters that are not used in the test body should be prefixed with "_" (e.g., _mock_obj) rather than removed. The patch decorator injects these parameters regardless of usage, so they must remain in the function signature.Learnt from: bmr-cymru Repo: snapshotmanager/snapm PR: 0 File: :0-0 Timestamp: 2025-10-22T16:43:42.894Z Learning: In snapm/manager/_manager.py _check_snapm_dir: the correct order for directory validation is: 1) check if path exists and validate it's a directory (not a symlink), 2) os.makedirs() to create if needed, 3) os.chmod() to fix permissions if directory existed with wrong perms, 4) validate final mode matches expected. Do not attempt chmod after makedirs for a newly created directory since makedirs(mode=...) already sets permissions. Do not log errors when chmod fails but the mode is already correct.Learnt from: bmr-cymru Repo: snapshotmanager/snapm PR: 490 File: snapm/manager/plugins/lvm2.py:618-629 Timestamp: 2025-09-24T02:49:21.752Z Learning: When API changes affect return types (like changing from int to tuple), all supporting infrastructure needs updating including mock scripts in tests, not just the call sites that consume the API.snapm/manager/_manager.py (1)
13-23: Runtime/mount/lock directory helper matches the intended security and creation flow
_check_snapm_dir()follows the documented pattern: validate an existing path (directory, not symlink),makedirs(..., exist_ok=True)to create if missing, conditionallychmodonly when the directory pre-existed with a different mode, and finally verify the resulting permissions._check_lock_dir()and_check_mounts_dir()then reuse this helper with their respective modes and labels, andManager.__init__surfaces failures asSnapmSystemError. This is consistent with the earlier guidance on secure runtime directories and avoids redundant chmod on freshly created paths. Based on learnings, this is the expected behaviour.Also applies to: 32-32, 359-411, 414-535
Make
SnapshotSettimeline categorization a first-class feature, exposed asSnapshotSet.categories, and addlistandshowfields to display the value.Resolves: #622
Resolves: #882
Resolves: #883
Resolves: #884
Summary by CodeRabbit
New Features
Changes
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.