SLING-13319 - Support dist-promotion for multiple artifacts

[jsedding]

No description provided.

[sonarqubecloud]

Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
82.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[royteeuwen]

Thanks for tackling this — supporting multi-artifact staging repositories is definitely needed. I built the branch locally: it compiles, spotless:check is clean and all 36 tests in UpdateDistCommandTest + FinalizeCommandTest pass. But I think there are a few correctness problems that the current tests don't reach, a couple of which can leave dist.apache.org in a state that can't be repaired by re-running the command.

Blocking

1. Partial publishes become permanently unrecoverable — planDistRelease returns only the already-published plans if any artifact is already published, silently dropping the ones that still need publishing. Details inline.
2. An artifact with zero staged files gets its previous release deleted — noArtifacts is now checked across all plans instead of per plan. Details inline.
3. publish() per artifact = N separate SVN commits — DistRepository.publish promises "a single atomic revision" and commitFiles aborts the edit on failure precisely so a partial commit is never left behind. Calling it in a loop breaks that guarantee at the release level: a multi-artifact release becomes N commits on dist.apache.org, and a failure halfway leaves it half-published (which then runs into (1)). It'd be better to collect all plans' files into a single publish call.
4. Version-prefix confusion in the new file filter — plain startsWith reintroduces the 1.0.14 / 1.0.140 bug that DistRepository.belongsToVersion exists to prevent. Details inline.
5. FinalizeCommand discards the exit code — a failed dist update no longer aborts finalize before the irreversible promote to Maven Central. Details inline.

Test feedback

• Unrelated comment churn. A rename refactor leaked into comments and assertion messages across UpdateDistCommandTest — "the new newVersion", "a newer newVersion", "a different major newVersion", "when an explicit newVersion is given". About ten hunks of pure noise that make the comments read wrong; could you revert those?
• The new test doesn't exercise the new code. testAutoDeduceIdentifiesMultiArtifactReleases only calls DistRepository.listPreviousReleaseFiles, which this PR doesn't touch. Noted inline with the cases I think need covering.
• testDryRunPmc is now vacuous — noted inline.

Happy to help with any of this if it's easier to split the fixes out.

[royteeuwen]

Blocking: partial publishes become permanently unrecoverable.

This filter keeps only the artifacts that are already published, and then if (!plans.isEmpty()) return plans; throws away every artifact that still needs publishing. So if even one artifact of a multi-artifact release is already in dist/release, the others are silently dropped from the plan — and doUpdateDist's plans.stream().allMatch(DistReleasePlan::alreadyPublished) is then trivially true, so it logs "nothing to do" and exits OK.

This is exactly the state you land in after a partially-failed run (see my comment on the per-plan DistRepository.publish call): artifact A committed, artifact B failed → re-run update-dist → "nothing to do", forever, with no way to recover other than doing it by hand.

The whole point of isVersionPublished was to make re-runs safe and this inverts it. I'd suggest a single pass that builds one plan per artifact with its own alreadyPublished flag, and then has doUpdateDist skip the published ones and publish the rest.

[royteeuwen]

Blocking: this can delete an artifact's previous release without publishing the new one.

noArtifacts is now true only if every plan has empty newFiles. If artifact A has staged files and B doesn't, B's plan still reaches DistRepository.publish(id, ver, List.of(), oldFiles, ...) below — and commitFiles has no guard for an empty newFiles, so it happily deletes oldFiles and adds nothing. B's previous release is then gone from dist/release.

The newFiles().isEmpty() check needs to stay per-plan (skip that plan, or fail the whole command).

[royteeuwen]

Blocking: startsWith reintroduces the version-prefix bug.

Publishing 1.0.14 will pick up 1.0.140's files here. DistRepository.belongsToVersion already handles exactly this (there's even a test guarding it — testAutoDeduceDoesNotConfuseVersionPrefixesAndKeepsNewerVersions); it's just private. Could you widen it to package-visible and reuse it instead?

Separately, a behaviour change worth confirming is intentional: previously every downloaded file was published, now only files matching some pom's artifactId-version prefix are, and anything that doesn't match is silently dropped.

[royteeuwen]

Minor: collectDownloadedFiles walks the entire download tree once per artifact. Worth hoisting the call out of the map and filtering the single result per artifact.

[royteeuwen]

Blocking (see summary item 3): calling publish once per plan turns a multi-artifact release into N separate SVN commits on dist.apache.org. DistRepository.publish's javadoc promises "a single atomic revision", and commitFiles aborts the edit on failure specifically so a partial commit is never left behind — that guarantee no longer holds for the release as a whole.

Collecting all plans' newFiles/oldFiles into one publish call would keep it atomic (the commit message would need to name the set rather than a single artifactId version).

[royteeuwen]

Nit: stepUpdateDistStage receives mode and it's now unused — this reaches for reusableCLIOptions.executionMode instead. Same value today, but inconsistent with the rest of the class (stepPromoteStage, stepUpdateSite, … all use the parameter).

Worth calling out too: the old stepUpdateDist had no INTERACTIVE branch and published directly, so finalize will now prompt per artifact mid-run. Probably an improvement, but it's an unannounced behaviour change.

[royteeuwen]

This looks like an accidental rename-in-comments: "the new newVersion". It recurs about ten times through this file (lines ~95, 108, 118, 124, 127, 206, 261, 269-270, 273, 386) and makes the comments read wrong. Could you revert the comment/assertion-message churn so the diff stays focused?

[royteeuwen]

This test doesn't actually exercise the new code — it only calls DistRepository.listPreviousReleaseFiles, which this PR doesn't touch. It never goes through planDistRelease with multiple artifacts, nor doUpdateDist over multiple plans. Both ArtifactUpdate entries also share the same artifactId with different versions, so it isn't really a multi-artifact release either.

The cases I'd most like to see covered, since they're the ones that bite in production:

• multiple pom artifacts in the staging repo → one plan each, correct newFiles split between them
• one artifact already published + one not → the unpublished one still gets published (currently broken)
• one artifact with no staged files → its oldFiles are not deleted (currently broken)

[royteeuwen]

Stray ; after the record declaration. Also newArtifact() above is unused.

[royteeuwen]

testDryRunPmc is vacuous now: with mockStatic(UpdateDistCommand.class) and no thenCallRealMethod() for doUpdateDist, the entire dist step is a no-op — this planDistRelease stub is never reached and the verify(DistRepository.publish, never()) below passes trivially. testAutoPmc got the thenCallRealMethod(); this test needs it too.

[royteeuwen]

Blocking: --previous-version is broken for multi-artifact releases — it aborts the run half-way and lands you in the unrecoverable state from my other comment.

listPreviousReleaseFiles short-circuits on an explicit previous version and returns listFiles(DIST_RELEASE_URL, artifactId + "-" + explicitPreviousVersion) — it never consults newVersion. Since every plan here is handed the same single previousVersion, two plans for the same artifactId get an identical oldFiles list, both computed before any publish runs.

Using this PR's own motivating scenario (org.apache.sling.servlets.resolver 2.12.0 + 3.0.10 staged together) with --previous-version 2.11.4:

1. plan 1 commits: adds 2.12.0, deletes the 2.11.4 files
2. plan 2 commits: deleteEntry on those same files, which no longer exist at HEAD

I reproduced this against a real local SVN repo using the harness already in UpdateDistCommandTest (createLocalRepoWithReleaseFiles), calling the 6-arg publish twice with the same oldFiles:

>>> after plan 1: [org.apache.sling.servlets.resolver-2.12.0.pom]
>>> plan 2 FAILED: java.io.IOException: Failed to update dist.apache.org
>>> final:        [org.apache.sling.servlets.resolver-2.12.0.pom]

commitFiles aborts the edit and rethrows, so 3.0.10 never reaches dist. And because 2.12.0 did land, re-running update-dist now hits the already-published short-circuit and reports "nothing to do" — so the release cannot be completed by re-running, only by hand.

Even leaving the failure aside, the option is semantically unusable for a multi-artifact release: you can only name one previous version, so 3.0.8 (the actual predecessor of the 3.0.x stream) is never removed. And for two different artifact ids it would delete B-<thatVersion> if such a file happens to exist for B, which isn't what the operator asked for.

Options, roughly in order of how much work they are:

• reject --previous-version when the staging repository contains more than one artifact (smallest fix, keeps the flag honest)
• make it accept per-artifact values, e.g. --previous-version <artifactId>=<version> (repeatable)
• at minimum, if the per-plan publish calls are collapsed into one atomic commit as suggested above, de-duplicate oldFiles across plans so the double-delete can't happen

A regression test for this is cheap — createLocalRepoWithReleaseFiles plus two plans sharing a previous version reproduces it in a few lines with no network.