Configuration standard #66
Conversation
WalkthroughAdds a new HyperFleet Configuration Standard document describing supported configuration sources (files, CLI, environment, remote), source precedence (CLI > Env > Config file > Defaults), file path resolution, environment variable and CLI flag naming conventions, configuration property syntax, validation and error handling, unknown-field behavior, multi-command and reload considerations, runtime display/loading guidance, examples, and implementation notes. Updates the logging specification to rename environment variables from LOG_LEVEL/LOG_FORMAT/LOG_OUTPUT to HYPERFLEET_LOG_LEVEL/HYPERFLEET_LOG_FORMAT/HYPERFLEET_LOG_OUTPUT; flags and defaults remain unchanged. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Pre-merge checks✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @hyperfleet/standards/configuration.md:
- Line 39: Replace the misspelled and inconsistent phrases in the document:
change "stablish" to "establish", "single word paths" to "single-word paths",
"non functional" to "non-functional", "3rd party" to "third-party", and "can
not" to "cannot" so the text follows correct English spelling and hyphenation
conventions.
🧹 Nitpick comments (1)
hyperfleet/standards/configuration.md (1)
44-46: Address markdown formatting issues flagged by linting.Several markdown formatting problems need correction:
- Lines 44–46: List indentation inconsistency (expected 2, got 3–4 spaces)
- Line 47: Code block missing language specifier (should be
```yaml)- Lines 64–65: Nested list indentation (expected 0, got 2 spaces)
- Lines 101–103, 109–120, 116–129, 123–129, 132–135, 154–163: Code blocks missing language specifiers
- Line 196: Bare URL should be formatted as a proper markdown link
🔎 Proposed fixes
- Properties are case insensitive - - Two properties `propertyA` and `propertya` should mean the same - - Properties should form a hierarchy of single-word paths - - E.g to represent the property `app.name` - - As a command-line parameter it will be `--app-name` - - As an environment variable it will be `HYPERFLEET_APP_NAME` - - In YAML files we it can be a nested properties -``` + Properties are case insensitive + - Two properties `propertyA` and `propertya` should mean the same + - Properties should form a hierarchy of single-word paths + - E.g to represent the property `app.name` + - As a command-line parameter it will be `--app-name` + - As an environment variable it will be `HYPERFLEET_APP_NAME` + - In YAML files we it can be a nested properties + ```yaml - ```bash + ```bash - ``` + ```bash - ``` + ``` - ``` + ```bash - ``` + ```bash - Example error output: -``` +Example error output: +```bash - An example to implement the configuration with support of cobra, viper and the [validation library](https://github.com/go-playground/validator) can be found at https://github.com/rh-amarin/viper-cobra-validation-poc. It showcases: + An example to implement the configuration with support of cobra, viper and the [validation library](https://github.com/go-playground/validator) can be found at [viper-cobra-validation-poc](https://github.com/rh-amarin/viper-cobra-validation-poc). It showcases:Also applies to: 47-50, 64-65, 101-103, 109-120, 116-129, 123-129, 132-135, 154-163, 196-196
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hyperfleet/standards/configuration.md
🧰 Additional context used
🪛 LanguageTool
hyperfleet/standards/configuration.md
[grammar] ~39-~39: Ensure spelling is correct
Context: ... has different syntax rules, we need to stablish a convention for config properties: - P...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~42-~42: Use a hyphen to join words.
Context: ...erties should form a hierarchy of single word paths - E.g to represent the prop...
(QB_NEW_EN_HYPHEN)
[grammar] ~70-~70: Use a hyphen to join words.
Context: ...application configuration, typically non functional parameters (technical configu...
(QB_NEW_EN_HYPHEN)
[grammar] ~81-~81: Use a hyphen to join words.
Context: ...vironment variables that are used by 3rd party libraries directly (e.g. OpenTelem...
(QB_NEW_EN_HYPHEN)
[style] ~174-~174: Consider using a different adverb to strengthen your wording.
Context: ...for commands that only need a subset or completely different set of configuration options....
(COMPLETELY_ENTIRELY)
[style] ~210-~210: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...`, indicating that the value is set but can not be consulted
(CAN_NOT_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/standards/configuration.md
44-44: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 3
(MD005, list-indent)
44-44: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
45-45: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 3
(MD005, list-indent)
45-45: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
46-46: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 4
(MD005, list-indent)
46-46: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
64-64: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
65-65: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
101-101: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
109-109: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
116-116: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
123-123: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
132-132: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
154-154: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
196-196: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (1)
hyperfleet/standards/configuration.md (1)
3-35: Excellent coverage of configuration standard requirements.The document comprehensively defines all stated PR objectives: override precedence (CLI > Env > Config > Defaults), naming conventions for environment variables and CLI flags, standard file paths with production/development defaults, validation strategy with clear error guidance, and the no-runtime-reloading policy. The structure is logical, examples are concrete and helpful, and the implementation reference provides a practical starting point for teams.
|
In order to see the effects of this in a real implementation I created this PR openshift-hyperfleet/hyperfleet-api#27 warning: for the API it is a massive change, since environment configuration inherits from TRex, and that PR changes that completely I expect the changes for sentinel/adapter to be lighter What is interesting from the example is:
|
| - Field 'Config.App.Name' failed validation: required | ||
| Please provide application name via: | ||
| • Flag: --app-name or -n | ||
| • Environment variable: HYPERFLEET_MYAPP_APP_NAME |
There was a problem hiding this comment.
| • Environment variable: HYPERFLEET_MYAPP_APP_NAME | |
| • Environment variable: HYPERFLEET_APP_NAME |
| - Logging the configuration at start time | ||
| - Offering a method to query it, e.g. through a `/config` endpoint that displays the values in JSON format | ||
|
|
||
| When displaying the configuration values, any sensitive data like credentials should be redacted and displayed as `*`, indicating that the value is set but can not be consulted |
There was a problem hiding this comment.
Minor suggestion: It might be helpful to clarify the difference between a redacted value (set but hidden) vs an unset value:
- "password": "***" → value is set but redacted
- "password": null or omitted → value is not set
This helps operators distinguish between "configured but hidden" and "not configured".
There was a problem hiding this comment.
Since config settings should have a default value, I think it makes difficult to track when the value has been set by cli-params, environments or config file, or if it is falling back to the default value.... at least that behavior is not provided by viper
We could also show only a couple of characters of the credential, that way, for devs/operators gives some clue about what value is really there.
password: "fo****"
wdyt?
There was a problem hiding this comment.
Do we need to distinguish this? Whether the password is configured or not, it at least has a default value or reports error message. Is not redacting it "**REDACTED**" enough?
There was a problem hiding this comment.
Do we need it? not strongly required, and I guess from the security point of view my recommendation is an abomination 😄
I was thinking on the numerous times I went crazy not finding an error, just because the password that was effectively in use was not the correct one... this would help to solve it
But, I guess we should prioritize security, so I will remove this
|
Is this related to https://issues.redhat.com/browse/HYPERFLEET-380 ? |
rh-amarin
force-pushed
the
configuration
branch
from
9ade0d3 to
5a589ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
hyperfleet/standards/configuration.md (1)
39-39: Grammar, spelling, and hyphenation errors still present.The following corrections flagged in the previous review remain unaddressed:
🔎 Proposed fixes
- Since each data source has different syntax rules, we need to stablish a convention for config properties: + Since each data source has different syntax rules, we need to establish a convention for config properties: - Properties should form a hierarchy of single word paths + Properties should form a hierarchy of single-word paths - General application configuration, typically non functional parameters (technical configuration) + General application configuration, typically non-functional parameters (technical configuration) - The exception would be for those environment variables that are used by 3rd party libraries directly (e.g. OpenTelemetry lib) + The exception would be for those environment variables that are used by third-party libraries directly (e.g. OpenTelemetry lib) - When displaying the configuration values, any sensitive data like credentials should be redacted and displayed as `*`, indicating that the value is set but can not be consulted + When displaying the configuration values, any sensitive data like credentials should be redacted and displayed as `*`, indicating that the value is set but cannot be consultedAlso applies to: 42-42, 70-70, 81-81, 210-210
🧹 Nitpick comments (4)
hyperfleet/standards/configuration.md (4)
44-50: Add language specifiers to all code blocks.Code blocks throughout the document lack language specifiers, making them harder to syntax-highlight. The code block at line 47 should specify
yaml, and other blocks should specifybashor appropriate language.🔎 Proposed fixes
- Properties should form a hierarchy of single-word paths - E.g to represent the property `app.name` - As a command-line parameter it will be `--app-name` - As an environment variable it will be `HYPERFLEET_APP_NAME` - In YAML files it can be a nested property -``` +```yaml app: name: myapp -``` +```Additionally, update other code blocks:
- Line 86-91:
bash(environment variable examples)- Line 101-103:
text(flag format template)- Line 109-113:
bash(global flags)- Line 116-120:
bash(server flags)- Line 123-129:
bash(database flags)- Line 132-135:
bash(logging flags)- Line 154-163:
textorbash(error output example)Also applies to: 101-103, 109-113, 116-120, 123-129, 132-135, 154-163
44-46: Fix list indentation inconsistencies.Lines 44–46 and 64–65 have inconsistent indentation that violates markdown list formatting (MD005, MD007). List items at the same level should use consistent indentation.
🔎 Proposed fixes
Lines 44-46 (should use 2-space indentation):
- Properties should form a hierarchy of single-word paths - E.g to represent the property `app.name` - As a command-line parameter it will be `--app-name` - As an environment variable it will be `HYPERFLEET_APP_NAME` - In YAML files it can be a nested property + Properties should form a hierarchy of single-word paths + - E.g to represent the property `app.name` + - As a command-line parameter it will be `--app-name` + - As an environment variable it will be `HYPERFLEET_APP_NAME` + - In YAML files it can be a nested propertyLines 64-65 (default values should not be indented):
3. Default values - - production: `/etc/hyperfleet/config.yaml` - - development: `./configs/config.yaml` + - production: `/etc/hyperfleet/config.yaml` + - development: `./configs/config.yaml`Also applies to: 64-65
174-174: Consider stronger wording for configuration flexibility requirement.Line 174 uses "completely different" which reads softly. Consider "entirely different" or "distinct" to strengthen the statement about configuration differences across commands.
196-196: Wrap bare URL in markdown link syntax.Line 196 contains a bare URL that should be wrapped in markdown link syntax for proper rendering.
🔎 Proposed fix
- An example to implement the configuration with support of cobra, viper and the [validation library](https://github.com/go-playground/validator) can be found at https://github.com/rh-amarin/viper-cobra-validation-poc. It showcases: + An example to implement the configuration with support of cobra, viper and the [validation library](https://github.com/go-playground/validator) can be found at [viper-cobra-validation-poc](https://github.com/rh-amarin/viper-cobra-validation-poc). It showcases:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hyperfleet/standards/configuration.md
🧰 Additional context used
🪛 LanguageTool
hyperfleet/standards/configuration.md
[grammar] ~39-~39: Ensure spelling is correct
Context: ... has different syntax rules, we need to stablish a convention for config properties: - P...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~42-~42: Use a hyphen to join words.
Context: ...erties should form a hierarchy of single word paths - E.g to represent the prop...
(QB_NEW_EN_HYPHEN)
[grammar] ~70-~70: Use a hyphen to join words.
Context: ...application configuration, typically non functional parameters (technical configu...
(QB_NEW_EN_HYPHEN)
[grammar] ~81-~81: Use a hyphen to join words.
Context: ...vironment variables that are used by 3rd party libraries directly (e.g. OpenTelem...
(QB_NEW_EN_HYPHEN)
[style] ~174-~174: Consider using a different adverb to strengthen your wording.
Context: ...for commands that only need a subset or completely different set of configuration options....
(COMPLETELY_ENTIRELY)
[style] ~210-~210: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...`, indicating that the value is set but can not be consulted
(CAN_NOT_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/standards/configuration.md
44-44: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 3
(MD005, list-indent)
44-44: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
45-45: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 3
(MD005, list-indent)
45-45: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
46-46: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 4
(MD005, list-indent)
46-46: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
64-64: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
65-65: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
101-101: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
109-109: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
116-116: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
123-123: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
132-132: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
154-154: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
196-196: Bare URL used
(MD034, no-bare-urls)
Yes they are very related. But, in my examples I addressed the validation part using https://github.com/go-playground/validator |
rh-amarin
force-pushed
the
configuration
branch
from
5a589ad to
3284533
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @hyperfleet/standards/configuration.md:
- Line 39: The document contains several spelling/grammar errors; update the
specified phrases: change "stablish" to "establish" (occurrence near the
sentence that begins "Since each data source..."), change "single word paths" to
"single-word paths", change "non functional" to "non-functional", change "3rd
party" to "third-party", and change "can not" to the single word "cannot";
locate these exact phrases in the file (they appear around the original lines
39, 42, 70, 81, and 210 respectively) and replace them preserving surrounding
punctuation and capitalization.
🧹 Nitpick comments (3)
hyperfleet/standards/configuration.md (3)
44-46: Correct list indentation for consistency.The bullet points under the properties example (lines 44-46) have inconsistent indentation. Markdown linting expects consistent 2-space indentation for nested list items.
🔎 Proposed fix
- Properties should form a hierarchy of single-word paths - E.g to represent the property `app.name` - - As a command-line parameter it will be `--app-name` - - As an environment variable it will be `HYPERFLEET_APP_NAME` - - In YAML files it can be a nested property + - As a command-line parameter it will be `--app-name` + - As an environment variable it will be `HYPERFLEET_APP_NAME` + - In YAML files it can be a nested property
47-47: Specify language identifiers for all fenced code blocks.Multiple code blocks lack language specifiers, which improves syntax highlighting and clarity in rendered documentation:
- Line 47: YAML block needs
```yaml- Line 101: Format block needs
```bash(or similar)- Line 109: Flags block needs
```bash- Line 116, 123, 132: Flag blocks need
```bash- Line 154: Error output block needs
```txtor```bash🔎 Example fix for line 47
-``` +```yaml app: name: myapp -``` +```Also applies to: 101-101, 109-109, 116-116, 123-123, 132-132, 154-154
196-196: Wrap bare URL in markdown link syntax.Line 196 contains a bare URL that should be formatted as a proper markdown link for better rendering and accessibility.
🔎 Proposed fix
- An example to implement the configuration with support of cobra, viper and the [validation library](https://github.com/go-playground/validator) can be found at https://github.com/rh-amarin/viper-cobra-validation-poc. It showcases: + An example to implement the configuration with support of cobra, viper and the [validation library](https://github.com/go-playground/validator) can be found at [viper-cobra-validation-poc](https://github.com/rh-amarin/viper-cobra-validation-poc). It showcases:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hyperfleet/standards/configuration.mdhyperfleet/standards/logging-specification.md
🧰 Additional context used
🪛 LanguageTool
hyperfleet/standards/configuration.md
[grammar] ~39-~39: Ensure spelling is correct
Context: ... has different syntax rules, we need to stablish a convention for config properties: - P...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~42-~42: Use a hyphen to join words.
Context: ...erties should form a hierarchy of single word paths - E.g to represent the prop...
(QB_NEW_EN_HYPHEN)
[grammar] ~70-~70: Use a hyphen to join words.
Context: ...application configuration, typically non functional parameters (technical configu...
(QB_NEW_EN_HYPHEN)
[grammar] ~81-~81: Use a hyphen to join words.
Context: ...vironment variables that are used by 3rd party libraries directly (e.g. OpenTelem...
(QB_NEW_EN_HYPHEN)
[style] ~174-~174: Consider using a different adverb to strengthen your wording.
Context: ...for commands that only need a subset or completely different set of configuration options....
(COMPLETELY_ENTIRELY)
[style] ~210-~210: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...`, indicating that the value is set but can not be consulted
(CAN_NOT_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/standards/configuration.md
44-44: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 3
(MD005, list-indent)
44-44: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
45-45: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 3
(MD005, list-indent)
45-45: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
46-46: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 4
(MD005, list-indent)
46-46: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
64-64: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
65-65: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
101-101: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
109-109: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
116-116: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
123-123: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
132-132: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
154-154: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
196-196: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (1)
hyperfleet/standards/logging-specification.md (1)
55-59: ✓ Environment variable naming aligned with Configuration Standard.The update to use the
HYPERFLEET_prefix for logging configuration variables (HYPERFLEET_LOG_LEVEL,HYPERFLEET_LOG_FORMAT,HYPERFLEET_LOG_OUTPUT) correctly implements the standard defined in the new Configuration Standard document and resolves the earlier concern about prefix consistency.
|
|
||
| ## Config properties syntax | ||
|
|
||
| Since each data source has different syntax rules, we need to stablish a convention for config properties: |
There was a problem hiding this comment.
Fix lingering spelling and grammar errors.
Several language issues remain unresolved from the prior review:
- Line 39: "stablish" → "establish"
- Line 42: "single word paths" → "single-word paths"
- Line 70: "non functional" → "non-functional"
- Line 81: "3rd party" → "third-party"
- Line 210: "can not" → "cannot"
🔎 Proposed fixes
- Since each data source has different syntax rules, we need to stablish a convention for config properties:
+ Since each data source has different syntax rules, we need to establish a convention for config properties:
- Properties should form a hierarchy of single word paths
+ Properties should form a hierarchy of single-word paths
- General application configuration, typically non functional parameters (technical configuration)
+ General application configuration, typically non-functional parameters (technical configuration)
- The exception would be for those environment variables that are used by 3rd party libraries directly (e.g. OpenTelemetry lib)
+ The exception would be for those environment variables that are used by third-party libraries directly (e.g. OpenTelemetry lib)
- When displaying the configuration values, any sensitive data like credentials should be redacted and displayed as `*`, indicating that the value is set but can not be consulted
+ When displaying the configuration values, any sensitive data like credentials should be redacted and displayed as `*`, indicating that the value is set but cannot be consultedAlso applies to: 42-42, 70-70, 81-81, 210-210
🧰 Tools
🪛 LanguageTool
[grammar] ~39-~39: Ensure spelling is correct
Context: ... has different syntax rules, we need to stablish a convention for config properties: - P...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In @hyperfleet/standards/configuration.md around line 39, The document contains
several spelling/grammar errors; update the specified phrases: change "stablish"
to "establish" (occurrence near the sentence that begins "Since each data
source..."), change "single word paths" to "single-word paths", change "non
functional" to "non-functional", change "3rd party" to "third-party", and change
"can not" to the single word "cannot"; locate these exact phrases in the file
(they appear around the original lines 39, 42, 70, 81, and 210 respectively) and
replace them preserving surrounding punctuation and capitalization.
3284533 to
56cf50d
Compare
rh-amarin
force-pushed
the
configuration
branch
from
56cf50d to
dc22f21
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @hyperfleet/standards/configuration.md:
- Line 42: Update the phrasing in the configuration document to fix grammar and
style: change "single word paths" to "single-word paths", "non functional" to
"non-functional", "3rd party" to "third-party", and "can not" to "cannot"
wherever those phrases appear (the lines flagged earlier). Ensure each
replacement preserves surrounding context and punctuation.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
hyperfleet/standards/logging-specification.md (2)
63-63: Update environment variable reference to match the naming convention.Line 63 references the old naming convention. Update to use the new HYPERFLEET_ prefix consistent with the table above.
🔎 Proposed fix
-For production, use `LOG_FORMAT=json` for better log aggregation. +For production, use `HYPERFLEET_LOG_FORMAT=json` for better log aggregation.
78-78: Update environment variable reference to match the naming convention.Line 78 references the old naming convention. Update to use the new HYPERFLEET_ prefix consistent with the table above and the configuration standard.
🔎 Proposed fix
-When `LOG_LEVEL` is set, only messages at that level or higher are output. +When `HYPERFLEET_LOG_LEVEL` is set, only messages at that level or higher are output.
🧹 Nitpick comments (2)
hyperfleet/standards/configuration.md (2)
167-169: Clarify viper.unmarshalExact() usage and error handling.The recommendation to use
viper.unmarshalExact()is helpful, but consider adding a note that this function enforces strict unmarshaling. Applications should wrap the unmarshaling call in proper error handling to provide users with clear feedback about which unknown fields were found, rather than surfacing the raw viper error.
174-174: Consider clarifying the phrasing about command-specific configuration.The phrase "only need a subset or completely different set" could be strengthened. Consider: "...commands that require only a subset, or a completely different set, of configuration options" for better readability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hyperfleet/standards/configuration.mdhyperfleet/standards/logging-specification.md
🧰 Additional context used
🪛 LanguageTool
hyperfleet/standards/configuration.md
[grammar] ~42-~42: Use a hyphen to join words.
Context: ...erties should form a hierarchy of single word paths - E.g to represent the prop...
(QB_NEW_EN_HYPHEN)
[grammar] ~70-~70: Use a hyphen to join words.
Context: ...application configuration, typically non functional parameters (technical configu...
(QB_NEW_EN_HYPHEN)
[grammar] ~81-~81: Use a hyphen to join words.
Context: ...vironment variables that are used by 3rd party libraries directly (e.g. OpenTelem...
(QB_NEW_EN_HYPHEN)
[style] ~174-~174: Consider using a different adverb to strengthen your wording.
Context: ...for commands that only need a subset or completely different set of configuration options....
(COMPLETELY_ENTIRELY)
[style] ~210-~210: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...`, indicating that the value is set but can not be consulted
(CAN_NOT_PREMIUM)
🔇 Additional comments (1)
hyperfleet/standards/configuration.md (1)
1-30: Well-structured configuration standard aligned with best practices.The document provides a comprehensive and clear standard for configuration management across HyperFleet applications. The precedence order (CLI > Env > Config > Defaults) aligns with how Cobra/Viper handle configuration, and the naming conventions (kebab-case for flags, UPPERCASE with underscores for env vars) follow established Go CLI patterns. The guidance on environment variable prefixing, validation error messages, and unknown field handling demonstrates thoughtful API design.
| Since each data source has different syntax rules, we need to establish a convention for config properties: | ||
| - Properties are case insensitive | ||
| - Two properties `propertyA` and `propertya` should mean the same | ||
| - Properties should form a hierarchy of single word paths |
There was a problem hiding this comment.
Fix unresolved grammar and style issues.
Several grammar and style issues from the previous review remain unaddressed:
- Line 42: "single word paths" should use a hyphen: "single-word paths"
- Line 70: "non functional" should use a hyphen: "non-functional"
- Line 81: "3rd party" should be spelled out: "third-party"
- Line 210: "can not" should be a single word: "cannot"
🔎 Proposed fixes
- Properties should form a hierarchy of single word paths
+ Properties should form a hierarchy of single-word paths- General application configuration, typically non functional parameters (technical configuration)
+ General application configuration, typically non-functional parameters (technical configuration)- The exception would be for those environment variables that are used by 3rd party libraries directly (e.g. OpenTelemetry lib)
+ The exception would be for those environment variables that are used by third-party libraries directly (e.g. OpenTelemetry lib)- When displaying the configuration values, any sensitive data like credentials should be redacted and displayed as `*`, indicating that the value is set but can not be consulted
+ When displaying the configuration values, any sensitive data like credentials should be redacted and displayed as `*`, indicating that the value is set but cannot be consultedAlso applies to: 70-70, 81-81, 210-210
🧰 Tools
🪛 LanguageTool
[grammar] ~42-~42: Use a hyphen to join words.
Context: ...erties should form a hierarchy of single word paths - E.g to represent the prop...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In @hyperfleet/standards/configuration.md around line 42, Update the phrasing in
the configuration document to fix grammar and style: change "single word paths"
to "single-word paths", "non functional" to "non-functional", "3rd party" to
"third-party", and "can not" to "cannot" wherever those phrases appear (the
lines flagged earlier). Ensure each replacement preserves surrounding context
and punctuation.
| - Logging the configuration at start time | ||
| - Offering a method to query it, e.g. through a `/config` endpoint that displays the values in JSON format | ||
|
|
||
| When displaying the configuration values, any sensitive data like credentials should be redacted and displayed as `*`, indicating that the value is set but can not be consulted |
There was a problem hiding this comment.
Do we need to distinguish this? Whether the password is configured or not, it at least has a default value or reports error message. Is not redacting it "**REDACTED**" enough?
| ## Standard Configuration File Paths | ||
| Config files for HyperFleet applications must be in YAML format |
There was a problem hiding this comment.
In line 12, it says Single or multiple formats (YAML, JSON), here it says must be in YAML format. From my understanding, this should be a contradiction. Maybe I misunderstand something here.
| ``` | ||
| --config <path> # Config file path | ||
| --name, -n <string> # component name (REQUIRED) | ||
| --version, -v <string> # component version |
There was a problem hiding this comment.
Better to say Display version information
|
|
||
| Since each data source has different syntax rules, we need to establish a convention for config properties: | ||
| - Properties are case insensitive | ||
| - Two properties `propertyA` and `propertya` should mean the same |
There was a problem hiding this comment.
Since it's a standard, why not mandate either camelCase or all lowercase?
3 participants
https://issues.redhat.com/browse/HYPERFLEET-379
Summary
Add Configuration Standard document defining how all HyperFleet applications should handle configuration loading, merging, and validation.
Key Standards Established
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.