snapm: make --yes/--no required and mutually exclusive for autoactivate#461
Merged
snapm: make --yes/--no required and mutually exclusive for autoactivate#461
Conversation
WalkthroughRefactored CLI parsing to require a mutually exclusive choice between --yes and --no for autoactivation; handlers now read a single boolean Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant CLI as snapm (argparse)
participant H_set as _autoactivate_cmd
participant H_snap as _snapshot_autoactivate_cmd
rect #F0FAFF
note over CLI: Required mutually exclusive flags (--yes / --no)
U->>CLI: snapm snapset autoactivate --yes | --no
CLI->>H_set: cmd_args.autoactivate = true|false
H_set->>H_set: set_autoactivate(cmd_args.autoactivate)
end
rect #F7FFF2
U->>CLI: snapm snapshot autoactivate --yes | --no
CLI->>H_snap: cmd_args.autoactivate = true|false
H_snap->>H_snap: apply per-snapshot autoactivate(cmd_args.autoactivate)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (45)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
snapm/command.py (1)
1748-1759: Good fix: required mutually exclusive --yes/--no mapped to one boolean.This cleanly enforces exclusivity and fixes the ignored --no bug.
Consider renaming the destination from "yes" to a semantic name like "autoactivate" and neutralizing help text for reuse across snapset/snapshot:
yes_no_mutex_group = parser.add_mutually_exclusive_group(required=True) -yes_no_mutex_group.add_argument( - "--yes", - action="store_true", - help="Enable snapshot autoactivation", -) +yes_no_mutex_group.add_argument( + "--yes", + dest="autoactivate", + action="store_true", + help="Enable autoactivation", +) yes_no_mutex_group.add_argument( "--no", - action="store_false", - dest="yes", - help="Disable snapshot autoactivation", + action="store_false", + dest="autoactivate", + help="Disable autoactivation", )And update handlers accordingly (outside this hunk):
# in _autoactivate_cmd auto = bool(cmd_args.autoactivate) # in _snapshot_autoactivate_cmd auto = bool(cmd_args.autoactivate)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
man/man8/snapm.8(2 hunks)snapm/command.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T18:27:31.307Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-22T18:27:31.307Z
Learning: The snapm project only supports GNU Groff markup for man pages and does not support other troff toolchains. All man page formatting should follow GNU Groff standards and practices.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-08-26T13:22:28.829Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#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
⏰ 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). (40)
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-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:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-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:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-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: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- 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-thin, fedora41)
- GitHub Check: python-basic-tests
🔇 Additional comments (2)
man/man8/snapm.8 (2)
244-244: Docs match CLI: --yes/--no now required and exclusive.Removing the brackets correctly reflects the new required mutually exclusive flags. Groff usage with RB here is consistent with project style.
296-296: Consistent update for snapshot autoactivate.Same approval as for snapset; synopsis now accurately communicates the requirement.
bmr-cymru
force-pushed
the
bmr-autoactivate-yes-no
branch
from
da82a3b to
6baa9ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
snapm/command.py (2)
1277-1280: Redundant bool() cast.autoactivate is already a boolean from argparse; the cast is unnecessary.
- auto = bool(cmd_args.autoactivate) + auto = cmd_args.autoactivate
1382-1387: Log the chosen state for parity with snapset handler (and drop redundant bool()).Include the resulting yes/no in the log for easier auditing.
- auto = bool(cmd_args.autoactivate) + auto = cmd_args.autoactivate @@ - _log_info("Set autoactivation status for %d snapshots", count) + _log_info("Set autoactivate=%s for %d snapshots", bool_to_yes_no(auto), count)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
man/man8/snapm.8(2 hunks)snapm/command.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- man/man8/snapm.8
⏰ 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). (45)
- GitHub Check: rpm-build:epel-9-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-10-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-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-42-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-41-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-41-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, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: python-basic-tests
bmr-cymru
force-pushed
the
bmr-autoactivate-yes-no
branch
from
6baa9ba to
561ce12
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
snapm/command.py (1)
1748-1760: Unified, required --yes/--no is correct; dest fixed. Please verify no leftovers.Both flags now write to
autoactivateand are mutually exclusive/required. This addresses the original bug and aligns the handlers.Run to ensure no remaining references to
cmd_args.yes/cmd_args.noor stray dests:#!/bin/bash set -euo pipefail rg -nP -C2 '\bcmd_args\.(yes|no)\b|dest\s*=\s*"(yes|no)"' --type=py
🧹 Nitpick comments (2)
snapm/command.py (2)
1277-1280: Nit: drop redundant bool() cast.
argparsealready produces a bool forstore_true/false.- auto = bool(cmd_args.autoactivate) + auto = cmd_args.autoactivate
1382-1387: Nit: minor cleanup + make log consistent with snapset handler.
- Remove redundant bool() cast.
- Include the yes/no in the log like snapset does.
- auto = bool(cmd_args.autoactivate) + auto = cmd_args.autoactivate @@ - _log_info("Set autoactivation status for %d snapshots", count) + _log_info("Set autoactivate=%s for %d snapshots", bool_to_yes_no(auto), count)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
man/man8/snapm.8(2 hunks)snapm/command.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- man/man8/snapm.8
🧰 Additional context used
🧬 Code graph analysis (1)
snapm/command.py (3)
snapm/_snapm.py (4)
autoactivate(995-1001)autoactivate(1004-1016)autoactivate(1515-1520)autoactivate(1523-1530)snapm/manager/plugins/stratis.py (1)
autoactivate(253-255)snapm/manager/plugins/lvm2.py (1)
autoactivate(317-322)
⏰ 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). (45)
- 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:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-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: 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-41-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-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-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-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- 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, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: python-basic-tests
Resolves: #426 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
bmr-cymru
force-pushed
the
bmr-autoactivate-yes-no
branch
from
561ce12 to
17548fe
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves: #426
Summary by CodeRabbit
Bug Fixes
Documentation