[grid] Giving precedence to file configuration over flags

In general, Grids should be configured via files, and before
this change, flags and default values were taking precedence
over files, which broke the expected behaviour. For example,
"relax-checks" in a file configuration was always ignored
because the default value is false.

This fixes SeleniumHQ/docker-selenium#1145

Author: diemol

java/server/src/org/openqa/selenium/grid/TemplateGridCommand.java

@@ -75,11 +75,14 @@ public final Executable configure(PrintStream out, PrintStream err, String... ar
         return;
       }
 
+      // Order matters here. The configs precedence is: 1. Env vars, 2. System properties,
+      // 3. Configuration files (config.toml), 4. Cli flags and default values
+      // 5. Default role config
       Set<Config> allConfigs = new LinkedHashSet<>();
       allConfigs.add(new EnvConfig());
       allConfigs.add(new ConcatenatingConfig(getSystemPropertiesConfigPrefix(), '.', System.getProperties()));
-      allFlags.forEach(flags -> allConfigs.add(new AnnotatedConfig(flags)));
       allConfigs.add(configFlags.readConfigFiles());
+      allFlags.forEach(flags -> allConfigs.add(new AnnotatedConfig(flags)));
       allConfigs.add(getDefaultConfig());
 
       Config config = new MemoizedConfig(new CompoundConfig(allConfigs.toArray(new Config[0])));

java/server/src/org/openqa/selenium/grid/node/config/NodeOptions.java

@@ -51,6 +51,8 @@
 
 public class NodeOptions {
 
+  private static final String NODE_SECTION = "node";
+
   private static final Logger LOG = Logger.getLogger(NodeOptions.class.getName());
   private static final Json JSON = new Json();
   private static final String DEFAULT_IMPL = "org.openqa.selenium.grid.node.local.LocalNodeFactory";
@@ -62,7 +64,7 @@ public NodeOptions(Config config) {
   }
 
   public Optional<URI> getPublicGridUri() {
-    return config.get("node", "grid-url").map(url -> {
+    return config.get(NODE_SECTION, "grid-url").map(url -> {
       try {
         return new URI(url);
       } catch (URISyntaxException e) {
@@ -72,7 +74,7 @@ public Optional<URI> getPublicGridUri() {
   }
 
   public Node getNode() {
-    return config.getClass("node", "implementation", Node.class, DEFAULT_IMPL);
+    return config.getClass(NODE_SECTION, "implementation", Node.class, DEFAULT_IMPL);
   }
 
   public Map<Capabilities, Collection<SessionFactory>> getSessionFactories(
@@ -93,13 +95,13 @@ public Map<Capabilities, Collection<SessionFactory>> getSessionFactories(
 
   public int getMaxSessions() {
     return Math.min(
-      config.getInt("node", "max-concurrent-sessions")
+      config.getInt(NODE_SECTION, "max-concurrent-sessions")
         .orElse(Runtime.getRuntime().availableProcessors()),
       Runtime.getRuntime().availableProcessors());
   }
 
   private void addDriverFactoriesFromConfig(ImmutableMultimap.Builder<Capabilities, SessionFactory> sessionFactories) {
-    config.getAll("node", "driver-factories").ifPresent(allConfigs -> {
+    config.getAll(NODE_SECTION, "driver-factories").ifPresent(allConfigs -> {
       if (allConfigs.size() % 2 != 0) {
         throw new ConfigException("Expected each driver class to be mapped to a config");
       }
@@ -146,7 +148,7 @@ private SessionFactory createSessionFactory(String clazz, Capabilities stereotyp
   private void addDetectedDrivers(
     Map<WebDriverInfo, Collection<SessionFactory>> allDrivers,
     ImmutableMultimap.Builder<Capabilities, SessionFactory> sessionFactories) {
-    if (!config.getBool("node", "detect-drivers").orElse(false)) {
+    if (!config.getBool(NODE_SECTION, "detect-drivers").orElse(false)) {
       return;
     }
 
@@ -158,7 +160,7 @@ private void addDetectedDrivers(
   private void addSpecificDrivers(
     Map<WebDriverInfo, Collection<SessionFactory>> allDrivers,
     ImmutableMultimap.Builder<Capabilities, SessionFactory> sessionFactories) {
-    List<String> drivers = config.getAll("node", "drivers").orElse(new ArrayList<>()).stream()
+    List<String> drivers = config.getAll(NODE_SECTION, "drivers").orElse(new ArrayList<>()).stream()
       .map(String::toLowerCase)
       .collect(Collectors.toList());
 
@@ -173,7 +175,7 @@ private Map<WebDriverInfo, Collection<SessionFactory>> discoverDrivers(
     int maxSessions,
     Function<WebDriverInfo, Collection<SessionFactory>> factoryFactory) {
 
-    if (!config.getBool("node", "detect-drivers").orElse(false)) {
+    if (!config.getBool(NODE_SECTION, "detect-drivers").orElse(false)) {
       return ImmutableMap.of();
     }