docs: add configuration policy guidance#8429
Conversation
| | [other-tasks.md](other-tasks.md) | Dev environment setup, benchmarks, composite builds, native image tests, OTLP protobuf updates | | ||
| | File | Load when | | ||
| |------------------------------------------------|------------------------------------------------------------------------------------------------| | ||
| | [configuration-policy.md](configuration-policy.md) | Configuration changes, env vars/system properties, declarative config | |
There was a problem hiding this comment.
Looking at the comment posted by @jack-berg , this content could be a part of the api-design.md file.
There was a problem hiding this comment.
Yes I think api-design.md is appropriate. Config IS part of our API.
| Declarative config should remain a strict superset of environment variable and system property | ||
| configuration. | ||
|
|
||
| If a change adds a new environment variable or system property, it should also add equivalent |
There was a problem hiding this comment.
I would imagine this issue would require stronger language:
..., it **must** also add equivalent declarative config support.
| ## PR expectations for configuration changes | ||
|
|
||
| PRs that add configuration should include: | ||
|
|
There was a problem hiding this comment.
nit: start bullet points with Capital letters.
| PRs that add configuration should include: | ||
|
|
||
| - the motivation for introducing new configuration | ||
| - the environment variable and system property names, if any |
There was a problem hiding this comment.
nit: Is 'if any' required?
This policy would be in-effect only if the environment variables and system properties are added, correct?
|
|
||
| - the motivation for introducing new configuration | ||
| - the environment variable and system property names, if any | ||
| - the equivalent declarative config form, where applicable |
There was a problem hiding this comment.
Similarly, if declarative config is supposed to be a strict superset, IMO, the 'where applicable' should be dropped because it will always be applicable.
Let me know if you feel this is wrong.
| - the equivalent declarative config form, where applicable | ||
| - tests and documentation updates covering the new configuration path | ||
|
|
||
| ## Use judgment |
There was a problem hiding this comment.
IMO, this should be towards the top as it should be the first step when considering adding new config options.
|
|
||
| ## PR expectations for configuration changes | ||
|
|
||
| PRs that add configuration should include: |
There was a problem hiding this comment.
Suggestion:
PRs that introduce new configuration options should include:
ADITYA-CODE-SOURCE
force-pushed
the
issue-8300-config-policy-doc
branch
from
57a49f8 to
b5df7fb
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8429 +/- ##
=========================================
Coverage 90.96% 90.96%
Complexity 7809 7809
=========================================
Files 892 892
Lines 23702 23702
Branches 2361 2361
=========================================
Hits 21561 21561
Misses 1420 1420
Partials 721 721 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@psx95 @jack-berg Updated — moved guidance into |
3 participants
Fixes #8300.
Why
What
docs/knowledge/configuration-policy.mddocumenting the configuration policydocs/knowledge/README.mdso it is discoverable from the knowledge indexTesting
./gradlew --no-daemon --max-workers=1 -Dorg.gradle.jvmargs="-Xmx512m -XX:MaxMetaspaceSize=192m" spotlessCheck