asf-tooling commented on issue #851:
URL:
https://github.com/apache/tooling-trusted-releases/issues/851#issuecomment-4407550831
<!-- gofannon-issue-triage-bot v2 -->
**Automated triage** — analyzed at `main@751c2146`
**Type:** `discussion` • **Classification:** `no_action` •
**Confidence:** `high`
**Application domain(s):** `release_lifecycle`, `automated_checks`
### Summary
This issue began as a request to add a confirmation step for release
managers ignoring non-blocking errors, but has evolved into a design discussion
about reclassifying the entire check result type system. @sbp proposed renaming
result types from success/error/blocker/exception to
pass/suggestion/concern/blocker/exception, and adding a 'selective strict mode'
(inverse of ignores). @dave2wave noted more discussion is needed and suggested
'dismiss' as better terminology. The team decided to draw up a reclassification
table for review but no implementation decision has been reached.
### Where this lives in the code today
#### `atr/tasks/checks/__init__.py` — `Recorder` (lines 256-282)
_currently does this_
The Recorder class has methods for each current check result type (success,
warning, failure, blocker, exception) - any reclassification would modify these
methods.
```python
async def blocker(
self,
message: str,
data: Any,
primary_rel_path: safe.RelPath | None = None,
member_rel_path: str | None = None,
) -> sql.CheckResult:
result = await self._add(
sql.CheckResultStatus.BLOCKER,
message,
data,
primary_rel_path=primary_rel_path,
member_rel_path=member_rel_path,
)
return result
async def exception(
...
async def failure(
...
async def success(
...
async def warning(
...
```
#### `atr/storage/readers/checks.py` — `GeneralPublic.__check_ignore_match`
(lines 105-122)
_currently does this_
Shows the current ignore logic where SUCCESS and BLOCKER are never ignored -
a 'selective strict mode' or reclassification would need to modify this
boundary logic.
```python
def __check_ignore_match(self, cr: sql.CheckResult, cri:
sql.CheckResultIgnore) -> bool:
# Does not check that the project name matches
if cr.status == sql.CheckResultStatus.SUCCESS:
# Successes are never ignored
return False
if cr.status == sql.CheckResultStatus.BLOCKER:
# Blockers are never ignored
return False
if cri.release_glob is not None:
if not self.__check_ignore_match_pattern(cri.release_glob,
str(cr.release_key)):
return False
if cri.revision_number is not None:
if cri.revision_number != cr.revision_number:
return False
if cri.checker_glob is not None:
if not self.__check_ignore_match_pattern(cri.checker_glob,
cr.checker):
return False
return self.__check_ignore_match_2(cr, cri)
```
#### `atr/shared/ignores.py` — `IgnoreStatus` (lines 35-41)
_currently does this_
Current ignore status enum maps to existing check result statuses - would
need updating if result types are reclassified.
```python
class IgnoreStatus(enum.Enum):
"""Wrapper enum for ignore status."""
NO_STATUS = "-"
EXCEPTION = "Exception"
FAILURE = "Failure"
WARNING = "Warning"
```
### Proposed approach
This issue is still in active design discussion. The team has not converged
on an implementation approach. The original idea of a confirmation checkbox was
critiqued by @sbp as ineffective. The current proposal involves reclassifying
check result types from success/warning/failure/blocker/exception to
pass/suggestion/concern/blocker/exception, plus adding a 'selective strict
mode' (inverse of ignores that promotes concerns to blockers). @dave2wave
suggested 'dismiss' as terminology. The team decided to produce a
reclassification table for review before proceeding.
No diff should be proposed at this time. The changes would touch the SQL
model enum (CheckResultStatus), the Recorder class methods, ignore matching
logic, statistics computation, and all UI rendering of check results - but the
exact new taxonomy hasn't been agreed upon.
### Open questions
- What is the final agreed taxonomy of check result types?
(pass/suggestion/concern/blocker/exception vs other options)
- What does the reclassification table look like - which existing checks map
to which new types?
- Should 'selective strict mode' be implemented alongside reclassification
or separately?
- Is 'dismiss' (per @dave2wave) the preferred term for the
ignore/acknowledge action?
- Does a database migration need to handle existing check results with the
old status values?
- Where is sql.CheckResultStatus defined and what are its current enum
values? (not in provided files)
_The agent reviewed this issue and is not proposing patches in this run.
Review the existing-code citations and open questions above before deciding
next steps._
### Files examined
- `atr/shared/ignores.py`
- `atr/storage/writers/checks.py`
- `atr/post/ignores.py`
- `atr/storage/readers/checks.py`
- `atr/get/checks.py`
- `atr/tasks/checks/__init__.py`
- `atr/get/compose.py`
- `atr/get/ignores.py`
---
*Draft from a triage agent. A human reviewer should validate before merging
any change. The agent did not run tests or verify diffs apply.*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
