sbp commented on code in PR #1248:
URL:
https://github.com/apache/tooling-trusted-releases/pull/1248#discussion_r3261348976
##########
atr/storage/writers/release.py:
##########
@@ -1048,6 +1053,85 @@ def __init__(
self.__asf_uid = asf_uid
self.__committee_key = committee_key
+ async def archive(
+ self,
+ project_key: safe.ProjectKey,
+ version_key: safe.VersionKey,
+ ) -> str | None:
+ """Archive a published release.
+
+ Marks the release as archived, removes its hard-linked files from the
+ downloads area, and records an ARCHIVE lifecycle event. Returns an
+ error string if the release can't be archived, or None on success.
+ SVN-side handoff to archive.apache.org is not yet implemented.
+ """
+ via = sql.validate_instrumented_attribute
+ release = await self.__data.release(
+ project_key=str(project_key),
+ version=str(version_key),
+ _committee=True,
+ ).get()
+ if release is None:
+ return f"Release {project_key!s} {version_key!s} not found"
+ if release.phase != sql.ReleasePhase.RELEASE:
+ return f"Release {project_key!s} {version_key!s} is not in the
release phase"
+ if release.archived is not None:
Review Comment:
TOCTOU here. I think this should go in the `sqlmodel.update` below.
##########
atr/post/announce.py:
##########
@@ -146,6 +121,14 @@ async def selected(
email_cc=announce_form.email_cc,
email_bcc=announce_form.email_bcc,
)
+ if archive_prior is not None:
+ archive_error = await wacm.release.archive(project_key,
archive_prior.safe_version_key)
Review Comment:
This should probably be atomic with the release, like in a previous PR where
I suggested wrapping them both in another storage interface method to make them
atomic, but in this case it's a bit more complicated because we'll be writing
to SVN and sending emails. At some point we may just have to accept that
breakages may leave us in a partial state, and figure out how we can recover.
##########
atr/get/announce.py:
##########
@@ -248,25 +249,36 @@ async def _render_page(
# Custom widget for download_path_suffix with custom documentation
download_path_widget =
_render_download_path_field(default_download_path_value,
download_path_description)
+ prior_release_version = ""
+ if release.archive_prior_release:
+ prior = await
interaction.prior_release_for_archive(release.project, release.version)
+ if prior is not None:
+ prior_release_version = prior.version
Review Comment:
But the `auto_archive` checkbox will still be checked if `prior` is `None`?
Should probably be contingent on `archive_prior_release` _and_ `prior` not
being `None`.
##########
atr/post/announce.py:
##########
@@ -105,32 +96,16 @@ async def selected(
version_key=str(version_key),
)
- policy = release.release_policy or release.project.release_policy
- if policy and policy.file_tag_mappings:
- missing = []
- tags = policy.file_tag_mappings.keys()
- distributions = [d.platform.value.gh_slug for d in
release.distributions if (not d.staging) and (not d.pending)]
- for tag in tags:
- if tag not in distributions:
- missing.append(tag)
- if missing:
- return await session.redirect(
- get.announce.selected,
- error=f"This release cannot be announced until the following
distributions have been recorded: {
- ', '.join(missing)
- }",
- project_key=str(project_key),
- version_key=str(version_key),
- )
+ if response := await _validate_distributions(session, release,
project_key, version_key):
+ return response
+ if response := await _validate_subject_template_hash(session, project_key,
announce_form):
+ return response
- # Validate that the subject template hasn't changed
- subject_template = await
construct.announce_release_subject_default(project_key)
- current_hash = construct.template_hash(subject_template)
- if current_hash != announce_form.subject_template_hash:
- return await session.form_error(
- "subject_template_hash",
- "The subject template has been modified since you loaded the form.
Please reload and try again.",
- )
+ # Re-resolve the prior release server-side rather than trusting any value
+ # the form might submit. The form's auto_archive field is the opt-in only.
+ archive_prior: sql.Release | None = None
+ if announce_form.auto_archive:
Review Comment:
Shouldn't this also check the policy level option? The comment indicates
that this actually happens, that the form isn't trusted by itself, but I can't
find where the policy option is checked. It doesn't seem to be checked in
`prior_release_for_archive`, which is quite a short function. There's also the
`Release.archive_prior_release` property... what happens in all possible
combinations? I assume that if the release flag is `False`, then no matter what
we shouldn't archive. What if the release flag is `True` but the policy flag is
`False`? Inconsistent state, error? So it should only archive if they're both
`True`.
--
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]
