fix: remove erroneous spi warnings (#33648)

closes: #34057

Signed-off-by: Steve Hawkins <shawkins@redhat.com>

quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/KeycloakMain.java

@@ -97,7 +97,8 @@ public class KeycloakMain implements QuarkusApplication {
 
             try {
                 PropertyMappers.sanitizeDisabledMappers();
-                Picocli.validateConfig(cliArgs, new Start());
+                PrintWriter outStream = new PrintWriter(System.out, true);
+                Picocli.validateConfig(cliArgs, new Start(), outStream);
             } catch (PropertyException | ProfileException e) {
                 handleUsageError(e.getMessage(), e.getCause());
                 return;

quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java

@@ -58,7 +58,6 @@ import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
 import org.eclipse.microprofile.config.spi.ConfigSource;
-import org.jboss.logging.Logger;
 import org.keycloak.common.profile.ProfileException;
 import org.keycloak.config.DeprecatedMetadata;
 import org.keycloak.config.Option;
@@ -81,7 +80,9 @@ import org.keycloak.quarkus.runtime.configuration.QuarkusPropertiesConfigSource;
 import org.keycloak.quarkus.runtime.configuration.mappers.PropertyMappers;
 import org.keycloak.quarkus.runtime.configuration.mappers.PropertyMapper;
 import org.keycloak.quarkus.runtime.Environment;
+import org.keycloak.quarkus.runtime.KeycloakMain;
 
+import io.quarkus.bootstrap.runner.QuarkusEntryPoint;
 import io.smallrye.config.ConfigValue;
 
 import picocli.CommandLine;
@@ -89,6 +90,9 @@ import picocli.CommandLine.ParameterException;
 import picocli.CommandLine.ParseResult;
 import picocli.CommandLine.DuplicateOptionAnnotationsException;
 import picocli.CommandLine.Help.Ansi;
+import picocli.CommandLine.Help.Ansi.Style;
+import picocli.CommandLine.Help.ColorScheme;
+import picocli.CommandLine.IFactory;
 import picocli.CommandLine.Model.CommandSpec;
 import picocli.CommandLine.Model.ISetter;
 import picocli.CommandLine.Model.OptionSpec;
@@ -349,8 +353,9 @@ public class Picocli {
      *
      * @param cliArgs
      * @param abstractCommand
+     * @param outWriter 
      */
-    public static void validateConfig(List<String> cliArgs, AbstractCommand abstractCommand) {
+    public static void validateConfig(List<String> cliArgs, AbstractCommand abstractCommand, PrintWriter outWriter) {
         IncludeOptions options = getIncludeOptions(cliArgs, abstractCommand, abstractCommand.getName());
 
         if (!options.includeBuildTime && !options.includeRuntime) {
@@ -427,24 +432,22 @@ public class Picocli {
                 }
             }
 
-            Logger logger = Logger.getLogger(Picocli.class); // logger can't be instantiated in a class field
-
             if (!ignoredBuildTime.isEmpty()) {
                 throw new PropertyException(format("The following build time options have values that differ from what is persisted - the new values will NOT be used until another build is run: %s\n",
                         String.join(", ", ignoredBuildTime)));
             } else if (!ignoredRunTime.isEmpty()) {
-                logger.warn(format("The following run time options were found, but will be ignored during build time: %s\n",
+                warn(format("The following run time options were found, but will be ignored during build time: %s\n",
-                        String.join(", ", ignoredRunTime)));
+                        String.join(", ", ignoredRunTime)), outWriter);
             }
 
             if (!disabledBuildTime.isEmpty()) {
-                outputDisabledProperties(disabledBuildTime, true, logger);
+                outputDisabledProperties(disabledBuildTime, true, outWriter);
             } else if (!disabledRunTime.isEmpty()) {
-                outputDisabledProperties(disabledRunTime, false, logger);
+                outputDisabledProperties(disabledRunTime, false, outWriter);
             }
 
             if (!deprecatedInUse.isEmpty()) {
-                logger.warn("The following used options or option values are DEPRECATED and will be removed or their behaviour changed in a future release:\n" + String.join("\n", deprecatedInUse) + "\nConsult the Release Notes for details.");
+                warn("The following used options or option values are DEPRECATED and will be removed or their behaviour changed in a future release:\n" + String.join("\n", deprecatedInUse) + "\nConsult the Release Notes for details.", outWriter);
             }
         } finally {
             DisabledMappersInterceptor.enable(disabledMappersInterceptorEnabled);
@@ -544,10 +547,15 @@ public class Picocli {
         disabledInUse.add(sb.toString());
     }
     
-    private static void outputDisabledProperties(Set<String> properties, boolean build, Logger logger) {
+    private static void warn(String text, PrintWriter outwriter) {
-        logger.warn(format("The following used %s time options are UNAVAILABLE and will be ignored during %s time:\n %s",
+        ColorScheme defaultColorScheme = picocli.CommandLine.Help.defaultColorScheme(Help.Ansi.AUTO);
+        outwriter.println(defaultColorScheme.apply("WARNING: ", Arrays.asList(Style.fg_yellow, Style.bold)) + text);
+    }
+
+    private static void outputDisabledProperties(Set<String> properties, boolean build, PrintWriter outWriter) {
+        warn(format("The following used %s time options are UNAVAILABLE and will be ignored during %s time:\n %s",
                 build ? "build" : "run", build ? "run" : "build",
-                String.join("\n", properties)));
+                String.join("\n", properties)), outWriter);
     }
 
     private static boolean hasConfigChanges(CommandLine cmdCommand) {
@@ -671,7 +679,16 @@ public class Picocli {
     }
 
     public CommandLine createCommandLine(Consumer<CommandSpec> consumer) {
-        CommandSpec spec = CommandSpec.forAnnotatedObject(new Main()).name(Environment.getCommand());
+        CommandSpec spec = CommandSpec.forAnnotatedObject(new Main(), new IFactory() {
+            @Override
+            public <K> K create(Class<K> cls) throws Exception {
+                K result = CommandLine.defaultFactory().create(cls);
+                if (result instanceof AbstractCommand ac) {
+                    ac.setPicocli(Picocli.this);
+                }
+                return result;
+            }
+        }).name(Environment.getCommand());
         consumer.accept(spec);
 
         CommandLine cmd = new CommandLine(spec);
@@ -681,7 +698,7 @@ public class Picocli {
         cmd.setHelpFactory(new HelpFactory());
         cmd.getHelpSectionMap().put(SECTION_KEY_COMMAND_LIST, new SubCommandListRenderer());
         cmd.setErr(getErrWriter());
-
+        cmd.setOut(getOutWriter());
         return cmd;
     }
 
@@ -689,6 +706,10 @@ public class Picocli {
         return new PrintWriter(System.err, true);
     }
     
+    protected PrintWriter getOutWriter() {
+        return new PrintWriter(System.out, true);
+    }
+
     private static void addHelp(CommandSpec currentSpec) {
         try {
             currentSpec.addOption(OptionSpec.builder(Help.OPTION_NAMES)
@@ -961,4 +982,13 @@ public class Picocli {
             }
         }
     }
+
+    public void start(CommandLine cmd) {
+        KeycloakMain.start((ExecutionExceptionHandler) cmd.getExecutionExceptionHandler(), cmd.getErr(), cmd.getParseResult().originalArgs().toArray(new String[0]));
+    }
+
+    public void build() throws Throwable {
+        QuarkusEntryPoint.main();
+    }
+
 }

quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractCommand.java

@@ -34,6 +34,7 @@ public abstract class AbstractCommand {
 
     @Spec
     protected CommandSpec spec;
+    protected Picocli picocli;
 
     protected void executionError(CommandLine cmd, String message) {
         executionError(cmd, message, null);
@@ -65,7 +66,7 @@ public abstract class AbstractCommand {
     }
 
     protected void validateConfig() {
-        Picocli.validateConfig(ConfigArgsConfigSource.getAllCliArgs(), this);
+        Picocli.validateConfig(ConfigArgsConfigSource.getAllCliArgs(), this, spec.commandLine().getOut());
     }
 
     public abstract String getName();
@@ -74,4 +75,8 @@ public abstract class AbstractCommand {
         return spec.commandLine();
     }
     
+    public void setPicocli(Picocli picocli) {
+        this.picocli = picocli;
+    }
+
 }

quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractStartCommand.java

@@ -25,7 +25,6 @@ import org.keycloak.quarkus.runtime.cli.ExecutionExceptionHandler;
 import org.keycloak.quarkus.runtime.configuration.ConfigArgsConfigSource;
 import org.keycloak.quarkus.runtime.configuration.mappers.HostnameV2PropertyMappers;
 import org.keycloak.quarkus.runtime.configuration.mappers.HttpPropertyMappers;
-import org.keycloak.url.HostnameV2ProviderFactory;
 
 import java.util.EnumSet;
 import java.util.List;
@@ -38,8 +37,6 @@ import static org.keycloak.quarkus.runtime.configuration.Configuration.getRawPer
 public abstract class AbstractStartCommand extends AbstractCommand implements Runnable {
     public static final String OPTIMIZED_BUILD_OPTION_LONG = "--optimized";
     
-    private boolean skipStart;
-
     @Override
     public void run() {
         Environment.setParsedCommand(this);
@@ -53,9 +50,7 @@ public abstract class AbstractStartCommand extends AbstractCommand implements Ru
             executionError(spec.commandLine(), Messages.optimizedUsedForFirstStartup());
         }
         
-        if (!skipStart) {
+        picocli.start(cmd);
-            KeycloakMain.start((ExecutionExceptionHandler) cmd.getExecutionExceptionHandler(), cmd.getErr(), cmd.getParseResult().originalArgs().toArray(new String[0]));
-        }
     }
 
     protected void doBeforeRun() {
@@ -76,8 +71,4 @@ public abstract class AbstractStartCommand extends AbstractCommand implements Ru
         return EnumSet.of(OptionCategory.IMPORT, OptionCategory.EXPORT);
     }
     
-    public void setSkipStart(boolean skipStart) {
-        this.skipStart = skipStart;
-    }
-
 }

quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/Build.java

@@ -79,7 +79,7 @@ public final class Build extends AbstractCommand implements Runnable {
             configureBuildClassLoader();
 
             beforeReaugmentationOnWindows();
-            QuarkusEntryPoint.main();
+            picocli.build();
 
             if (!isDevProfile()) {
                 println(spec.commandLine(), "Server configuration updated and persisted. Run the following command to review the configuration:\n");
@@ -133,7 +133,7 @@ public final class Build extends AbstractCommand implements Runnable {
     private void cleanTempResources() {
         if (!LaunchMode.current().isDevOrTest()) {
             // only needed for dev/testing purposes
-            getHomePath().resolve("quarkus-artifact.properties").toFile().delete();
+            Optional.ofNullable(getHomePath()).ifPresent(path -> path.resolve("quarkus-artifact.properties").toFile().delete());
         }
     }
 

quarkus/runtime/src/test/java/or/keycloak/quarkus/runtime/cli/PicocliTest.java

@@ -19,17 +19,16 @@ package or.keycloak.quarkus.runtime.cli;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.not;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 
 import java.io.PrintWriter;
 import java.io.StringWriter;
-import java.util.Arrays;
 import java.util.List;
 
 import org.junit.Test;
 import org.keycloak.quarkus.runtime.cli.Picocli;
-import org.keycloak.quarkus.runtime.cli.command.AbstractStartCommand;
 import org.keycloak.quarkus.runtime.configuration.ConfigArgsConfigSource;
 import org.keycloak.quarkus.runtime.configuration.test.AbstractConfigurationTest;
 
@@ -43,14 +42,23 @@ public class PicocliTest extends AbstractConfigurationTest {
     private class NonRunningPicocli extends Picocli {
 
         final StringWriter err = new StringWriter();
+        final StringWriter out = new StringWriter();
         SmallRyeConfig config;
         int exitCode = Integer.MAX_VALUE;
 
         String getErrString() {
+            return normalize(err);
+        }
+        
         // normalize line endings - TODO: could also normalize non-printable chars
         // but for now those are part of the expected output
-            return System.lineSeparator().equals("\n") ? err.toString()
+        String normalize(StringWriter writer) {
-                    : err.toString().replace(System.lineSeparator(), "\n");
+            return System.lineSeparator().equals("\n") ? writer.toString()
+                    : writer.toString().replace(System.lineSeparator(), "\n");
+        }
+        
+        String getOutString() {
+            return normalize(out);
         }
 
         @Override
@@ -58,6 +66,11 @@ public class PicocliTest extends AbstractConfigurationTest {
             return new PrintWriter(err, true);
         }
         
+        @Override
+        protected PrintWriter getOutWriter() {
+            return new PrintWriter(out, true);
+        }
+
         @Override
         protected void exitOnFailure(int exitCode, CommandLine cmd) {
             this.exitCode = exitCode;
@@ -68,32 +81,28 @@ public class PicocliTest extends AbstractConfigurationTest {
         };
 
         @Override
-        protected int run(CommandLine cmd, String[] argArray) {
+        public void parseAndRun(List<String> cliArgs) {
-            skipStart(cmd);
+            config = createConfig();
-            return super.run(cmd, argArray);
+            super.parseAndRun(cliArgs);
-        }
-
-        private void skipStart(CommandLine cmd) {
-            for (CommandLine sub : cmd.getSubcommands().values()) {
-                if (sub.getCommand() instanceof AbstractStartCommand) {
-                    ((AbstractStartCommand) (sub.getCommand())).setSkipStart(true);
-                }
-                skipStart(sub);
-            }
         }
         
         @Override
-        public void parseAndRun(List<String> cliArgs) {
+        public void start(CommandLine cmd) {
-            ConfigArgsConfigSource.setCliArgs(cliArgs.toArray(String[]::new));
+            // skip
-            config = createConfig();
+        }
-            super.parseAndRun(cliArgs);
+        
+        @Override
+        public void build() throws Throwable {
+            // skip
         }
 
     };
 
     NonRunningPicocli pseudoLaunch(String... args) {
         NonRunningPicocli nonRunningPicocli = new NonRunningPicocli();
-        nonRunningPicocli.parseAndRun(Arrays.asList(args));
+        ConfigArgsConfigSource.setCliArgs(args);
+        var cliArgs = Picocli.parseArgs(args);
+        nonRunningPicocli.parseAndRun(cliArgs);
         return nonRunningPicocli;
     }
 
@@ -206,4 +215,18 @@ public class PicocliTest extends AbstractConfigurationTest {
                 "Option: '--db postgres' is not expected to contain whitespace, please remove any unnecessary quoting/escaping"));
     }
     
+    @Test
+    public void spiRuntimeAllowedWithStart() {
+        NonRunningPicocli nonRunningPicocli = pseudoLaunch("start", "--http-enabled=true", "--spi-something-pass=changeme");
+        assertEquals(CommandLine.ExitCode.OK, nonRunningPicocli.exitCode);
+        assertThat(nonRunningPicocli.getOutString(), not(containsString("kc.spi-something-pass")));
+    }
+    
+    @Test
+    public void spiRuntimeWarnWithBuild() {
+        NonRunningPicocli nonRunningPicocli = pseudoLaunch("build", "--spi-something-pass=changeme");
+        assertEquals(CommandLine.ExitCode.OK, nonRunningPicocli.exitCode);
+        assertThat(nonRunningPicocli.getOutString(), containsString("The following run time options were found, but will be ignored during build time: kc.spi-something-pass"));
+    }
+    
 }