-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Optionally abort startup when unknown configuration parameters were detected #5676
Optionally abort startup when unknown configuration parameters were detected #5676
Conversation
d7f4cad
to
c3da460
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #5676 +/- ##
=============================================
+ Coverage 67.77% 67.88% +0.11%
- Complexity 16474 16560 +86
=============================================
Files 1901 1908 +7
Lines 72121 72355 +234
Branches 7430 7443 +13
=============================================
+ Hits 48881 49121 +240
+ Misses 20727 20714 -13
- Partials 2513 2520 +7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see the benefit of exposing the config validation logic outside of OTP. It would be possible to work-around it by analyzing the log for validation errors, but it's messy to do in an automated fashion, and to fully emulate the behavior the OTP process would have to be stopped. So this adds value.
However, I don't see that OTP sets an exit code, somewhat limiting the use in automated logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main consern with this PR is that the we opens up for an alternative way of setting up OTP. The design is well though through to support a wide range of deployments, while this PR is not. Supporting alternatives always require more maintenance. The code changes here are relative small, but we are likely to see other similar requests in the future. I have taken some time to think through this, and I do see it is useful for some deployments, so I will approve it (when my comments are fixed). I have also considered other strategies, but have not found one which would work in all cases with this strict validation applied.
This deviates from the main model/design. But, it supports simpler deployments. I have considered spliting environment specific config (including ways if filtering the config - supported today) and OTP tuning. But, it is not so simple. E.g. you may want to turn on a feature in the test environment, but not in the production environment.
src/main/java/org/opentripplanner/standalone/config/ConfigModel.java
Outdated
Show resolved
Hide resolved
12d3d2d
to
e6ba88e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc is ok, just some minor stuff left.
6056d2b
to
5aeeffa
Compare
src/main/java/org/opentripplanner/standalone/config/framework/json/NodeAdapter.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opentripplanner/standalone/config/framework/json/NodeAdapterTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opentripplanner/standalone/config/framework/json/NodeAdapterTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opentripplanner/standalone/config/framework/json/NodeAdapterTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opentripplanner/standalone/config/ConfigModel.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Thomas Gran <[email protected]>
Co-authored-by: Thomas Gran <[email protected]>
src/test/java/org/opentripplanner/standalone/config/framework/json/NodeAdapterTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opentripplanner/standalone/config/framework/json/NodeAdapterTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Thomas Gran <[email protected]>
Summary
When managing a large number of deployments many configuration files need to be maintained and when upgrading to newer versions, it's easy to overlook that the configuration schema has changed.
There is already a validation system in place that prints warnings but it would be nice if you could make OTP fail loudly when the configuration is invalid. This way you'd notice straight away.
This PR adds this capability: you can pass the CLI param
--configCheck
and OTP fails to startup. This forces you to deal with the invalid configuration straight away.Unit tests
✔️
Documentation
✔️