Skip to content

fix: preventing args to service commands #45050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
shawkins wants to merge 1 commit into
base: main
Choose a base branch
from
+67 −50
Draft

fix: preventing args to service commands #45050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 3 additions & 16 deletions quarkus/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,21 +121,8 @@ Example:

### Updating Expectations

Changing the help output will cause HelpCommandDistTest to fail. This test uses [ApprovalTests](https://github.com/approvals/ApprovalTests.Java) which creates `.received.txt` files containing the actual output when tests fail. To update the expected output (see [Approving The Result](https://github.com/approvals/ApprovalTests.Java/blob/master/approvaltests/docs/tutorials/GettingStarted.md#approving-the-result)):
Changing to the help output will cause HelpCommandDistTest to fail. This test uses [ApprovalTests](https://github.com/approvals/ApprovalTests.Java) which creates `.received.txt` files containing the actual output when tests fail. To update the expected output (see [Approving The Result](https://github.com/approvals/ApprovalTests.Java/blob/master/approvaltests/docs/tutorials/GettingStarted.md#approving-the-result)) run:

1. Run the failing test:
```
../mvnw clean install -Dtest=HelpCommandDistTest
```
KEYCLOAK_REPLACE_EXPECTED=true ../mvnw clean install -Dtest=HelpCommandDistTest

2. Review the generated `.received.txt` files in the test directory and compare them with the `.approved.txt` files.

3. If the changes look correct, rename the `.received.txt` files to `.approved.txt` to approve the new output:
```
# Example for a specific test
mv HelpCommandDistTest.testHelp.received.txt HelpCommandDistTest.testHelp.approved.txt
```

Note: If the files match, the received file will be deleted automatically. You must include the `.approved.` files in source control.

Alternatively, you can configure an [approval reporter](https://github.com/approvals/ApprovalTests.Java/blob/master/approvaltests/docs/reference/Reporters.md) to use a diff tool for easier comparison.
then use a diff to ensure the changes look good.
31 changes: 18 additions & 13 deletions quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import org.keycloak.quarkus.runtime.cli.command.AbstractNonServerCommand;
import org.keycloak.quarkus.runtime.cli.command.Build;
import org.keycloak.quarkus.runtime.cli.command.Main;
import org.keycloak.quarkus.runtime.cli.command.ShowConfig;
import org.keycloak.quarkus.runtime.cli.command.Tools;
import org.keycloak.quarkus.runtime.cli.command.WindowsService;
import org.keycloak.quarkus.runtime.configuration.ConfigArgsConfigSource;
Expand Down Expand Up @@ -93,14 +94,15 @@ public class Picocli {
public static final String ARG_SHORT_PREFIX = "-";
public static final String NO_PARAM_LABEL = "none";

private record IncludeOptions(boolean includeRuntime, boolean includeBuildTime) {
private record IncludeOptions(boolean includeRuntime, boolean includeBuildTime, boolean allowUnrecognized) {
}

private final ExecutionExceptionHandler errorHandler = new ExecutionExceptionHandler();
private Optional<AbstractCommand> parsedCommand = Optional.empty();
private boolean warnedTimestampChanged;

private Ansi colorMode = hasColorSupport() ? Ansi.ON : Ansi.OFF;
private IncludeOptions options;

public static boolean hasColorSupport() {
return QuarkusConsole.hasColorSupport();
Expand Down Expand Up @@ -139,7 +141,7 @@ public void parseAndRun(List<String> cliArgs) {
}
initConfig(currentCommand);

if (!unrecognizedArgs.isEmpty()) {
if (!unrecognizedArgs.isEmpty() && options.allowUnrecognized) {
// TODO: further refactor this as these args should be the source for ConfigArgsConfigSource
unrecognizedArgs.removeIf(arg -> {
boolean hasArg = false;
Expand All @@ -157,10 +159,11 @@ public void parseAndRun(List<String> cliArgs) {
}
return false;
});
if (!unrecognizedArgs.isEmpty()) {
addCommandOptions(cl, currentCommand);
throw new KcUnmatchedArgumentException(cl, unrecognizedArgs);
}
}

if (!unrecognizedArgs.isEmpty()) {
addCommandOptions(cl, currentCommand);
throw new KcUnmatchedArgumentException(cl, unrecognizedArgs);
}

if (isHelpRequested(result)) {
Expand Down Expand Up @@ -222,8 +225,6 @@ public void validateConfig() {
}
warnOnDuplicatedOptionsInCli();

IncludeOptions options = getIncludeOptions(abstractCommand);

if (!options.includeBuildTime && !options.includeRuntime) {
return;
}
Expand Down Expand Up @@ -639,7 +640,7 @@ public <K> K create(Class<K> cls) throws Exception {
* Removes platform-specific commands on non-applicable platforms
*/
private void removePlatformSpecificCommands(CommandLine cmd) {
if (!Environment.isWindows()) {
if (!this.showAllCommands() && !Environment.isWindows()) {
CommandLine toolsCmd = cmd.getSubcommands().get(Tools.NAME);
if (toolsCmd != null) {
CommandLine windowsServiceCmd = toolsCmd.getSubcommands().get(WindowsService.NAME);
Expand All @@ -650,6 +651,11 @@ private void removePlatformSpecificCommands(CommandLine cmd) {
}
}

protected boolean showAllCommands() {
// not an official option, just a way for integration tests to produce the same output regardless of OS
return Boolean.parseBoolean(System.getenv("KEYCLOAK_ALL_COMMANDS"));
}

public PrintWriter getErrWriter() {
return new PrintWriter(System.err, true);
}
Expand All @@ -660,16 +666,14 @@ public PrintWriter getOutWriter() {

private IncludeOptions getIncludeOptions(AbstractCommand abstractCommand) {
if (abstractCommand == null) {
return new IncludeOptions(false, false);
return new IncludeOptions(false, false, false);
}
boolean autoBuild = abstractCommand instanceof AbstractAutoBuildCommand;
boolean includeBuildTime = abstractCommand instanceof Build || (autoBuild && !abstractCommand.isOptimized());
return new IncludeOptions(autoBuild, includeBuildTime);
return new IncludeOptions(autoBuild, includeBuildTime, autoBuild || includeBuildTime || abstractCommand instanceof ShowConfig);
}

private void addCommandOptions(CommandLine command, AbstractCommand ac) {
IncludeOptions options = getIncludeOptions(ac);

if (!options.includeBuildTime && !options.includeRuntime) {
return;
}
Expand Down Expand Up @@ -910,6 +914,7 @@ public void initConfig(AbstractCommand command) {
throw new IllegalStateException("Config should not be initialized until profile is determined");
}
this.parsedCommand = Optional.ofNullable(command);
options = getIncludeOptions(command);

if (!Environment.isRebuilt() && command instanceof AbstractAutoBuildCommand
&& !command.isOptimized()) {
Expand Down
24 changes: 24 additions & 0 deletions quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1707,4 +1707,28 @@ public void otelAll() {
"quarkus.otel.logs.enabled", "true"
));
}

@Test
public void testWindowsServiceHelp() {
NonRunningPicocli nonRunningPicocli = new NonRunningPicocli() {
@Override
protected boolean showAllCommands() {
return true;
}
};
KeycloakMain.main(new String[] {"tools", "windows-service", "--help"}, nonRunningPicocli);
assertTrue(nonRunningPicocli.getOutString().contains("install"));
}

@Test
public void testWindowsServiceArgs() {
NonRunningPicocli nonRunningPicocli = new NonRunningPicocli() {
@Override
protected boolean showAllCommands() {
return true;
}
};
KeycloakMain.main(new String[] {"tools", "windows-service", "--db=dev-mem"}, nonRunningPicocli);
assertTrue(nonRunningPicocli.getErrString().contains("Unknown option"));
}
}
43 changes: 22 additions & 21 deletions quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/HelpCommandDistTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,14 @@

package org.keycloak.it.cli.dist;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.List;

import org.keycloak.it.junit5.extension.CLIResult;
import org.keycloak.it.junit5.extension.DistributionTest;
import org.keycloak.it.junit5.extension.RawDistOnly;
import org.keycloak.it.junit5.extension.WithEnvVars;
import org.keycloak.it.utils.KeycloakDistribution;
import org.keycloak.quarkus.runtime.Environment;
import org.keycloak.quarkus.runtime.cli.command.BootstrapAdmin;
Expand All @@ -36,21 +39,24 @@
import org.keycloak.quarkus.runtime.cli.command.UpdateCompatibilityCheck;
import org.keycloak.quarkus.runtime.cli.command.UpdateCompatibilityMetadata;

import com.spun.util.io.FileUtils;
import io.quarkus.test.junit.main.Launch;
import org.apache.commons.io.FileUtils;
import org.approvaltests.Approvals;
import org.approvaltests.core.Options;
import org.approvaltests.core.VerifyResult;
import org.hamcrest.MatcherAssert;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;

import static org.keycloak.quarkus.runtime.cli.command.AbstractAutoBuildCommand.OPTIMIZED_BUILD_OPTION_LONG;

@WithEnvVars({"KEYCLOAK_ALL_COMMANDS", "true"})
Copy link
Contributor

@vmuzikar vmuzikar Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this approach, we won't know the hiding of the OS-specific cocmmands actually works.

Copy link
Contributor Author

@shawkins shawkins Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant to add a Linux specific test showing that these commands won't work there.

The main thought here is that we don't need helpcommanddisttest to be OS dependent.

I'd also be fine to extend this approach to a way to set the OS for picocli, rather than just relying upon the Environment method.

Copy link
Contributor

@vmuzikar vmuzikar Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main thought here is that we don't need helpcommanddisttest to be OS dependent.

Not sure about that. How do we know the OS dependent logic for hiding/removing commands also works correctly for the help command?

@DistributionTest
@Tag(DistributionTest.WIN)
@RawDistOnly(reason = "Verifying the help message output doesn't need long spin-up of docker dist tests.")
public class HelpCommandDistTest {

public static final String REPLACE_EXPECTED = "KEYCLOAK_REPLACE_EXPECTED";

@Test
@Launch({})
void testDefaultToHelp(CLIResult cliResult) {
Expand Down Expand Up @@ -217,24 +223,19 @@ private void assertHelp(CLIResult cliResult) {
.replace("including\nbuild ", "including build\n");
}

// Custom comparator that strips Windows-specific lines from the approved file on non-Windows platforms
Options options = new Options().withComparator((receivedFile, approvedFile) -> {
String received = FileUtils.readFile(receivedFile);
String approved = FileUtils.readFile(approvedFile);

if (!Environment.isWindows()) {
approved = stripWindowsServiceLines(approved);
try {
Approvals.verify(output);
} catch (Error cause) {
if ("true".equals(System.getenv(REPLACE_EXPECTED))) {
try {
FileUtils.write(Approvals.createApprovalNamer().getApprovedFile(".txt"), output,
StandardCharsets.UTF_8);
} catch (IOException e) {
throw new RuntimeException("Failed to assert help, and could not replace expected", cause);
}
} else {
throw cause;
}
return VerifyResult.from(approved.equals(received));
});

Approvals.verify(output, options);
}

private String stripWindowsServiceLines(String text) {
return text
.replaceAll("(?m)^ {4}windows-service\\s+Manage Keycloak as a Windows service\\.\\R", "")
.replaceAll("(?m)^ {6}install\\s+Install Keycloak as a Windows service\\.\\R", "")
.replaceAll("(?m)^ {6}uninstall\\s+Uninstall Keycloak Windows service\\.\\R", "");
}
}
}
Loading