Configuration standard #66
Conversation
WalkthroughAdds a new HyperFleet Configuration Standard document that specifies supported configuration sources (files, CLI, environment, remote), source precedence (CLI > Env > Config file > Defaults), file discovery and path conventions, environment variable and CLI flag naming conventions, config property syntax, validation/error handling, unknown-field behavior, multi-command and reload considerations, display/redaction guidance, examples, and implementation notes. Also 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 are unchanged. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
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.
rh-amarin
force-pushed
the
configuration
branch
2 times, most recently
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.
rh-amarin
force-pushed
the
configuration
branch
from
dc22f21 to
168d6dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@hyperfleet/standards/configuration.md`:
- Line 228: Fix the spelling mistake in the standards text: replace the
incorrect token "adaper" with "adapter" in the sentence "Modify the adaper
service code to adopt the standard" so the sentence reads "Modify the adapter
service code to adopt the standard"; search for the exact phrase "Modify the
adaper service code to adopt the standard" and update that token.
- Line 112: The flag specification for "--version, -v <string>" is incorrect
because version flags should be boolean; change the specification for the
"--version" / "-v" flag to be a no-argument/boolean flag (remove the "<string>"
type) so the flag simply triggers display of version info and exit; update any
parsing/usage references to treat "--version" / "-v" as a boolean flag rather
than expecting a string.
🧹 Nitpick comments (2)
hyperfleet/standards/configuration.md (2)
196-196: Use proper markdown link formatting.The bare URL should be formatted as a markdown link for better readability and to follow documentation best practices.
📝 Suggested formatting
-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 in the [viper-cobra-validation-poc repository](https://github.com/rh-amarin/viper-cobra-validation-poc). It showcases:
233-234: Clean up code span formatting.Remove trailing spaces inside code spans for cleaner markdown formatting.
📝 Proposed fix
-### Change Adapter to add a new `config` source to parameter loading -- Today the AdapterConfig can read params using ` source: env.VARIABLE` -- Change it so the Adapter config can use` source: config.VARIABLE` +### Change Adapter to add a new `config` source to parameter loading +- Today the AdapterConfig can read params using `source: env.VARIABLE` +- Change it so the Adapter config can use `source: config.VARIABLE`
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hyperfleet/standards/configuration.mdhyperfleet/standards/logging-specification.md
🚧 Files skipped from review as they are similar to previous changes (1)
- hyperfleet/standards/logging-specification.md
🧰 Additional context used
🪛 LanguageTool
hyperfleet/standards/configuration.md
[grammar] ~40-~40: 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] ~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 ## Action items and next ...
(CAN_NOT_PREMIUM)
[grammar] ~228-~228: Ensure spelling is correct
Context: ...new configuration standard - Modify the adaper service code to adopt the standard - Mo...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~238-~238: ‘take into account’ might be wordy. Consider a shorter alternative.
Context: ...he claude plugin - Modify the plugin to take into account the new configuration standard
(EN_WORDINESS_PREMIUM_TAKE_INTO_ACCOUNT)
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/standards/configuration.md
42-42: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 3
(MD005, list-indent)
42-42: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
43-43: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 3
(MD005, list-indent)
43-43: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 4; Actual: 5
(MD007, ul-indent)
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)
233-233: Spaces inside code span elements
(MD038, no-space-in-code)
234-234: Spaces inside code span elements
(MD038, no-space-in-code)
🔇 Additional comments (1)
hyperfleet/standards/configuration.md (1)
137-170: Strong validation guidance.This section provides excellent guidance on configuration validation with clear error handling requirements and helpful examples. The emphasis on strict validation at startup and unknown field detection will prevent common configuration errors.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| ``` | ||
| --config <path> # Config file path | ||
| --name, -n <string> # component name (REQUIRED) | ||
| --version, -v <string> # Display version information |
There was a problem hiding this comment.
Fix the --version flag specification.
The --version flag should not accept a string argument. Version flags are typically boolean flags that display version information and exit when present.
📝 Proposed fix
---version, -v <string> # Display version information
+--version, -v # Display version information📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --version, -v <string> # Display version information | |
| --version, -v # Display version information |
🤖 Prompt for AI Agents
In `@hyperfleet/standards/configuration.md` at line 112, The flag specification
for "--version, -v <string>" is incorrect because version flags should be
boolean; change the specification for the "--version" / "-v" flag to be a
no-argument/boolean flag (remove the "<string>" type) so the flag simply
triggers display of version info and exit; update any parsing/usage references
to treat "--version" / "-v" as a boolean flag rather than expecting a string.
| - Modify helm files to simplify the use of environment variables if config is cover by other methods | ||
|
|
||
| ### Change Adapter to use new configuration standard | ||
| - Modify the adaper service code to adopt the standard |
There was a problem hiding this comment.
Fix spelling error.
"adaper" should be "adapter".
📝 Proposed fix
-### Change Adapter to use new configuration standard
-- Modify the adaper service code to adopt the standard
+### Change Adapter to use new configuration standard
+- Modify the adapter service code to adopt the standard🧰 Tools
🪛 LanguageTool
[grammar] ~228-~228: Ensure spelling is correct
Context: ...new configuration standard - Modify the adaper service code to adopt the standard - Mo...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In `@hyperfleet/standards/configuration.md` at line 228, Fix the spelling mistake
in the standards text: replace the incorrect token "adaper" with "adapter" in
the sentence "Modify the adaper service code to adopt the standard" so the
sentence reads "Modify the adapter service code to adopt the standard"; search
for the exact phrase "Modify the adaper service code to adopt the standard" and
update that token.
|
|
||
| Rules: | ||
| - All letters must be uppercase | ||
| - All environment variables for HyperFleet applications should be prefixed with `HYPERFLEET_`. |
There was a problem hiding this comment.
I don't think we need HYPERFLEET prefix for generic env variables like LOG_LEVEL. Think about that for service offering team deployment, they want to adjust log level for the component deployments they have to learn all ENV variables of log level.
There was a problem hiding this comment.
What does it mean "generic"?
IMO we want the values as specific as possible for our system
AFAIK the name LOG_LEVEL is used in some projects and libraries, but not even a standard anywhere.
E.g. Otel uses OTEL_LOG_LEVEL to avoid conflicting names
I agree that having a prefix as HYPERFLEET_ is both ugly and cumbersome, I was fighting myself when writing it in the proposal, but it allow us to:
- Clearly identify what variables are meant for our sytem
- Identify variables that are wrong
- e.g. the config system could read all HYPERFLEET_* variables and warn the user if there are non valid ones, similar to what the cli-parameters do when using a wrong one
There was a problem hiding this comment.
I agree with HYPERFLEET_ to our system specified env variables. But for some well known ones like LOG_LEVEL, I would suggest to keep it as it is.
There was a problem hiding this comment.
During the discussion with @xueli181114 yesterday, I was convinced and inclined to believe that generic options like LOG_LEVEL could be left unprefixed to reduce the number of environment variables required for configuration.
However, after reconsidering today, I'm starting to question whether we really need to do this. Log levels are essentially the service's own runtime behavior, not platform-wide global attributes. Allowing each independent project to control its own log level is semantically clearer, more flexible in production environments, and better suited to actual operations and troubleshooting logic.
In contrast, while unified environment variables seem to simplify configuration, in real-world systems they often just shift complexity: to avoid naming conflicts and inconsistent behavior, we still ultimately need to introduce additional conventions or mechanisms. In this case, the benefits of unification are actually limited.
| | Log Level | `--log-level` | `LOG_LEVEL` | `info` | Minimum level: `debug`, `info`, `warn`, `error` | | ||
| | Log Format | `--log-format` | `LOG_FORMAT` | `text` | Output format: `text` or `json` | | ||
| | Log Output | `--log-output` | `LOG_OUTPUT` | `stdout` | Destination: `stdout` or `stderr` | | ||
| | Log Level | `--log-level` | `HYPERFLEET_LOG_LEVEL` | `info` | Minimum level: `debug`, `info`, `warn`, `error` | |
There was a problem hiding this comment.
LOG_LEVEL is generic env variable, it can be used as a common env variable between components.
4 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.