Skip to content

Conversation

@mulkieran
Copy link
Member

@mulkieran mulkieran commented Jan 6, 2026

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Fixed volume creation failure that occurred when creating multiple volumes
    • Improved error message handling for property setting operations
  • Chores

    • Updated build automation configuration

✏️ Tip: You can customize this high-level summary in your review settings.

@mulkieran mulkieran added this to the v3.9.0 milestone Jan 6, 2026
@mulkieran mulkieran self-assigned this Jan 6, 2026
@mulkieran mulkieran moved this to In Progress in 2026January Jan 6, 2026
@mulkieran mulkieran force-pushed the additional-zbus-interfaces branch 2 times, most recently from 23aa537 to 0045d67 Compare January 7, 2026 01:18
@mulkieran
Copy link
Member Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Jan 7, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo dnf install -y 'dnf*-command(copr)'
  • dnf copr enable packit/stratis-storage-stratis-cli-1251-copr_pull
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@coderabbitai
Copy link

coderabbitai bot commented Jan 7, 2026

Walkthrough

Configuration and error handling adjustments: removed manual trigger setting from copr_build job, eliminated DPClientSetPropertyContext-specific error message handling, and updated test expectations for volume creation from failure to success.

Changes

Cohort / File(s) Change Summary
Configuration
.packit.yaml
Removed manual_trigger: true from copr_build job; job now responds solely to PR-based triggering
Error Handling
src/stratis_cli/_error_reporting.py
Removed dedicated error message construction block for DPClientSetPropertyContext property-setting failures in _interpret_errors_2; removed inline pragma comment from closing parenthesis
Test Update
tests/integration/logical/test_create.py
Reversed test expectation in test_creation from expecting failure to success; replaced error assertion with command execution via TEST_RUNNER

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly references the main change: updating error reporting to handle org.freedesktop.zbus.Error. This aligns with the primary modification in src/stratis_cli/_error_reporting.py where zbus error matching was adjusted.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In @src/stratis_cli/_error_reporting.py:
- Around line 288-290: Tests don't exercise the branch where dbus_name ==
"org.freedesktop.zbus.Error" with a DPClientSetPropertyContext, so either re-add
the "# pragma: no cover" (with a short comment explaining why this zbus.Error
path is untested/untestable) to the conditional handling or add unit tests that
simulate DPClientSetPropertyContext raising/containing
"org.freedesktop.zbus.Error" and assert the handling code is executed;
specifically update the branch that checks dbus_name and
DPClientSetPropertyContext (symbols: dbus_name, DPClientSetPropertyContext,
"org.freedesktop.zbus.Error") to include the pragma+explanatory comment or
create a test that constructs a DPClientSetPropertyContext and triggers the zbus
error so coverage includes that branch.
🧹 Nitpick comments (1)
tests/integration/logical/test_create.py (1)

49-49: Consider using unpacking syntax for list construction.

The list concatenation can be simplified using the unpacking syntax for better readability.

Based on static analysis hints.

♻️ Proposed refactor
-        command_line = self._MENU + [self._POOLNAME] + self._VOLNAMES
+        command_line = [*self._MENU, self._POOLNAME, *self._VOLNAMES]
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d17f3c0 and f55b443.

📒 Files selected for processing (3)
  • .packit.yaml
  • src/stratis_cli/_error_reporting.py
  • tests/integration/logical/test_create.py
💤 Files with no reviewable changes (1)
  • .packit.yaml
🧰 Additional context used
🪛 Ruff (0.14.10)
tests/integration/logical/test_create.py

49-49: Consider [*self._MENU, self._POOLNAME, *self._VOLNAMES] instead of concatenation

Replace with [*self._MENU, self._POOLNAME, *self._VOLNAMES]

(RUF005)

⏰ 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). (15)
  • GitHub Check: osh-diff-scan:fedora-rawhide-x86_64:copr_pull
  • GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
  • GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
  • GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
  • GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
  • GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
  • GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
  • GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
  • GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
  • GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
  • GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
  • GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
  • GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
  • GitHub Check: test-runs (3.12)
  • GitHub Check: test-runs (3.13)

Comment on lines 47 to 50
Creation of two volumes at once should succeed.
"""
command_line = self._MENU + [self._POOLNAME] + self._VOLNAMES
self.check_error(StratisCliEngineError, command_line, _ERROR)
TEST_RUNNER(command_line)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's read the test file to see lines 47-50 and 109-110
cd tests/integration/logical && wc -l test_create.py

Repository: stratis-storage/stratis-cli

Length of output: 89


🏁 Script executed:

# Read the relevant sections of the test file
sed -n '40,115p' tests/integration/logical/test_create.py

Repository: stratis-storage/stratis-cli

Length of output: 2641


🏁 Script executed:

# Search for comments about filesystem/volume creation restrictions in the entire repository
rg -n -C3 -i 'only one filesystem|one volume per request|single filesystem per'

Repository: stratis-storage/stratis-cli

Length of output: 1257


Verify the actual restriction status and update the contradictory code accordingly.

The contradiction identified is real: Create3TestCase.test_creation() creates two volumes in a single call, but Create5TestCase.setUp() explicitly splits volume creation into separate calls with the comment "only one filesystem per request is currently allowed." This inconsistency indicates either:

  1. The restriction has been lifted and the comment at lines 109-110 (and similar comment in test_destroy.py line 85) should be removed.
  2. The test at lines 47-50 is incorrect and should split the volume creation to match the documented restriction.

Clarify which scenario applies and update the code and comments accordingly.

🧰 Tools
🪛 Ruff (0.14.10)

49-49: Consider [*self._MENU, self._POOLNAME, *self._VOLNAMES] instead of concatenation

Replace with [*self._MENU, self._POOLNAME, *self._VOLNAMES]

(RUF005)

@mulkieran
Copy link
Member Author

mulkieran commented Jan 7, 2026

Actionable comments posted: 2
🤖 Fix all issues with AI agents

🧹 Nitpick comments (1)

tests/integration/logical/test_create.py (1)> 49-49: Consider using unpacking syntax for list construction.

The list concatenation can be simplified using the unpacking syntax for better readability.
Based on static analysis hints.

♻️ Proposed refactor

-        command_line = self._MENU + [self._POOLNAME] + self._VOLNAMES
+        command_line = [*self._MENU, self._POOLNAME, *self._VOLNAMES]

📜 Review details

@coderabbitai I don't like the unpacking syntax, it doesn't look type-correct.

@coderabbitai
Copy link

coderabbitai bot commented Jan 7, 2026

Tip

For best results, initiate chat on the files or code changes.

@mulkieran, understood! I'll respect your preference for the list concatenation syntax. Both approaches are functionally equivalent and type-correct in Python, but if you find the concatenation style clearer for your codebase, that's perfectly reasonable.


✏️ Learnings added
Learnt from: mulkieran
URL: https://github.com/stratis-storage/stratis-cli/pull/1251

Timestamp: 2026-01-07T17:35:02.787Z
Learning: In the stratis-cli Python project, prefer list concatenation syntax (e.g., `self._MENU + [self._POOLNAME] + self._VOLNAMES`) over unpacking syntax (e.g., `[*self._MENU, self._POOLNAME, *self._VOLNAMES]`) for list construction.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@mulkieran mulkieran force-pushed the additional-zbus-interfaces branch 2 times, most recently from 3527c55 to 031d1cf Compare January 8, 2026 19:59
At present, the zbus D-Bus error is the only error that can be returned
when setting a property.

Signed-off-by: mulhern <[email protected]>
@mulkieran mulkieran force-pushed the additional-zbus-interfaces branch from 031d1cf to a30484d Compare January 15, 2026 15:17
@mulkieran
Copy link
Member Author

ignore failure...

@mulkieran mulkieran removed this from the v3.9.0 milestone Jan 15, 2026
@mulkieran
Copy link
Member Author

Too small to keep, closing.

@mulkieran mulkieran closed this Jan 15, 2026
@mulkieran mulkieran deleted the additional-zbus-interfaces branch January 15, 2026 17:16
@github-project-automation github-project-automation bot moved this from In Progress to Done in 2026January Jan 15, 2026
@mulkieran mulkieran removed this from 2026January Jan 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant