Repository navigation
Add separate YAML runtime configuration (#40) - #555
aarrasseayoub01 wants to merge 2 commits into
Conversation
* Add separate YAML runtime configuration * Remove unrelated HTTPS validation changes * Keep config file substitution in ServerConfig * Nest runtime YAML settings * Use canonical runtime configuration names * Normalize legacy runtime properties to canonical names * Organize runtime authorization and CORS settings * Use TCPS in runtime configuration example
oracle-contributor-agreement
Bot
added
the
OCA Verified
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
Review by @dustin-sale via Codex.
Requested changes
- [P1] Abort startup when an explicitly selected runtime configuration cannot be loaded.
Additional review notes
None.
Validation
Native mvn clean package passed: 26 tests passed and one database-backed integration test was skipped. A targeted loader probe reproduced configuration loss after an unresolved environment variable. Java coverage is not enforced by the current POM.
See the inline comment in this review for evidence, impact, and suggested remediation.
| } catch (Exception e) { | ||
| LOG.log(Level.SEVERE, "Unable to load runtime config file: " + runtimeConfigFilePath, e); | ||
| return null; |
There was a problem hiding this comment.
Comment from @dustin-sale via Codex.
[P1] Abort startup when an explicitly selected runtime file fails
Evidence: This catch returns null for unreadable files, malformed YAML, and environment-substitution failures. The startup initializer then calls LoadedConstants.initialize(null), discarding all runtime YAML settings. A loader probe with authentication, DeepSec, HTTP transport, and a list-tools selection confirmed that adding one unresolved environment placeholder resets those values to false, false, stdio, and null respectively.
Impact: A deployment can continue under different settings after a configuration error. If database credentials remain available from JVM properties or the datasource file, losing toolSelection.enabled broadens the tool selection, and losing DeepSec removes the configured end-user database context. HTTP has a separate authentication startup guard, but that does not preserve the other discarded settings.
Requested change: Return null only when no runtime file was selected. Propagate loading/parsing/substitution errors for an explicitly selected file so startup aborts, and add regression tests for those failure cases.
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
Review by @dustin-sale via Codex.
Requested changes
No blocking changes requested. The previous P1 about runtime-file loading failures is resolved, with regression tests for all three reported failure paths.
Additional review notes
- [P2] Align the documented HTTP port settings with the HTTPS port setting the listener consumes.
Validation
mvn clean package passed: 29 tests passed and one database-backed integration test was skipped. A startup-configuration probe verified YAML initialization and reproduced the unused HTTP port setting. Java coverage remains unenforced.
See the inline comment in this review for evidence, impact, and suggested remediation.
| places: for example, `database.url` and `network.http.port`. Existing property names such as | ||
| `db.url` and `http.port` remain supported as compatibility aliases. When both forms are supplied, |
There was a problem hiding this comment.
Comment from @dustin-sale via Codex.
[P2] Document the port setting the listener actually consumes
Evidence: These lines advertise network.http.port and its http.port alias, but neither controls a listener. RuntimePropertyNames maps the alias, while LoadedConstants reads only network.https.port and startHttpServer uses HTTPS_PORT. A startup-configuration probe with network.http.port: 8080 left HTTPS_PORT null.
Impact: Operators can configure the advertised port without changing the listening port. The example uses its separate HTTPS port, and supplying only network.http.port leaves the required listener port unset.
Requested change: Use network.https.port and https.port in this explanation, remove the unused HTTP port from the README and runtime YAML examples, and remove the unused alias mapping unless support is intentionally implemented.
Add separate YAML runtime configuration
Remove unrelated HTTPS validation changes
Keep config file substitution in ServerConfig
Nest runtime YAML settings
Use canonical runtime configuration names
Normalize legacy runtime properties to canonical names
Organize runtime authorization and CORS settings
Use TCPS in runtime configuration example