Add Claude AI code review workflow#14708
Add Claude AI code review workflow#14708AviadP wants to merge 1 commit intored-hat-storage:masterfrom
Conversation
Signed-off-by: Aviadp <apolak@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AviadP The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
|
||
| These violate critical project rules and must always be flagged: | ||
|
|
||
| - `except Exception` or bare `except:` — must use specific exceptions |
There was a problem hiding this comment.
l don't see this as blocker, sometime we use this.
We definitely should not allow diabolic anti pattern like:
except Exception:
pass
Better to always use specific exception whenever it's possible, but sometimes there are so many possibilities that catching generic Exception is only one solution to avoid many different unpredictable exceptions.
There was a problem hiding this comment.
the tool should only add comments, not change requests, so it is not real blocker. using braod exceptions makes debug much harder and sometimes lets test that should fail to pass
| max-files: '40' | ||
| context-intro: > | ||
| You are reviewing a PR in ocs-ci — a Python/pytest testing framework | ||
| for Red Hat OpenShift Container Storage (OCS/ODF). The codebase uses |
There was a problem hiding this comment.
I would change to something like this
| for Red Hat OpenShift Container Storage (OCS/ODF). The codebase uses | |
| for Red Hat OpenShift Data Foundation (ODF) formerly known as OpenShift Container Storage OCS. The codebase uses |
| has strict conventions for locator placement, exception handling, and | ||
| test teardown. Read the project CLAUDE.md for full conventions. | ||
| critical-rules: | | ||
| 1. Never use `except Exception` or bare `except:` — always use specific exceptions |
There was a problem hiding this comment.
As pointed bellow , this probably should not be a blocker. But if it's used we should log the exception out:
logger.error("Exception hit:", exc_info=True)
something like this. So if something will debugged we see what is happening.
| critical-rules: | | ||
| 1. Never use `except Exception` or bare `except:` — always use specific exceptions | ||
| 2. UI locators MUST be centralized in `ocs_ci/ocs/ui/views.py` (unless the locator contains a variable) | ||
| 3. Fixtures MUST NOT use `yield` — use `request.addfinalizer()` instead |
There was a problem hiding this comment.
why yield is not acceptable? I think it's a valid scenario which we use
There was a problem hiding this comment.
|
|
||
| - `except Exception` or bare `except:` — must use specific exceptions | ||
| - UI locators defined outside `ocs_ci/ocs/ui/views.py` (unless the locator contains a variable) | ||
| - Fixtures using `yield` — must use `request.addfinalizer()` instead |
There was a problem hiding this comment.
Pointed out above, this is valid case to use yield
2 participants
claude-reviewlabel or@claudecommentto decide:
review tool link