Skip to content

Conversation

@liangxin1300
Copy link
Collaborator

@liangxin1300 liangxin1300 commented May 1, 2025

#1679 only updated some sanity check functions and failed to change all related return values to use VerifyResult.

This PR ensures that all related functions now consistently use VerifyResult as the return value.

@liangxin1300 liangxin1300 force-pushed the 20250501_verify_result branch from 28ab757 to ab009e0 Compare May 1, 2025 12:24
@liangxin1300 liangxin1300 changed the title Dev: cibconfig: Use VerifyResult to define the return value of check_sanity function Dev: cibconfig: Use VerifyResult to define the return value of sanity check related functions May 1, 2025
@liangxin1300 liangxin1300 marked this pull request as ready for review May 6, 2025 01:45
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates several sanity check functions and their related tests to consistently return values of type VerifyResult instead of plain integers. Key changes include updating test assertions in the unit tests, modifying the functions in crmsh/ra.py to return VerifyResult (with type annotations) instead of numeric codes, and updating various return statements in crmsh/cibconfig.py accordingly.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
test/unittests/test_bugs.py Updated sanity check assertions to compare against VerifyResult values and added a utils import.
crmsh/ra.py Replaced numeric return codes with VerifyResult constants and added proper type annotations.
crmsh/cibconfig.py Modified functions to consistently return VerifyResult and updated bitwise flag usage.
Comments suppressed due to low confidence (1)

crmsh/ra.py:1216

  • Ensure that the VerifyResult type supports bitwise operations and guarantees predictable results when combined. If not, consider implementing a dedicated method for combining or evaluating verify statuses.
rc = VerifyResult.WARNING

c = factory.find_object('coloc-p1-p2')
assert c and c.check_sanity() > 0
rc = c.check_sanity()
assert c and bool(rc) is True and utils.VerifyResult.WARNING in rc
Copy link

Copilot AI May 6, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider replacing the membership check ('in') with an explicit bitwise or equality comparison to clearly express the expected VerifyResult flag. This will improve clarity for maintainers unfamiliar with the VerifyResult type’s internals.

Suggested change
assert c and bool(rc) is True and utils.VerifyResult.WARNING in rc
assert c and bool(rc) is True and (rc & utils.VerifyResult.WARNING) == utils.VerifyResult.WARNING

Copilot uses AI. Check for mistakes.
Comment on lines +502 to 504
rc = VerifyResult.SUCCESS
for param, resources in list(clash_dict.items()):
# at least one new object must be involved
Copy link

Copilot AI May 6, 2025

Choose a reason for hiding this comment

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

[nitpick] Add inline documentation or comments to explain the use of VerifyResult as return values in these functions and how to interpret combined flags. This will aid future maintainers in understanding the intended logic.

Suggested change
rc = VerifyResult.SUCCESS
for param, resources in list(clash_dict.items()):
# at least one new object must be involved
# Initialize the return code as SUCCESS. This will be updated to WARNING
# if any uniqueness violations are detected involving new objects.
rc = VerifyResult.SUCCESS
for param, resources in list(clash_dict.items()):
# At least one new object must be involved in the clash for a WARNING.

Copilot uses AI. Check for mistakes.
liangxin1300 added a commit that referenced this pull request May 6, 2025
@liangxin1300 liangxin1300 merged commit 7ca341f into ClusterLabs:master May 6, 2025
32 checks passed
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.

2 participants