Conversation
| via = sql.validate_instrumented_attribute | ||
| old_key = move.key | ||
| version = str(move.row.version) | ||
| new_key = f"{move.to_project}-{version}" |
There was a problem hiding this comment.
If the uploader gets the version wrong, couldn't this corrupt the release? There doesn't appear to be any validation of this new_key. I tested, and if there are artifacts then you get a 500 error anyway, but it breaks the whole import. If there are no artifacts, it corrupts silently. I assume that this will cause problems elsewhere, but I didn't check (I mean, release key is proj-0.1 but version is 0.2).
There was a problem hiding this comment.
Quite right, and that hits something that bugged me when I did this - I've changed to start deriving the release keys instead of exposing them. For the scope of this page, it doesn't really matter that you can't change a key, you create a new one and reassign the artifacts to it - you can always remove the empty release as a separate operation by merging it with another.
There was another bug here - if you change version and nothing else your edit was silently dropped.
| path = str(row.artifact_path) | ||
| pk = (str(row.project_key), str(row.version), path) | ||
| suffix = str(row.download_path_suffix) if row.download_path_suffix else "" | ||
| matched = snapshot.artifact_by_dist.get((suffix, path)) if suffix else None |
There was a problem hiding this comment.
So this is looking up artifacts by (download path, filename)? Problem is, since download path is an exported field, it's also editable. If anybody changes it, how will it know what's being moved? So, any download path edit becomes a crash: it's an actual 500.
There was a problem hiding this comment.
The path is the identity, so we should be using it to look up the existing one, and it's the only real conflict in seeding/correcting artifacts (you can't use the same full artifact path in more than one release). The add path now checks to make sure there's no conflict there, but I'm considering actually having the uploaded artifact CSV be an exhaustive list - remove all that don't match from the same PMC. This is for correcting the seeds, after all.
| continue | ||
| current_project = snapshot.release_project.get(key) | ||
| if current_project is None: | ||
| diff.adds.append(row) |
There was a problem hiding this comment.
I guess if target_release_key in snapshot.release_keys was intended to be the global check here (it's below in the code), but because there's a continue it's never reached. Basically this check is only looking at whether it's a release key in the existing committee, whereas the later check, which is not reached, would check whether it's a release key in the whole database. Small point, but also easy to fix.
There was a problem hiding this comment.
Good spot. It's the two separate branches - the latter being one for moving to another committee. So we don't actually check existence when we're adding a new row! That second check is still correct for moving a release, but we do need to check for conflict in the first case.
| .values(version_key=new_key, project_key=move.to_project) | ||
| ) | ||
| # Other release-key children re-point to the new key | ||
| for model, attr in repoint.RELEASE_KEY_REFS: |
There was a problem hiding this comment.
This contains sql.Task.version_key, but it shouldn't do because that's not a release key (example-0.1), it's just the version (0.1):
RELEASE_KEY_REFS: Final[list[tuple[type[sqlmodel.SQLModel], str]]] = [
(sql.Release, "key"),
(sql.Artifact, "release_key"),
(sql.LifecycleEvent, "version_key"),
(sql.CheckResult, "release_key"),
(sql.BallotPaper, "release_key"),
(sql.Distribution, "release_key"),
(sql.Quarantined, "release_key"),
(sql.Revision, "release_key"),
(sql.Task, "version_key"),
]
There was a problem hiding this comment.
So I think Tasks will have to be handled separately.
There was a problem hiding this comment.
The (sql.LifecycleEvent, "version_key") entry looks suspicious too, but I didn't check it.
There was a problem hiding this comment.
Two issues here - 1. Claude was overzealous in finding release keys and I missed it (it identified version_key there because it is called that in LifecycleEvent - I'll raise a bug and fix later). 2. I neglected to note in this part that tasks don't need moved since the whole admin page doesn't let you edit projects that have already started using ATR properly (since mangling releases, artifacts etc including on-disk and draft etc. just seems fraught with peril). This is purely for catalogued projects/releases/artifacts which exist in SVN but have no ATR-managed files anywhere. Which won't have tasks!
| existing_pks = {(pk[0], pk[1], pk[2]) for pk in snapshot.artifact_by_dist.values()} | ||
| for row in rows: | ||
| reference_conflict = _artifact_reference_conflict(row, snapshot, known_projects, known_releases) | ||
| if reference_conflict is not None: |
There was a problem hiding this comment.
This only seems to check the destination, not the source.
There was a problem hiding this comment.
Below, if current_project in snapshot.managed_project_keys checks the source for releases. Artifact sources aren't checked.
There was a problem hiding this comment.
This is a check actually intended to prevent using these tools to manipulate ATR-managed releases (where we'll have on-disk files, revisions, etc). But we should be checking the source as well.
2 participants
Adds support for import/export of a PMC's projects, releases, artifacts by an admin.
For review - it's a decent sized change and likely wants some manual testing on dev once deployed! I can do that once it's merged to
tertia