royteeuwen commented on code in PR #40:
URL:
https://github.com/apache/sling-org-apache-sling-committer-cli/pull/40#discussion_r3781511107
##########
src/main/java/org/apache/sling/cli/impl/release/UpdateDistCommand.java:
##########
@@ -390,6 +390,17 @@ private static boolean belongsToVersion(String fileName,
String artifactId, Stri
return next == '.' || next == '-';
}
+ /**
+ * Returns the names of every {@code .pom} published in {@code
dist/release} for {@code version}, across
+ * all artifacts. Used to resolve a release's artifact ids from the
released POMs once the staging
+ * repository is gone; the caller narrows the candidates down by reading
each POM's {@code <name>}.
+ */
+ static List<String> listReleasePomFileNames(String version) throws
IOException {
Review Comment:
Agreed — the dist.apache.org access is now a `DistRepository` class in a new
`org.apache.sling.cli.impl.dist` package (matching how `nexus`, `jira` and
`pgp` are grouped). It owns the SVN listing, the previous-version selection and
the publish, so both `update-dist` and the website update call it instead of
reaching into a command. `UpdateDistCommand` keeps only what is its own: the
staged-artifact download and the release plan.
##########
src/main/java/org/apache/sling/cli/impl/release/UpdateLocalSiteCommand.java:
##########
@@ -46,63 +76,289 @@
})
@CommandLine.Command(
name = UpdateLocalSiteCommand.NAME,
- description = "Updates the Sling website with the new release
information, " + "based on a local checkout",
+ description = "Updates the Sling website with the new release
information, based on a local checkout",
subcommands = CommandLine.HelpCommand.class)
public class UpdateLocalSiteCommand extends AbstractReleaseCommand {
static final String GROUP = "release";
static final String NAME = "update-local-site";
- private static final String GIT_CHECKOUT = "/tmp/sling-site";
+ /** System property, or {@code SLING_CLI_SITE_CHECKOUT} environment
variable, overriding the checkout. */
+ static final String CHECKOUT_PROPERTY = "sling.cli.site.checkout";
+
+ /** Cloned over https from gitbox so the same ASF credentials that commit
to dist.apache.org can push. */
+ static final String SITE_GIT_URL =
"https://gitbox.apache.org/repos/asf/sling-site.git";;
+
+ /**
+ * Where the site is checked out. Deliberately not under the shared
temporary directory: the checkout is
+ * reused across runs and is committed from, so a world-writable location
would let anything else on the
+ * host substitute the content that gets pushed to the website. Resolved
per call so it can be redirected.
+ */
+ static String checkoutDir() {
+ String configured = System.getProperty(CHECKOUT_PROPERTY);
+ if (configured == null || configured.isBlank()) {
+ configured = System.getenv("SLING_CLI_SITE_CHECKOUT");
Review Comment:
You are right, and my reason was weak: the env var only existed because the
helpers shared by `update-local-site`, `update-news` and `finalize` resolved it
from global state. It is now a `--site-checkout` option (a small
`SiteCheckoutOptions` mixin, like `ReusableCLIOptions`) and the path is
threaded through as a parameter, so there is no hidden global state — that also
removed the `System.setProperty` juggling the tests needed. The environment
variable is gone entirely, not kept as a fallback: nothing else in the CLI
takes an option that way, the only other env vars being the ASF credentials,
which are secrets passed via `--env-file`.
##########
src/main/java/org/apache/sling/cli/impl/DateProvider.java:
##########
@@ -30,7 +31,7 @@ public class DateProvider {
private static final DateTimeFormatter jiraReleaseDate =
DateTimeFormatter.ofPattern("yyyy-MM-d");
public OffsetDateTime getCurrentDate() {
- return OffsetDateTime.now();
+ return OffsetDateTime.now(ZoneId.systemDefault());
Review Comment:
It is a fix for SonarCloud `java:S8688` ("Explicitly specify the time zone
by passing a ZoneId or a Clock to the .now() method"), which was flagged as an
existing issue on `DateProvider` — `OffsetDateTime.now()` became
`OffsetDateTime.now(ZoneId.systemDefault())`, so behaviour is unchanged. You
are right that it is unrelated to this PR; I picked it up while clearing the
Sonar remarks the analysis raised here. Happy to pull it into a separate PR if
you would rather keep this one focused.
##########
src/test/java/org/apache/sling/cli/impl/release/UpdateNewsCommandTest.java:
##########
@@ -0,0 +1,257 @@
+/*
+ * 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.
+ */
+package org.apache.sling.cli.impl.release;
+
+import java.io.IOException;
+import java.util.Set;
+
+import org.apache.commons.lang3.reflect.FieldUtils;
+import org.apache.sling.cli.impl.Command;
+import org.apache.sling.cli.impl.Credentials;
+import org.apache.sling.cli.impl.CredentialsService;
+import org.apache.sling.cli.impl.ExecutionMode;
+import org.apache.sling.cli.impl.jbake.JBakeContentUpdater;
+import org.apache.sling.cli.impl.junit.LogCapture;
+import org.apache.sling.cli.impl.nexus.RepositoryService;
+import org.apache.sling.cli.impl.nexus.StagingRepository;
+import org.apache.sling.cli.impl.people.Member;
+import org.apache.sling.cli.impl.people.MembersFinder;
+import org.apache.sling.testing.mock.osgi.junit.OsgiContext;
+import org.eclipse.jgit.api.AddCommand;
+import org.eclipse.jgit.api.CloneCommand;
+import org.eclipse.jgit.api.CommitCommand;
+import org.eclipse.jgit.api.DiffCommand;
+import org.eclipse.jgit.api.FetchCommand;
+import org.eclipse.jgit.api.Git;
+import org.eclipse.jgit.api.PushCommand;
+import org.eclipse.jgit.api.ResetCommand;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.mockito.MockedConstruction;
+import org.mockito.MockedStatic;
+import picocli.CommandLine;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.ArgumentMatchers.isNull;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.mockConstruction;
+import static org.mockito.Mockito.mockStatic;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+public class UpdateNewsCommandTest {
+
+ @Rule
+ public final OsgiContext osgiContext = new OsgiContext();
+
+ @Rule
+ public final LogCapture logCapture = new
LogCapture(UpdateNewsCommand.class);
+
+ @Rule
+ public final TemporaryFolder checkout = new TemporaryFolder();
+
+ /** Keeps the site checkout inside the test's temporary folder rather than
the real user's home. */
+ @Before
+ public void redirectCheckout() {
+ System.setProperty(
+ UpdateLocalSiteCommand.CHECKOUT_PROPERTY,
checkout.getRoot().getAbsolutePath());
+ }
+
+ @After
+ public void restoreCheckout() {
+ System.clearProperty(UpdateLocalSiteCommand.CHECKOUT_PROPERTY);
+ }
+
+ private PushCommand pushCommand;
+ private CommitCommand commitCommand;
+
+ /** Stubs out JGit so nothing is cloned, opened, reset or pushed for real.
*/
+ private MockedStatic<Git> stubGit() {
Review Comment:
Fair, and it turned out to be more than a style point: those mocks had to be
taught about `checkout()`, `setDepth()` and `setCommitter()` one breakage at a
time, and kept passing while the real behaviour was broken. Both test classes
now run against a real repository instead — a `SiteRepository` rule seeds a
bare "upstream" plus a checkout from the existing site fixtures, and the tests
clone, fetch, commit and push between them, entirely locally. That matches the
rest of the suite, where `MockJira` and `MockNexus` are real local HTTP servers
rather than mocked clients. It paid for itself immediately: it caught that the
commit had the wrong committer identity, which the mocked version could not
see. Only the network services (Nexus, dist.apache.org, Whimsy) are still
mocked.
--
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]
