You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tooling.apache.org by "alitheg (via GitHub)" <gi...@apache.org> on 2026/04/21 08:17:44 UTC
[PR] Project delete / archival (tooling-trusted-releases)
alitheg opened a new pull request, #1186:
URL: https://github.com/apache/tooling-trusted-releases/pull/1186
I'll squash commits on this branch when it's done
--
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: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
[GH] Project delete / archival (tooling-trusted-releases)
Posted by "alitheg (via GitHub)" <gi...@apache.org>.
alitheg commented on code in PR #1186:
URL: https://github.com/apache/tooling-trusted-releases/pull/1186#discussion_r3125063989
##########
atr/util.py:
##########
@@ -382,6 +360,16 @@ def email_from_uid(uid: str) -> str | None:
return None
+def ensure_project_active(project: sql.Project) -> None:
Review Comment:
Ah, yes - I looked at putting it in the root but the only things in the root currently were classes for the actual readers and writers and the methods you call to access them. If you're happy with essentially a utility class living in there too it's an easy move!
##########
playwright/test.py:
##########
@@ -573,6 +579,46 @@ def test_all(page: Page, credentials: Credentials, skip_slow: bool) -> None:
logging.info(f"Tests took {round(finish - start, 3)} seconds")
+def test_archive_01_banner_absent_when_active(page: Page, credentials: Credentials) -> None:
Review Comment:
Ah I thought we were trying to only use Playwright for new tests and e2e was old. Or have I got that entirely the wrong way around (or totally wrong)?
##########
tests/e2e/tokens/helpers.py:
##########
@@ -26,7 +26,7 @@
def delete_token_by_label(page: Page, label: str) -> None:
row = get_token_row_by_label(page, label)
if row.count() > 0:
- row.get_by_role("button", name="Delete").click()
Review Comment:
I thought Revoke was more correct for tokens, since unlike SSH keys (which are always *valid* it's just the user chooses where they're accepted), we issue them. I can leave at delete if you prefer.
##########
tests/e2e/tokens/helpers.py:
##########
@@ -26,7 +26,7 @@
def delete_token_by_label(page: Page, label: str) -> None:
row = get_token_row_by_label(page, label)
if row.count() > 0:
- row.get_by_role("button", name="Delete").click()
Review Comment:
I made it in the SSH key changes set but because the e2e tests aren't run in CI (see my above comment/question about which tests are *current*), I missed that the tests were failing.
##########
tests/e2e/tokens/helpers.py:
##########
@@ -26,7 +26,7 @@
def delete_token_by_label(page: Page, label: str) -> None:
row = get_token_row_by_label(page, label)
if row.count() > 0:
- row.get_by_role("button", name="Delete").click()
Review Comment:
I made it in the SSH key changes set but because the e2e tests aren't run in CI (see my above comment/question about which tests are *current*), I missed that the tests were failing. Usually I run them all but clearly missed this time.
--
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: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
[GH] Project delete / archival (tooling-trusted-releases)
Posted by "dave2wave (via GitHub)" <gi...@apache.org>.
dave2wave commented on code in PR #1186:
URL: https://github.com/apache/tooling-trusted-releases/pull/1186#discussion_r3125867774
##########
atr/get/projects.py:
##########
@@ -94,28 +95,50 @@ async def add_project(
@get.typed
-async def projects(_session: web.Public, _projects: Literal["projects"]) -> str:
+async def projects(session: web.Public, _projects: Literal["projects"]) -> str:
"""
URL: /projects
Main project directory page.
"""
async with db.session() as data:
- projects = await data.project(_committee=True).order_by(sql.Project.name).all()
-
- delete_forms: dict[str, htm.Element] = {}
- for project in projects:
- delete_forms[str(project.key)] = await form.render(
- model_cls=shared.projects.DeleteSelectedProject,
- action=util.as_url(post.projects.delete),
- form_classes=".d-inline-block.m-0",
- submit_classes="btn-sm btn-outline-danger",
- submit_label="Delete project",
- empty=True,
- defaults={"project_key": str(project.key)},
- confirm="Are you sure you want to delete this project? This cannot be undone.",
- )
+ projects = await data.project(_committee=True, _releases=True).order_by(sql.Project.name).all()
+
+ committee_project_counts: Counter[str] = Counter(str(p.committee.key) for p in projects if p.committee)
Review Comment:
Sorry that I've change my direction.
--
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: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
[GH] Project delete / archival (tooling-trusted-releases)
Posted by "alitheg (via GitHub)" <gi...@apache.org>.
alitheg commented on code in PR #1186:
URL: https://github.com/apache/tooling-trusted-releases/pull/1186#discussion_r3129260370
##########
atr/get/projects.py:
##########
@@ -94,28 +95,50 @@ async def add_project(
@get.typed
-async def projects(_session: web.Public, _projects: Literal["projects"]) -> str:
+async def projects(session: web.Public, _projects: Literal["projects"]) -> str:
"""
URL: /projects
Main project directory page.
"""
async with db.session() as data:
- projects = await data.project(_committee=True).order_by(sql.Project.name).all()
-
- delete_forms: dict[str, htm.Element] = {}
- for project in projects:
- delete_forms[str(project.key)] = await form.render(
- model_cls=shared.projects.DeleteSelectedProject,
- action=util.as_url(post.projects.delete),
- form_classes=".d-inline-block.m-0",
- submit_classes="btn-sm btn-outline-danger",
- submit_label="Delete project",
- empty=True,
- defaults={"project_key": str(project.key)},
- confirm="Are you sure you want to delete this project? This cannot be undone.",
- )
+ projects = await data.project(_committee=True, _releases=True).order_by(sql.Project.name).all()
+
+ committee_project_counts: Counter[str] = Counter(str(p.committee.key) for p in projects if p.committee)
Review Comment:
> Sorry that I've change my direction.
No problem! I'd just posted that before I brought it up in the call - thought it best to make sure we're on the same page. New direction makes most sense I think.
--
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: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
[GH] Project delete / archival (tooling-trusted-releases)
Posted by "alitheg (via GitHub)" <gi...@apache.org>.
alitheg commented on code in PR #1186:
URL: https://github.com/apache/tooling-trusted-releases/pull/1186#discussion_r3186897618
##########
atr/get/vote.py:
##########
@@ -678,14 +678,15 @@ async def _render_vote_authenticated(
potency = binding_word if is_binding else non_binding_word
_render_binding_status(page, is_binding, binding_committee, vote_round)
- _render_vote_delivery(page, archive_url, vote_recipient, trusted_vote=False)
+ _render_vote_delivery(page, release, archive_url, vote_recipient, trusted_vote=False)
Review Comment:
Ah I may have messed up a rebase.
--
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: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
Re: [PR] Project delete / archival (tooling-trusted-releases)
Posted by "alitheg (via GitHub)" <gi...@apache.org>.
alitheg merged PR #1186:
URL: https://github.com/apache/tooling-trusted-releases/pull/1186
--
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: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
[GH] Project delete / archival (tooling-trusted-releases)
Posted by "sbp (via GitHub)" <gi...@apache.org>.
sbp commented on code in PR #1186:
URL: https://github.com/apache/tooling-trusted-releases/pull/1186#discussion_r3183931484
##########
atr/get/vote.py:
##########
@@ -678,14 +678,15 @@ async def _render_vote_authenticated(
potency = binding_word if is_binding else non_binding_word
_render_binding_status(page, is_binding, binding_committee, vote_round)
- _render_vote_delivery(page, archive_url, vote_recipient, trusted_vote=False)
+ _render_vote_delivery(page, release, archive_url, vote_recipient, trusted_vote=False)
Review Comment:
The code may have changed in the meantime or something, because the actual function argument order is now `archive_url, release`.
##########
atr/storage/writers/project.py:
##########
@@ -186,17 +188,59 @@ async def create(self, committee_key: safe.CommitteeKey, display_name: str, labe
else:
raise
+ async def archive(self, project_key: safe.ProjectKey) -> None:
+ project = await self.__data.project(key=str(project_key), status=sql.ProjectStatus.ACTIVE, _releases=True).get()
+
+ if not project:
+ raise storage.AccessError(f"Project '{project_key}' not found.")
+
+ # TODO: once release archival is implemented, relax this further
+ post_draft_phases = {
+ sql.ReleasePhase.RELEASE_CANDIDATE,
+ sql.ReleasePhase.RELEASE_PREVIEW,
+ sql.ReleasePhase.RELEASE,
+ }
+ if any(r.phase in post_draft_phases for r in project.releases):
Review Comment:
So this means that a project can be archived when it has draft releases? If so, it appears that the `delete` function in `atr.storage.writers.release` can't be used to later delete those drafts, because it now checks `storage.ensure_project_active(release.project)`. Therefore, if a project is archived when it has draft releases, those releases will be stranded.
##########
atr/storage/writers/revision.py:
##########
@@ -394,6 +394,10 @@ async def clear_quarantine(
version_key: safe.VersionKey,
quarantined_id: int,
) -> None:
+ project = await self.__data.project(key=str(project_key)).demand(
+ storage.AccessError(f"Project '{project_key}' not found.")
+ )
+ storage.ensure_project_active(project)
Review Comment:
The `set_tag` method in this module is missing `storage.ensure_project_active`. Ideally we'd make this the default, then opt out if we want to allow the action for non-active projects, but I'm not sure how to best go about that.
##########
atr/storage/writers/project.py:
##########
@@ -186,17 +188,59 @@ async def create(self, committee_key: safe.CommitteeKey, display_name: str, labe
else:
raise
+ async def archive(self, project_key: safe.ProjectKey) -> None:
+ project = await self.__data.project(key=str(project_key), status=sql.ProjectStatus.ACTIVE, _releases=True).get()
+
+ if not project:
+ raise storage.AccessError(f"Project '{project_key}' not found.")
+
+ # TODO: once release archival is implemented, relax this further
+ post_draft_phases = {
+ sql.ReleasePhase.RELEASE_CANDIDATE,
+ sql.ReleasePhase.RELEASE_PREVIEW,
+ sql.ReleasePhase.RELEASE,
+ }
+ if any(r.phase in post_draft_phases for r in project.releases):
Review Comment:
In general, what was the intent behind allowing a release to be archived if it has draft releases? I remember we discussed this, but I don't remember what the outcome was.
--
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: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
[GH] Project delete / archival (tooling-trusted-releases)
Posted by "sbp (via GitHub)" <gi...@apache.org>.
sbp commented on code in PR #1186:
URL: https://github.com/apache/tooling-trusted-releases/pull/1186#discussion_r3125269390
##########
playwright/test.py:
##########
@@ -573,6 +579,46 @@ def test_all(page: Page, credentials: Credentials, skip_slow: bool) -> None:
logging.info(f"Tests took {round(finish - start, 3)} seconds")
+def test_archive_01_banner_absent_when_active(page: Page, credentials: Credentials) -> None:
Review Comment:
Heh, yeah, it's the other way around! The e2e tests are better for new things because we can more easily run subsets of what we add there. We try to keep Playwright working and compatible only, although it should also absorb any particularly significant features.
##########
tests/e2e/tokens/helpers.py:
##########
@@ -26,7 +26,7 @@
def delete_token_by_label(page: Page, label: str) -> None:
row = get_token_row_by_label(page, label)
if row.count() > 0:
- row.get_by_role("button", name="Delete").click()
Review Comment:
Ah, okay, no problem. I was only wondering why the tests changed but the code didn't!
##########
atr/get/projects.py:
##########
@@ -94,28 +95,50 @@ async def add_project(
@get.typed
-async def projects(_session: web.Public, _projects: Literal["projects"]) -> str:
+async def projects(session: web.Public, _projects: Literal["projects"]) -> str:
"""
URL: /projects
Main project directory page.
"""
async with db.session() as data:
- projects = await data.project(_committee=True).order_by(sql.Project.name).all()
-
- delete_forms: dict[str, htm.Element] = {}
- for project in projects:
- delete_forms[str(project.key)] = await form.render(
- model_cls=shared.projects.DeleteSelectedProject,
- action=util.as_url(post.projects.delete),
- form_classes=".d-inline-block.m-0",
- submit_classes="btn-sm btn-outline-danger",
- submit_label="Delete project",
- empty=True,
- defaults={"project_key": str(project.key)},
- confirm="Are you sure you want to delete this project? This cannot be undone.",
- )
+ projects = await data.project(_committee=True, _releases=True).order_by(sql.Project.name).all()
+
+ committee_project_counts: Counter[str] = Counter(str(p.committee.key) for p in projects if p.committee)
Review Comment:
I was just thinking about it being consistent with its handler, as part of the "everything should have `ACTIVE` checks" phase of the review. I think the idea is the opposite, that we _can't_ archive if there are ongoing releases.
--
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: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
[GH] Project delete / archival (tooling-trusted-releases)
Posted by "alitheg (via GitHub)" <gi...@apache.org>.
alitheg commented on code in PR #1186:
URL: https://github.com/apache/tooling-trusted-releases/pull/1186#discussion_r3125355354
##########
atr/get/projects.py:
##########
@@ -94,28 +95,50 @@ async def add_project(
@get.typed
-async def projects(_session: web.Public, _projects: Literal["projects"]) -> str:
+async def projects(session: web.Public, _projects: Literal["projects"]) -> str:
"""
URL: /projects
Main project directory page.
"""
async with db.session() as data:
- projects = await data.project(_committee=True).order_by(sql.Project.name).all()
-
- delete_forms: dict[str, htm.Element] = {}
- for project in projects:
- delete_forms[str(project.key)] = await form.render(
- model_cls=shared.projects.DeleteSelectedProject,
- action=util.as_url(post.projects.delete),
- form_classes=".d-inline-block.m-0",
- submit_classes="btn-sm btn-outline-danger",
- submit_label="Delete project",
- empty=True,
- defaults={"project_key": str(project.key)},
- confirm="Are you sure you want to delete this project? This cannot be undone.",
- )
+ projects = await data.project(_committee=True, _releases=True).order_by(sql.Project.name).all()
+
+ committee_project_counts: Counter[str] = Counter(str(p.committee.key) for p in projects if p.committee)
Review Comment:
Dave's original comment read "A project that has current or archived releases may only be archived. If archived then any current releases are also archived." which I thought implied we should just allow archiving with current releases.
--
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: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
[GH] Project delete / archival (tooling-trusted-releases)
Posted by "sbp (via GitHub)" <gi...@apache.org>.
sbp commented on code in PR #1186:
URL: https://github.com/apache/tooling-trusted-releases/pull/1186#discussion_r3124809329
##########
tests/unit/test_archived_project_writer_guards.py:
##########
@@ -0,0 +1,326 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest.mock as mock
+from types import SimpleNamespace
+
+import pytest
+
+import atr.models.safe as safe
+import atr.models.sql as sql
+import atr.storage as storage
+import atr.storage.writers.announce as announce
+import atr.storage.writers.checks as checks
+import atr.storage.writers.distributions as distributions
+import atr.storage.writers.release as release
+import atr.storage.writers.revision as revision
+import atr.storage.writers.sbom as sbom
+import atr.storage.writers.vote as vote
+
+
+def _retired_project(key: str = "project") -> SimpleNamespace:
+ return SimpleNamespace(
+ key=key,
+ status=sql.ProjectStatus.RETIRED,
+ committee_key=key,
+ display_name=key.capitalize(),
+ short_display_name=key.capitalize(),
+ )
+
+
+def _retired_release(project_key: str = "project", version: str = "1.0.0") -> SimpleNamespace:
+ return SimpleNamespace(
+ key=sql.release_key(project_key, version),
+ project=_retired_project(project_key),
+ version=version,
+ )
+
+
+def _query_returning(obj: object) -> mock.MagicMock:
+ query = mock.MagicMock()
+ query.demand = mock.AsyncMock(return_value=obj)
+ query.get = mock.AsyncMock(return_value=obj)
+ return query
+
+
+def _async_context_manager(value: object) -> mock.MagicMock:
+ ctx = mock.MagicMock()
+ ctx.__aenter__ = mock.AsyncMock(return_value=value)
+ ctx.__aexit__ = mock.AsyncMock(return_value=None)
+ return ctx
+
+
+# --- revision --------------------------------------------------------------
Review Comment:
Section comments break the function ordering script. Perhaps the function ordering script should be fixed to accommodate these, rather than removing all comments everywhere, though!
##########
atr/api/__init__.py:
##########
@@ -1661,6 +1665,54 @@ async def vote_tabulate(
).model_dump(mode="json"), 200
+class _TestProjectStatusArgs(pydantic.BaseModel):
Review Comment:
Whoops. We never prefix classes with an underscore, but I notice that our code conventions don't presently reflect that. (They only mention type variables.) I'll update the conventions.
##########
tests/unit/test_create_revision.py:
##########
@@ -111,6 +110,7 @@ async def test_clone_from_older_revision_skips_merge_without_intervening_change(
release = mock.MagicMock()
release.phase = sql.ReleasePhase.RELEASE_PREVIEW
release.project = mock.MagicMock()
+ release.project.status = sql.ProjectStatus.ACTIVE
Review Comment:
These tests now fail:
```
tests/unit/test_create_revision.py ..F.F........ [ 27%]
```
With problems in `test_intervening_revision_with_path_provenance_verifies_sources` and `test_modify_path_provenance_flows_into_write_files_data`.
##########
tests/unit/test_archived_project_writer_guards.py:
##########
@@ -0,0 +1,326 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest.mock as mock
+from types import SimpleNamespace
+
+import pytest
+
+import atr.models.safe as safe
+import atr.models.sql as sql
+import atr.storage as storage
+import atr.storage.writers.announce as announce
+import atr.storage.writers.checks as checks
+import atr.storage.writers.distributions as distributions
+import atr.storage.writers.release as release
+import atr.storage.writers.revision as revision
+import atr.storage.writers.sbom as sbom
+import atr.storage.writers.vote as vote
+
+
+def _retired_project(key: str = "project") -> SimpleNamespace:
+ return SimpleNamespace(
+ key=key,
+ status=sql.ProjectStatus.RETIRED,
+ committee_key=key,
+ display_name=key.capitalize(),
+ short_display_name=key.capitalize(),
+ )
+
+
+def _retired_release(project_key: str = "project", version: str = "1.0.0") -> SimpleNamespace:
+ return SimpleNamespace(
+ key=sql.release_key(project_key, version),
+ project=_retired_project(project_key),
+ version=version,
+ )
+
+
+def _query_returning(obj: object) -> mock.MagicMock:
+ query = mock.MagicMock()
+ query.demand = mock.AsyncMock(return_value=obj)
+ query.get = mock.AsyncMock(return_value=obj)
+ return query
+
+
+def _async_context_manager(value: object) -> mock.MagicMock:
+ ctx = mock.MagicMock()
+ ctx.__aenter__ = mock.AsyncMock(return_value=value)
+ ctx.__aexit__ = mock.AsyncMock(return_value=None)
+ return ctx
+
+
+# --- revision --------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_revision_create_revision_with_quarantine_blocks_retired(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ writer = object.__new__(revision.CommitteeParticipant)
+ writer._CommitteeParticipant__data = mock.MagicMock()
+ writer._CommitteeParticipant__asf_uid = "tester"
+ writer._CommitteeParticipant__committee_key = "project"
+
+ inner_data = mock.MagicMock()
+ inner_data.release = mock.MagicMock(return_value=_query_returning(_retired_release()))
+ monkeypatch.setattr(revision.db, "session", lambda: _async_context_manager(inner_data))
+
+ with pytest.raises(storage.AccessError, match="archived"):
+ await writer.create_revision_with_quarantine(
+ safe.ProjectKey("project"),
+ safe.VersionKey("1.0.0"),
+ "tester",
+ allowed_phases=frozenset({sql.ReleasePhase.RELEASE_CANDIDATE_DRAFT}),
+ )
+
+
+@pytest.mark.asyncio
+async def test_revision_clear_quarantine_blocks_retired() -> None:
+ data = mock.MagicMock()
+ data.project = mock.MagicMock(return_value=_query_returning(_retired_project()))
+ writer = object.__new__(revision.CommitteeParticipant)
+ writer._CommitteeParticipant__data = data
+ writer._CommitteeParticipant__asf_uid = "tester"
+ writer._CommitteeParticipant__committee_key = "project"
+
+ with pytest.raises(storage.AccessError, match="archived"):
+ await writer.clear_quarantine(
+ safe.ProjectKey("project"),
+ safe.VersionKey("1.0.0"),
+ 42,
+ )
+
+
+# --- release ---------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_release_delete_blocks_retired() -> None:
+ data = mock.MagicMock()
+ data.release = mock.MagicMock(return_value=_query_returning(_retired_release()))
+ writer = object.__new__(release.CommitteeParticipant)
+ writer._CommitteeParticipant__data = data
+ writer._CommitteeParticipant__asf_uid = "tester"
+ writer._CommitteeParticipant__committee_key = "project"
+
+ with pytest.raises(storage.AccessError, match="archived"):
+ await writer.delete(safe.ProjectKey("project"), safe.VersionKey("1.0.0"))
+
+
+@pytest.mark.asyncio
+async def test_release_import_from_svn_blocks_retired() -> None:
+ data = mock.MagicMock()
+ data.project = mock.MagicMock(return_value=_query_returning(_retired_project()))
+ writer = object.__new__(release.CommitteeParticipant)
+ writer._CommitteeParticipant__data = data
+ writer._CommitteeParticipant__asf_uid = "tester"
+ writer._CommitteeParticipant__committee_key = "project"
+
+ with pytest.raises(storage.AccessError, match="archived"):
+ await writer.import_from_svn(
+ safe.ProjectKey("project"),
+ safe.VersionKey("1.0.0"),
+ safe.RelPath("svn/url"),
+ "r1",
+ None,
+ )
+
+
+@pytest.mark.asyncio
+async def test_release_promote_to_candidate_blocks_retired() -> None:
+ data = mock.MagicMock()
+ data.release = mock.MagicMock(return_value=_query_returning(_retired_release()))
+ writer = object.__new__(release.CommitteeParticipant)
+ writer._CommitteeParticipant__data = data
+ writer._CommitteeParticipant__asf_uid = "tester"
+ writer._CommitteeParticipant__committee_key = "project"
+
+ with pytest.raises(storage.AccessError, match="archived"):
+ await writer.promote_to_candidate(
+ safe.ReleaseKey("project-1.0.0"),
+ safe.RevisionNumber("00001"),
+ )
+
+
+# --- vote ------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_vote_resolve_release_blocks_retired_after_merge() -> None:
+ data = mock.MagicMock()
+ merged = _retired_release()
+ data.merge = mock.AsyncMock(return_value=merged)
+ writer = object.__new__(vote.CommitteeMember)
+ writer._CommitteeMember__data = data
+ writer._CommitteeMember__asf_uid = "tester"
+ writer._CommitteeMember__committee_key = "project"
+
+ stub_release = SimpleNamespace(project=SimpleNamespace(status=sql.ProjectStatus.ACTIVE, key="project"))
+ with pytest.raises(storage.AccessError, match="archived"):
+ await writer.resolve_release(
+ safe.ProjectKey("project"),
+ stub_release,
+ None,
+ "passed",
+ SimpleNamespace(),
+ "Chair",
+ "body",
+ )
+ data.merge.assert_awaited_once_with(stub_release)
+
+
+@pytest.mark.asyncio
+async def test_vote_send_resolution_blocks_retired() -> None:
+ writer = object.__new__(vote.CommitteeMember)
+ writer._CommitteeMember__data = mock.MagicMock()
+ writer._CommitteeMember__asf_uid = "tester"
+ writer._CommitteeMember__committee_key = "project"
+
+ rel = _retired_release()
+ with pytest.raises(storage.AccessError, match="archived"):
+ await writer.send_resolution(
+ rel,
+ "passed",
+ "body",
+ "tester",
+ "Chair",
+ SimpleNamespace(),
+ )
+
+
+# --- announce --------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_announce_release_blocks_retired(monkeypatch: pytest.MonkeyPatch) -> None:
+ monkeypatch.setattr(
+ announce.util,
+ "permitted_announce_recipients",
+ lambda _uid: {"dev@project.apache.org"},
+ )
+ data = mock.MagicMock()
+ data.release = mock.MagicMock(return_value=_query_returning(_retired_release()))
+ writer = object.__new__(announce.CommitteeMember)
+ writer._CommitteeMember__data = data
+ writer._CommitteeMember__asf_uid = "tester"
+ writer._CommitteeMember__committee_key = "project"
+ writer._CommitteeParticipant__data = data
+ writer._CommitteeParticipant__committee_key = "project"
+
+ with pytest.raises(storage.AccessError, match="archived"):
+ await writer.release(
+ safe.ProjectKey("project"),
+ safe.VersionKey("1.0.0"),
+ safe.RevisionNumber("00001"),
+ "dev@project.apache.org",
+ "body",
+ None,
+ "tester",
+ "Chair",
+ )
+
+
+# --- distributions ---------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_distributions_automate_blocks_retired() -> None:
+ data = mock.MagicMock()
+ data.project = mock.MagicMock(return_value=_query_returning(_retired_project()))
+ writer = object.__new__(distributions.CommitteeMember)
+ writer._CommitteeMember__data = data
+ writer._CommitteeMember__asf_uid = "tester"
+ writer._CommitteeMember__committee_key = "project"
+
+ platform = next(iter(sql.DistributionPlatform))
+ with pytest.raises(storage.AccessError, match="archived"):
+ await writer.automate(
+ safe.ReleaseKey("project-1.0.0"),
+ platform,
+ "project",
+ None,
+ safe.ProjectKey("project"),
+ safe.VersionKey("1.0.0"),
+ "release",
+ None,
+ safe.Alphanumeric("pkg"),
+ safe.VersionKey("1.0.0"),
+ False,
+ )
+
+
+@pytest.mark.asyncio
+async def test_distributions_record_blocks_retired_via_release_lookup() -> None:
+ data = mock.MagicMock()
+ data.release = mock.MagicMock(return_value=_query_returning(_retired_release()))
+ writer = object.__new__(distributions.CommitteeMember)
+ writer._CommitteeMember__data = data
+ writer._CommitteeMember__asf_uid = "tester"
+ writer._CommitteeMember__committee_key = "project"
+
+ platform = next(iter(sql.DistributionPlatform))
+ with pytest.raises(storage.AccessError, match="archived"):
+ await writer.record(
+ safe.ReleaseKey("project-1.0.0"),
+ platform,
+ None,
+ safe.Alphanumeric("pkg"),
+ safe.VersionKey("1.0.0"),
+ False,
+ False,
+ None,
+ )
+
+
+# --- sbom ------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_sbom_augment_blocks_retired() -> None:
+ data = mock.MagicMock()
+ data.project = mock.MagicMock(return_value=_query_returning(_retired_project()))
+ writer = object.__new__(sbom.CommitteeParticipant)
+ writer._CommitteeParticipant__data = data
+ writer._CommitteeParticipant__asf_uid = "tester"
+ writer._CommitteeParticipant__committee_key = "project"
+
+ with pytest.raises(storage.AccessError, match="archived"):
+ await writer.augment_cyclonedx(
+ safe.ProjectKey("project"),
+ safe.VersionKey("1.0.0"),
+ safe.RevisionNumber("00001"),
+ safe.RelPath("artifact.tar.gz"),
+ )
+
+
+# --- checks ----------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_checks_ignore_add_propagates_chokepoint_block() -> None:
+ data = mock.MagicMock()
+ data.project = mock.MagicMock(return_value=_query_returning(_retired_project()))
+ writer = object.__new__(checks.CommitteeMember)
+ writer._CommitteeMember__data = data
Review Comment:
Presumably only one or the other is needed? (`CommitteeMember` vs `CommitteeParticipant`)
##########
playwright/test.py:
##########
@@ -573,6 +579,46 @@ def test_all(page: Page, credentials: Credentials, skip_slow: bool) -> None:
logging.info(f"Tests took {round(finish - start, 3)} seconds")
+def test_archive_01_banner_absent_when_active(page: Page, credentials: Credentials) -> None:
Review Comment:
Not sure we necessarily need Playwright tests for this feature. Could they be e2e tests? Also, if these crash half way through, then I think we get stuck with a `RETIRED` project and then all later runs will break. (This is why I had to add all the other state resetting stuff in these tests.)
##########
tests/unit/test_archived_project_writer_guards.py:
##########
@@ -0,0 +1,326 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest.mock as mock
+from types import SimpleNamespace
+
+import pytest
+
+import atr.models.safe as safe
+import atr.models.sql as sql
+import atr.storage as storage
+import atr.storage.writers.announce as announce
+import atr.storage.writers.checks as checks
+import atr.storage.writers.distributions as distributions
+import atr.storage.writers.release as release
+import atr.storage.writers.revision as revision
+import atr.storage.writers.sbom as sbom
+import atr.storage.writers.vote as vote
+
+
+def _retired_project(key: str = "project") -> SimpleNamespace:
+ return SimpleNamespace(
+ key=key,
+ status=sql.ProjectStatus.RETIRED,
+ committee_key=key,
+ display_name=key.capitalize(),
+ short_display_name=key.capitalize(),
+ )
+
+
+def _retired_release(project_key: str = "project", version: str = "1.0.0") -> SimpleNamespace:
+ return SimpleNamespace(
+ key=sql.release_key(project_key, version),
+ project=_retired_project(project_key),
+ version=version,
+ )
+
+
+def _query_returning(obj: object) -> mock.MagicMock:
+ query = mock.MagicMock()
+ query.demand = mock.AsyncMock(return_value=obj)
+ query.get = mock.AsyncMock(return_value=obj)
+ return query
+
+
+def _async_context_manager(value: object) -> mock.MagicMock:
+ ctx = mock.MagicMock()
+ ctx.__aenter__ = mock.AsyncMock(return_value=value)
+ ctx.__aexit__ = mock.AsyncMock(return_value=None)
+ return ctx
+
+
+# --- revision --------------------------------------------------------------
Review Comment:
But note also that although these tests are nicely group-prefixed, they're also in entirely the wrong order.
##########
atr/util.py:
##########
@@ -382,6 +360,16 @@ def email_from_uid(uid: str) -> str | None:
return None
+def ensure_project_active(project: sql.Project) -> None:
Review Comment:
This function is only ever used in `storage`, and imports `storage`, so I think it would be happier in `storage` somewhere. Probably just at the root is fine, or whatever makes sense.
##########
tests/e2e/tokens/helpers.py:
##########
@@ -26,7 +26,7 @@
def delete_token_by_label(page: Page, label: str) -> None:
row = get_token_row_by_label(page, label)
if row.count() > 0:
- row.get_by_role("button", name="Delete").click()
Review Comment:
Why did these get renamed? I don't see the corresponding change elsewhere.
##########
atr/storage/writers/project.py:
##########
@@ -186,17 +186,55 @@ async def create(self, committee_key: safe.CommitteeKey, display_name: str, labe
else:
raise
+ async def archive(self, project_key: safe.ProjectKey) -> None:
+ project = await self.__data.project(key=str(project_key), status=sql.ProjectStatus.ACTIVE).get()
+
+ if not project:
+ raise storage.AccessError(f"Project '{project_key}' not found.")
+
+ if project.committee_key:
+ committee_projects = await self.__data.project(
+ committee_key=project.committee_key, status=sql.ProjectStatus.ACTIVE
Review Comment:
This correctly checks that the `status` is `ACTIVE`, but e.g. `category_add` and `category_remove` in this file don't have those checks. I may have forgotten to add the checks elsewhere, too.
##########
atr/storage/writers/project.py:
##########
@@ -186,17 +186,55 @@ async def create(self, committee_key: safe.CommitteeKey, display_name: str, labe
else:
raise
+ async def archive(self, project_key: safe.ProjectKey) -> None:
Review Comment:
This function doesn't seem to have any protections. What if there's a release in progress, e.g. holding a vote or something? Shouldn't the function hold a lock (or do some complex atomic query) and ensure that there are no active releases?
I notice it doesn't actually say that in https://github.com/apache/tooling-trusted-releases/issues/510 but it would seem like it would cause a problem if a project were archived when it was mid-release.
##########
atr/get/projects.py:
##########
@@ -94,28 +95,50 @@ async def add_project(
@get.typed
-async def projects(_session: web.Public, _projects: Literal["projects"]) -> str:
+async def projects(session: web.Public, _projects: Literal["projects"]) -> str:
"""
URL: /projects
Main project directory page.
"""
async with db.session() as data:
- projects = await data.project(_committee=True).order_by(sql.Project.name).all()
-
- delete_forms: dict[str, htm.Element] = {}
- for project in projects:
- delete_forms[str(project.key)] = await form.render(
- model_cls=shared.projects.DeleteSelectedProject,
- action=util.as_url(post.projects.delete),
- form_classes=".d-inline-block.m-0",
- submit_classes="btn-sm btn-outline-danger",
- submit_label="Delete project",
- empty=True,
- defaults={"project_key": str(project.key)},
- confirm="Are you sure you want to delete this project? This cannot be undone.",
- )
+ projects = await data.project(_committee=True, _releases=True).order_by(sql.Project.name).all()
+
+ committee_project_counts: Counter[str] = Counter(str(p.committee.key) for p in projects if p.committee)
Review Comment:
Does this need to count `ACTIVE` projects only? Do we even need to track inactive projects in the database, necessarily? Perhaps we could have a different table for them or something.
--
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: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
[GH] Project delete / archival (tooling-trusted-releases)
Posted by "alitheg (via GitHub)" <gi...@apache.org>.
alitheg commented on code in PR #1186:
URL: https://github.com/apache/tooling-trusted-releases/pull/1186#discussion_r3186902097
##########
atr/storage/writers/project.py:
##########
@@ -186,17 +188,59 @@ async def create(self, committee_key: safe.CommitteeKey, display_name: str, labe
else:
raise
+ async def archive(self, project_key: safe.ProjectKey) -> None:
+ project = await self.__data.project(key=str(project_key), status=sql.ProjectStatus.ACTIVE, _releases=True).get()
+
+ if not project:
+ raise storage.AccessError(f"Project '{project_key}' not found.")
+
+ # TODO: once release archival is implemented, relax this further
+ post_draft_phases = {
+ sql.ReleasePhase.RELEASE_CANDIDATE,
+ sql.ReleasePhase.RELEASE_PREVIEW,
+ sql.ReleasePhase.RELEASE,
+ }
+ if any(r.phase in post_draft_phases for r in project.releases):
Review Comment:
The original idea I think was that draft releases would be discarded if a project is archived (archived == retired).
--
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: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
[GH] Project delete / archival (tooling-trusted-releases)
Posted by "alitheg (via GitHub)" <gi...@apache.org>.
alitheg commented on code in PR #1186:
URL: https://github.com/apache/tooling-trusted-releases/pull/1186#discussion_r3125219780
##########
atr/get/projects.py:
##########
@@ -94,28 +95,50 @@ async def add_project(
@get.typed
-async def projects(_session: web.Public, _projects: Literal["projects"]) -> str:
+async def projects(session: web.Public, _projects: Literal["projects"]) -> str:
"""
URL: /projects
Main project directory page.
"""
async with db.session() as data:
- projects = await data.project(_committee=True).order_by(sql.Project.name).all()
-
- delete_forms: dict[str, htm.Element] = {}
- for project in projects:
- delete_forms[str(project.key)] = await form.render(
- model_cls=shared.projects.DeleteSelectedProject,
- action=util.as_url(post.projects.delete),
- form_classes=".d-inline-block.m-0",
- submit_classes="btn-sm btn-outline-danger",
- submit_label="Delete project",
- empty=True,
- defaults={"project_key": str(project.key)},
- confirm="Are you sure you want to delete this project? This cannot be undone.",
- )
+ projects = await data.project(_committee=True, _releases=True).order_by(sql.Project.name).all()
+
+ committee_project_counts: Counter[str] = Counter(str(p.committee.key) for p in projects if p.committee)
Review Comment:
A reasonable question. Was the idea that you can archive if there are live releases? In which case I assume we'd need to keep a hold of the project since the release depends on it?
--
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: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org