royteeuwen commented on code in PR #56:
URL:
https://github.com/apache/sling-org-apache-sling-committer-cli/pull/56#discussion_r3890413659
##########
src/main/java/org/apache/sling/cli/impl/release/UpdateDistCommand.java:
##########
@@ -169,22 +189,50 @@ record DistReleasePlan(
* Shared by this command and {@link FinalizeCommand} so the flow is not
duplicated. When the version is
* already present in {@code dist/release} the returned plan is marked
{@link DistReleasePlan#alreadyPublished()}.
*/
- static DistReleasePlan planDistRelease(
+ static List<DistReleasePlan> planDistRelease(
RepositoryService repositoryService, StagingRepository repository,
String previousVersion)
throws IOException {
LocalRepository localRepository =
repositoryService.download(repository);
- Artifact primary = localRepository.getArtifacts().stream()
+ List<Artifact> artifacts = localRepository.getArtifacts().stream()
.filter(a -> "pom".equals(a.getType()))
- .findFirst()
- .orElseThrow(() -> new IllegalStateException("No POM artifact
found in staging repository"));
- String artifactId = primary.getArtifactId();
- String newVersion = primary.getVersion();
- if (DistRepository.isVersionPublished(artifactId, newVersion)) {
- return new DistReleasePlan(artifactId, newVersion, List.of(),
List.of(), true);
+ .toList();
+
+ if (artifacts.isEmpty()) {
+ throw new IllegalStateException("No POM artifact found in staging
repository");
+ }
+
+ List<DistReleasePlan> plans = artifacts.stream()
+ .filter(a -> {
+ try {
+ return
DistRepository.isVersionPublished(a.getArtifactId(), a.getVersion());
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ })
+ .map(a -> new DistReleasePlan(a.getArtifactId(),
a.getVersion(), List.of(), List.of(), true))
+ .toList();
+
+ if (!plans.isEmpty()) {
+ return plans;
+ } else {
+ return artifacts.stream()
+ .map(a -> {
+ try {
+ String artifactId = a.getArtifactId();
+ String newVersion = a.getVersion();
+ List<Path> newFiles =
collectDownloadedFiles(localRepository.getRootFolder()).stream()
+ .filter(path ->
+
path.getFileName().toString().startsWith(artifactId + "-" + newVersion))
+ .toList();
+ List<String> oldFiles =
+
DistRepository.listPreviousReleaseFiles(artifactId, newVersion,
previousVersion);
Review Comment:
**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.
--
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]
