tidy-up

components/proxy/src/main/java/com/hotels/styx/admin/AdminServerBuilder.java

@@ -80,19 +80,13 @@ public class AdminServerBuilder {
     private final Environment environment;
     private final Configuration configuration;
 
-//    private Iterable<NamedPlugin> plugins;
     private Registry<BackendService> backendServicesRegistry;
 
     public AdminServerBuilder(Environment environment) {
         this.environment = environment;
         this.configuration = environment.configuration();
     }
 
-//    public AdminServerBuilder plugins(Iterable<NamedPlugin> plugins) {
-//        this.plugins = requireNonNull(plugins);
-//        return this;
-//    }
-
     public AdminServerBuilder backendServicesRegistry(Registry<BackendService> backendServicesRegistry) {
         this.backendServicesRegistry = requireNonNull(backendServicesRegistry);
         return this;
@@ -178,12 +172,6 @@ private static Iterable<IndexHandler.Link> indexLinkPaths() {
                 link("Plugins", "/admin/plugins"));
     }
 
-//    private List<Route> routesForPlugins() {
-//        return stream(plugins.spliterator(), true)
-//                .flatMap(namedPlugin -> routesForPlugin(namedPlugin).stream())
-//                .collect(toList());
-//    }
-
     private static List<Route> routesForPlugin(NamedPlugin namedPlugin) {
         List<PluginAdminEndpointRoute> routes = pluginAdminEndpointRoutes(namedPlugin);
 

components/proxy/src/main/java/com/hotels/styx/proxy/plugin/PluginSuppliers.java

@@ -36,14 +36,15 @@
 
 /**
  * A helper class for creating plugin supplier objects.
+ *
+ * TODO this class seems to be a bit of a relic, furthermore, since it's no longer loading the plugins itself, the failure handling strategy shouldn't be here
  */
 public class PluginSuppliers {
     public static final String DEFAULT_PLUGINS_METRICS_SCOPE = "styx.plugins";
     private static final Logger LOG = getLogger(PluginSuppliers.class);
 
     private final Configuration configuration;
     private final PluginFactoryLoader pluginFactoryLoader;
-    private final Environment environment;
 
     private final FailureHandlingStrategy<Pair<String, SpiExtension>, ConfiguredPluginFactory> failureHandlingStrategy =
             new FailureHandlingStrategy.Builder<Pair<String, SpiExtension>, ConfiguredPluginFactory>()
@@ -60,7 +61,6 @@ public PluginSuppliers(Environment environment) {
     PluginSuppliers(Environment environment, PluginFactoryLoader pluginFactoryLoader) {
         this.configuration = environment.configuration();
         this.pluginFactoryLoader = requireNonNull(pluginFactoryLoader);
-        this.environment = requireNonNull(environment);
     }
 
     private Optional<PluginsMetadata> readPluginsConfig() {

components/proxy/src/main/java/com/hotels/styx/startup/AdminServerSetUp.java

@@ -22,6 +22,8 @@
 
 /**
  * Used to set-up the administration server for Styx.
+ *
+ * TODO this class has just a single utility method - looks like it's only used in one place too. Just move the method?
  */
 public final class AdminServerSetUp {
     private AdminServerSetUp() {
@@ -30,7 +32,6 @@ private AdminServerSetUp() {
     public static HttpServer createAdminServer(StyxServerComponents config) {
         return new AdminServerBuilder(config.environment())
                 .backendServicesRegistry((Registry<BackendService>) config.services().get("backendServiceRegistry"))
-//                .plugins(config.plugins())
                 .build();
     }
 }

components/proxy/src/main/java/com/hotels/styx/startup/DemoPlugin.java

@@ -38,6 +38,9 @@
 public class DemoPlugin implements Plugin {
     private static final Logger LOGGER = getLogger(DemoPlugin.class);
 
+    /**
+     * TODO we should use some config so that this plugin can demonstrate config loading too.
+     */
     public DemoPlugin() {
         LOGGER.info("Demo plugin constructed");
     }

components/proxy/src/main/java/com/hotels/styx/startup/StyxServerComponents.java

@@ -57,34 +57,22 @@ public class StyxServerComponents {
 
     private final Environment environment;
     private final Map<String, StyxService> services;
-    // TODO we will need to return PluginFactory + Plugin.Environment so that this can be handled independently of the app start-up
     private final List<NamedPlugin> plugins;
-//    private final PluginsLoader pluginsLoader;
 
+    // TODO this method is now big and ugly. split it up.
     private StyxServerComponents(Builder builder) {
         StyxConfig styxConfig = requireNonNull(builder.styxConfig);
 
         this.environment = newEnvironment(styxConfig, builder.metricRegistry);
         builder.loggingSetUp.setUp(environment);
 
-//        this.pluginsLoader = requireNonNull(builder.pluginsLoader);
-
-//        this.plugins = builder.pluginsLoader.load(environment);
-
-        // TODO how to create pluginEnvironment? That's why we created this ugly "PluginsLoader" concept, because it encapsulates that.
-        // TODO the only part of the pluginEnvironment that should actually be handled by the "supplier" of the plugins is the plugin-config object, since that's the only one that
-        // TODO - varies by specific plugin, rather than whether it's a test/actually running
-
         List<ConfiguredPluginFactory> cpfs = builder.configuredPluginFactories;
 
         if (cpfs == null) {
             Iterable<ConfiguredPluginFactory> iterable = new PluginSuppliers(environment).fromConfigurations();
             cpfs = ImmutableList.copyOf(iterable);
         }
 
-        // TODO remove when done
-        LOGGER.info("configuredPluginFactories={}", cpfs);
-
         this.plugins = cpfs.stream().map(cpf -> {
             LOGGER.info("Instantiating Plugin, pluginName={}...", cpf.name());
 
@@ -136,10 +124,6 @@ public List<NamedPlugin> plugins() {
         return plugins;
     }
 
-//    public PluginsLoader pluginsLoader() {
-//        return pluginsLoader;
-//    }
-
     private static Environment newEnvironment(StyxConfig styxConfig, MetricRegistry metricRegistry) {
         return new Environment.Builder()
                 .configuration(styxConfig)
@@ -237,7 +221,7 @@ public StyxServerComponents build() {
     }
 
     /**
-     * TODO this can be moved if we desire.
+     * TODO this can be moved if we desire. If not, replace this comment with proper documentation.
      */
     public static class ConfiguredPluginFactory {
         private final String name;

components/proxy/src/test/java/com/hotels/styx/proxy/plugin/PluginSuppliersTest.java

@@ -46,6 +46,7 @@
 import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.is;
 
+// TODO see TODO in PluginSuppliers class before attempting to fix these tests
 public class PluginSuppliersTest {
     Path FIXTURES_CLASS_PATH = fixturesHome(PluginSuppliersTest.class, "/plugins");
 

distribution/conf/default.yml

@@ -88,6 +88,7 @@ url:
 
 #originRestrictionCookie: yourCookieNameHere
 
+# TODO comment out before raising PR
 plugins:
   active: demo
   all:
@@ -96,5 +97,5 @@ plugins:
         class: "com.hotels.styx.startup.DemoPlugin$Factory"
         classPath: "${STYX_HOME:}"
       config:
-        foo: "bar"
+        foo: "bar" # TODO we should use some real config so we can demo that feature too
 

support/test-api/src/main/java/com/hotels/styx/testapi/StyxServer.java

@@ -43,7 +43,6 @@
 
 import static com.hotels.styx.testapi.ssl.SslTesting.acceptAllSslRequests;
 import static java.util.Objects.requireNonNull;
-import static java.util.stream.Collectors.toList;
 import static java.util.stream.Collectors.toSet;
 
 /**
@@ -65,15 +64,9 @@ private StyxServer(Builder builder) {
 
         MemoryBackedRegistry<com.hotels.styx.api.extension.service.BackendService> backendServicesRegistry = new MemoryBackedRegistry<>();
 
-        List<ConfiguredPluginFactory> cpfs = builder.pluginFactories.stream()
-                .map(pfc -> {
-                    // TODO looks like these classes have identical contents, we can unify them
-                    return new ConfiguredPluginFactory(pfc.name, pfc.pluginFactory, pfc.pluginConfig);
-                }).collect(toList());
-
         StyxServerComponents config = new StyxServerComponents.Builder()
                 .styxConfig(styxConfig(builder))
-                .pluginFactories(cpfs)
+                .pluginFactories(builder.pluginFactories)
                 .additionalServices(ImmutableMap.of("backendServiceRegistry", new RegistryServiceAdapter(backendServicesRegistry)))
                 .build();
 
@@ -176,7 +169,7 @@ public <T> T pluginConfig(Class<T> clazz) {
      */
     public static final class Builder {
         private final Map<String, com.hotels.styx.api.extension.service.BackendService> routes = new HashMap<>();
-        private final List<PluginFactoryConfig> pluginFactories = new ArrayList<>();
+        private final List<ConfiguredPluginFactory> pluginFactories = new ArrayList<>();
         private int proxyHttpPort;
         private int adminHttpPort;
         private int proxyHttpsPort;
@@ -231,12 +224,12 @@ public Builder adminHttpPort(int adminPort) {
          * @return this builder
          */
         public Builder addPlugin(String name, Plugin plugin) {
-            pluginFactories.add(new PluginFactoryConfig(name, env -> plugin, null));
+            pluginFactories.add(new ConfiguredPluginFactory(name, env -> plugin, null));
             return this;
         }
 
         public Builder addPluginFactory(String name, PluginFactory pluginFactory, Object pluginConfig) {
-            pluginFactories.add(new PluginFactoryConfig(name, pluginFactory, pluginConfig));
+            pluginFactories.add(new ConfiguredPluginFactory(name, pluginFactory, pluginConfig));
             return this;
         }
 

system-tests/e2e-suite/src/test/scala/com/hotels/styx/startup/StartupSpec.scala

@@ -25,6 +25,7 @@ import com.hotels.styx.support.configuration.{HttpBackend, Origins, StyxConfig}
 import com.hotels.styx.{DefaultStyxConfiguration, StyxProxySpec}
 import org.scalatest.FunSpec
 
+// TODO use Spec this to test the liveness/readiness checks once refactoring is finished
 class StartupSpec extends FunSpec
   with StyxProxySpec
 //  with DefaultStyxConfiguration