sbp commented on code in PR #1264:
URL:
https://github.com/apache/tooling-trusted-releases/pull/1264#discussion_r3289029829
##########
atr/models/sql.py:
##########
@@ -1556,6 +1556,12 @@ class Distribution(sqlmodel.SQLModel, table=True):
api_url: str | None = sqlmodel.Field(default=None)
web_url: str | None = sqlmodel.Field(default=None)
created_by: str | None = sqlmodel.Field(default=None)
+ # Records which Revision was the source of the staged artifacts.
+ # Set when staging=True so that the finish-phase code can verify the
+ # artifacts being promoted match those that were voted on, addressing
+ # the caveat in
https://github.com/apache/tooling-trusted-releases/issues/751
+ # The column is nullable so that pre-existing rows are unaffected.
+ staging_revision_key: str | None = sqlmodel.Field(default=None,
foreign_key="revision.key")
Review Comment:
FK without a cascade.
##########
atr/storage/writers/distributions.py:
##########
@@ -302,6 +309,12 @@ async def __upgrade_staging_to_final(
version=version,
).demand(RuntimeError(f"Distribution {tag} not found"))
if existing.staging:
+ # TODO(#751): When remote promotion of artifacts to third party
+ # platforms is implemented, assert here that
existing.staging_revision_key
+ # equals the current Revision.key for the release before flipping
+ # staging to False, so that the artifacts being promoted are
+ # guaranteed to match those that were voted on. The column was
+ # added in #751 for exactly this purpose.
Review Comment:
A new revision is created at the end of the vote phase, when going to
finish. This was historical, when we had revisions in the finish phase. The
staging revision shouldn't be a revision behind during the vote, only
afterwards. It wasn't a problem before when we had finish phase revisions that
the new revisions weren't the same number as the one voted on. ATR ensured that
the changes applied were meaning preserving.
##########
tests/unit/test_distribution_staging.py:
##########
@@ -0,0 +1,114 @@
+# 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 pytest
+
+import atr.models.distribution as distribution
+import atr.models.safe as safe
+import atr.models.sql as sql
+import atr.shared.distribution as shared_distribution
+
+
+def _data(platform: sql.DistributionPlatform, *, owner_namespace: str | None =
None) -> distribution.Data:
+ return distribution.Data(
+ platform=platform,
+ owner_namespace=safe.Alphanumeric(owner_namespace) if owner_namespace
else None,
+ package=safe.Alphanumeric("example"),
+ version=safe.VersionKey("1.0.0"),
+ details=False,
+ )
+
+
+# --- _template_url: enum is now the single source of truth ---
Review Comment:
These comments mean that we can't run `fix_order` on the file.
##########
atr/storage/writers/distributions.py:
##########
@@ -302,6 +309,12 @@ async def __upgrade_staging_to_final(
version=version,
).demand(RuntimeError(f"Distribution {tag} not found"))
if existing.staging:
+ # TODO(#751): When remote promotion of artifacts to third party
+ # platforms is implemented, assert here that
existing.staging_revision_key
+ # equals the current Revision.key for the release before flipping
+ # staging to False, so that the artifacts being promoted are
+ # guaranteed to match those that were voted on. The column was
+ # added in #751 for exactly this purpose.
Review Comment:
Oh, and we can still remove RC tags and empty directories, so we still need
this.
##########
atr/models/sql.py:
##########
@@ -1556,6 +1556,12 @@ class Distribution(sqlmodel.SQLModel, table=True):
api_url: str | None = sqlmodel.Field(default=None)
web_url: str | None = sqlmodel.Field(default=None)
created_by: str | None = sqlmodel.Field(default=None)
+ # Records which Revision was the source of the staged artifacts.
+ # Set when staging=True so that the finish-phase code can verify the
+ # artifacts being promoted match those that were voted on, addressing
+ # the caveat in
https://github.com/apache/tooling-trusted-releases/issues/751
+ # The column is nullable so that pre-existing rows are unaffected.
+ staging_revision_key: str | None = sqlmodel.Field(default=None,
foreign_key="revision.key")
Review Comment:
Could set null on delete. Only happens if distributions aren't deleted first
anyway, but then it's order dependent which would be nice to avoid.
--
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]
