asf-tooling commented on issue #1182:
URL:
https://github.com/apache/tooling-trusted-releases/issues/1182#issuecomment-4409751023
<!-- gofannon-issue-triage-bot v2 -->
**Automated triage** — analyzed at `main@2da7807a`
**Type:** `refactor` • **Classification:** `actionable` •
**Confidence:** `medium`
**Application domain(s):** `release_lifecycle`
### Summary
The issue asks to disallow phase transitions from non-latest revisions. @sbp
clarified this only applies to the compose→vote transition (finish no longer
has revisions). The backend in `_start_vote_no_commit` already has a check
rejecting non-latest revisions (raising a 409), but the `promote_to_candidate`
method still accepts a `selected_revision_number` parameter, and presumably the
UI still presents the option. The simplification would remove the ability to
select a revision for promotion and always use the latest.
### Where this lives in the code today
#### `atr/storage/writers/release.py` —
`CommitteeParticipant.promote_to_candidate` (lines 450-472)
_needs modification_
This method accepts a `selected_revision_number` parameter that the issue
suggests removing in favor of always using the latest revision internally.
```python
async def promote_to_candidate(
self,
release_key: safe.ReleaseKey,
selected_revision_number: safe.RevisionNumber,
*,
allowed_vote_modes: frozenset[sql.VoteMode],
) -> str | None:
"""Promote a release candidate draft to a new phase."""
try:
await self.__data.begin_immediate()
release, vote_seq, vote_mode = await self._start_vote_no_commit(
release_key,
selected_revision_number,
allowed_vote_modes=allowed_vote_modes,
promote=True,
)
await self.__data.commit()
except storage.AccessError as e:
await self.__data.rollback()
return str(e)
except Exception:
await self.__data.rollback()
raise
```
#### `atr/storage/writers/release.py` —
`CommitteeParticipant._start_vote_no_commit` (lines 483-503)
_currently does this_
This method already contains the enforcement check rejecting promotions from
non-latest revisions. The issue's simplification would make this check
unnecessary for the promote=True path by always using the latest revision
internally.
```python
async def _start_vote_no_commit( # noqa: C901
self,
release_key: safe.ReleaseKey,
selected_revision_number: safe.RevisionNumber,
*,
allowed_vote_modes: frozenset[sql.VoteMode],
promote: bool,
expected_podling_thread_id: str | None = None,
) -> tuple[sql.Release, int, sql.VoteMode]:
release_for_pre_checks = await self.__data.release(
key=str(release_key), _project=True, _committee=True,
_project_release_policy=True
).demand(storage.AccessError("Release candidate draft not found",
status=404))
project_key = release_for_pre_checks.safe_project_key
version_key = release_for_pre_checks.safe_version_key
revision_number = release_for_pre_checks.safe_latest_revision_number
...
# Check that the revision number is the latest
if revision_number != selected_revision_number:
raise storage.AccessError(
"The selected revision number does not match the latest
revision number", status=409
)
```
### Proposed approach
The simplification involves modifying `promote_to_candidate` to no longer
accept a `selected_revision_number` parameter, instead internally looking up
the latest revision number. This eliminates the need for the UI to pass a
revision number and removes the possibility of even attempting to promote from
a non-latest revision. The `_start_vote_no_commit` method would still receive
the revision number for its optimistic concurrency check in the SQL UPDATE
WHERE clause (preventing races), but it would derive it from
`release_for_pre_checks.safe_latest_revision_number` rather than from user
input when `promote=True`.
Since I cannot see the caller of `promote_to_candidate` (likely in a POST
handler for vote/draft operations) or the template that renders the promote
button, I cannot provide a complete diff. The core change is in
`_start_vote_no_commit`: when `promote=True`, ignore the
`selected_revision_number` parameter and use `revision_number` (the latest)
directly. Then update callers to not need to pass a revision number for
promotion.
### Suggested patches
#### `atr/storage/writers/release.py`
When promoting (compose→vote), always use the latest revision number instead
of requiring the caller to supply it, simplifying the interface and preventing
any possibility of non-latest promotion.
````diff
--- a/atr/storage/writers/release.py
+++ b/atr/storage/writers/release.py
@@ -280,7 +280,7 @@
async def promote_to_candidate(
self,
release_key: safe.ReleaseKey,
- selected_revision_number: safe.RevisionNumber,
+ selected_revision_number: safe.RevisionNumber | None = None,
*,
allowed_vote_modes: frozenset[sql.VoteMode],
) -> str | None:
@@ -330,6 +330,12 @@
revision_number = release_for_pre_checks.safe_latest_revision_number
if promote:
vote_mode = release_for_pre_checks.effective_vote_mode
+ # When promoting, always use the latest revision - disallow
promotion
+ # from any revision other than the latest
+ if selected_revision_number is None:
+ selected_revision_number = revision_number
+ elif revision_number != selected_revision_number:
+ raise storage.AccessError(
+ "Phase transitions are only allowed from the latest
revision", status=409
+ )
else:
vote_mode = release_for_pre_checks.vote_mode
if vote_mode is None:
@@ -350,11 +356,6 @@
if promote:
raise storage.AccessError("This release is not in the
candidate draft phase", status=409)
raise storage.AccessError("The release state has changed,
please refresh and try again", status=409)
- # Check that the revision number is the latest
- if revision_number != selected_revision_number:
- raise storage.AccessError(
- "The selected revision number does not match the latest
revision number", status=409
- )
if await interaction.has_blocker_checks(
release_for_pre_checks, selected_revision_number,
caller_data=self.__data
````
### Open questions
- Where is `promote_to_candidate` called from? I don't see the POST handler
that starts a vote (likely in `atr/post/draft.py` or similar). The caller also
needs to be updated to stop passing a user-selected revision number.
- Is there a template that renders a revision selector for the
promote/start-vote button? If so, that UI element should be removed or hidden.
- The `_start_vote_no_commit` method is also called with `promote=False`
(for re-votes). Does the non-latest check need to be preserved for that path?
Based on the issue, it seems only `promote=True` is affected.
- The SQL UPDATE uses `sql.latest_revision_number_query() ==
str(selected_revision_number)` as an optimistic concurrency guard. This should
be kept regardless, using the internally-derived latest revision number.
### Files examined
- `atr/post/compose.py`
- `atr/post/finish.py`
- `atr/storage/writers/release.py`
- `atr/storage/writers/revision.py`
- `atr/post/revisions.py`
- `atr/shared/finish.py`
- `atr/get/compose.py`
- `atr/get/finish.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]
