HYPERFLEET-549 - feat: standard configuration#71
HYPERFLEET-549 - feat: standard configuration#71rh-amarin wants to merge 4 commits intoopenshift-hyperfleet:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR restructures configuration into a nested model under clients, adds sentinel metadata, logging and debug sections, and renames many keys from camelCase to snake_case. HyperFleet API settings move to clients.hyperfleet_api, broker topic nests under clients.broker.topic, and environment variables gain a HYPERFLEET_ prefix. LoadConfig now merges CLI flags, env vars, and files with explicit precedence and validation; a new config-dump CLI command prints the merged, redacted YAML. The HyperFleet client constructor now accepts sentinel name and version to set a User-Agent. Helm charts, examples, tests, and docs updated accordingly. Sequence Diagram(s)sequenceDiagram
participant User
participant CLI_Flags as "CLI Flags"
participant Env as "Environment Vars"
participant Config_File as "Config File"
participant Loader as "config.LoadConfig"
participant Logger as "initLogging"
participant App as "Sentinel (serve / config-dump)"
participant Hyperfleet as "HyperFleet Client"
User->>CLI_Flags: provide flags (optional)
User->>Env: set env vars (optional)
User->>Config_File: provide config file (optional)
CLI_Flags->>Loader: flag values
Env->>Loader: env values
Config_File->>Loader: file values
Loader->>Loader: merge (flags > env > file > defaults)
Loader->>Loader: validate & produce redacted copy
Loader->>Logger: pass Log config
Logger->>App: initialize logging
Loader->>Hyperfleet: supply base_url, timeout, sentinel name, version
App->>Hyperfleet: initialize client (User-Agent set)
opt config-dump
User->>App: run config-dump
App->>Loader: load merged config
App->>User: print redacted YAML
end
opt serve
User->>App: run serve
App->>App: start watchers & broker using merged config
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
3 times, most recently
from
96cde3b to
37a82bc
Compare
|
@coderabbitai review |
|
@coderabbitai help |
|
@coderabbitai review |
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
2 times, most recently
from
d30dabd to
a4e1d5f
Compare
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
from
a4e1d5f to
361a1cd
Compare
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
from
d08733f to
6d23adb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/integration/test-config-loading.sh (1)
128-131:BASE_BROKERis defined but never used.Static analysis correctly identifies that
BASE_BROKERis declared but not referenced anywhere in the test sections. Either remove it or add tests that use it.-BASE_BROKER=( - " broker:" - " topic: base-topic" -)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test-config-loading.sh` around lines 128 - 131, The variable BASE_BROKER is declared but never used; either remove the unused declaration or add tests that reference it: update the test script to use BASE_BROKER in the relevant test cases (e.g., source or include it when building broker config for tests that expect "broker: topic: base-topic") or delete the BASE_BROKER array declaration entirely to satisfy static analysis; locate the BASE_BROKER symbol in test/integration/test-config-loading.sh and either remove the definition or wire it into the test blocks that construct or validate broker configs (so the "base-topic" value is actually exercised).internal/config/config_loading_test.go (1)
359-363: Empty test section header left behind.Lines 359-363 contain only section comment markers for
TestLoadConfig_LegacyBrokerEnvVarswith no implementation. Consider removing this placeholder if legacy broker env vars are no longer applicable, or add a TODO comment if tests are planned.♻️ Suggested cleanup
-// ============================================================================ -// TestLoadConfig_LegacyBrokerEnvVars -// ============================================================================ - - // ============================================================================ // TestLoadConfig_FilePrecedence // ============================================================================🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_loading_test.go` around lines 359 - 363, Remove or complete the empty test section header for TestLoadConfig_LegacyBrokerEnvVars: either delete the placeholder comment block if legacy broker env var tests are no longer needed, or replace it with a short TODO comment and a minimal test stub referencing TestLoadConfig_LegacyBrokerEnvVars so the intent is explicit; ensure you update any surrounding test grouping comments in the file to keep sections consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/config/config_loading_test.go`:
- Around line 359-363: Remove or complete the empty test section header for
TestLoadConfig_LegacyBrokerEnvVars: either delete the placeholder comment block
if legacy broker env var tests are no longer needed, or replace it with a short
TODO comment and a minimal test stub referencing
TestLoadConfig_LegacyBrokerEnvVars so the intent is explicit; ensure you update
any surrounding test grouping comments in the file to keep sections consistent.
In `@test/integration/test-config-loading.sh`:
- Around line 128-131: The variable BASE_BROKER is declared but never used;
either remove the unused declaration or add tests that reference it: update the
test script to use BASE_BROKER in the relevant test cases (e.g., source or
include it when building broker config for tests that expect "broker: topic:
base-topic") or delete the BASE_BROKER array declaration entirely to satisfy
static analysis; locate the BASE_BROKER symbol in
test/integration/test-config-loading.sh and either remove the definition or wire
it into the test blocks that construct or validate broker configs (so the
"base-topic" value is actually exercised).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dae37ce1-6e5e-4171-9526-b91756343c63
📒 Files selected for processing (25)
README.mdcharts/README.mdcharts/templates/configmap.yamlcharts/templates/deployment.yamlcharts/values.yamlcmd/sentinel/main.goconfigs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamldocs/configuration.mddocs/running-sentinel.mdinternal/client/client.gointernal/client/client_test.gointernal/config/config.gointernal/config/config_loading_test.gointernal/config/config_test.gointernal/config/testdata/full-workflow.yamlinternal/config/testdata/message-data-blank-id.yamlinternal/config/testdata/minimal.yamlinternal/config/testdata/unknown-field.yamlinternal/config/testdata/valid-complete.yamlinternal/sentinel/sentinel.gointernal/sentinel/sentinel_test.gotest/integration/integration_test.gotest/integration/test-config-loading.sh
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/config/testdata/minimal.yaml
- cmd/sentinel/main.go
- internal/client/client.go
- internal/sentinel/sentinel.go
- internal/config/testdata/message-data-blank-id.yaml
- charts/README.md
- internal/config/testdata/unknown-field.yaml
- docs/configuration.md
- configs/dev-example.yaml
- docs/running-sentinel.md
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
2 times, most recently
from
9a773c8 to
2806de1
Compare
|
Category: JIRA The PR description says it adds CONFIGURATION_MIGRATION.md for operators migrating from the |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
cmd/sentinel/main.go (1)
354-358:⚠️ Potential issue | 🟠 Major
config-dumpstill emits raw secrets by default.This marshals the unredacted merged config, so sensitive values such as
clients.hyperfleet_api.default_headerswill be printed verbatim to stdout and are easy to leak into CI logs or shared terminals. Safer default iscfg.RedactedCopy(), with a separate explicit opt-in if raw output is still needed.Safer default
- data, err := yaml.Marshal(cfg) + data, err := yaml.Marshal(cfg.RedactedCopy())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/sentinel/main.go` around lines 354 - 358, The config-dump currently marshals and prints the unredacted merged config (cfg), which exposes secrets; change the marshaling to use cfg.RedactedCopy() by default (i.e., call yaml.Marshal(cfg.RedactedCopy())) and print that redacted output, and if raw output is still required add/obey an explicit opt-in flag (e.g., --raw) that, when set, marshals the original cfg via yaml.Marshal(cfg) so raw secrets are only emitted when explicitly requested; update the code paths around yaml.Marshal and fmt.Print to reference cfg.RedactedCopy() and the new --raw guard.internal/config/config.go (1)
360-365:⚠️ Potential issue | 🟠 Major
RedactedCopy()is still shallow for nestedmessage_datavalues
MessageDatais only copied at the top level. Nested maps/slices remain shared, so mutating the returned copy can still mutate live config, which breaks the “deep copy” contract.Proposed fix
func (c *SentinelConfig) RedactedCopy() *SentinelConfig { cp := *c @@ if c.MessageData != nil { - md := make(map[string]interface{}, len(c.MessageData)) - for k, v := range c.MessageData { - md[k] = v - } - cp.MessageData = md + cp.MessageData = deepCopyInterfaceMap(c.MessageData) } @@ return &cp } + +func deepCopyInterfaceMap(src map[string]interface{}) map[string]interface{} { + dst := make(map[string]interface{}, len(src)) + for k, v := range src { + dst[k] = deepCopyAny(v) + } + return dst +} + +func deepCopyAny(v interface{}) interface{} { + switch t := v.(type) { + case map[string]interface{}: + return deepCopyInterfaceMap(t) + case []interface{}: + out := make([]interface{}, len(t)) + for i := range t { + out[i] = deepCopyAny(t[i]) + } + return out + default: + return t + } +}As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 360 - 365, RedactedCopy() currently copies c.MessageData only at the top level (md[k]=v), leaving nested maps/slices shared; change it to perform a deep copy of MessageData so mutations on cp.MessageData cannot affect the original. Implement and call a helper like deepCopyValue/deepCopyMap that recursively copies map[string]interface{} and []interface{} (and preserves primitives), then assign the result to cp.MessageData instead of the shallow md; ensure the helper is used for each value when populating cp.MessageData in RedactedCopy().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/templates/configmap.yaml`:
- Around line 78-83: The template currently uses truthy guards that drop keys
when values are zero; update the guards to use the hasKey pattern (as used for
create_topic_if_missing) so explicit 0 is preserved. Replace the conditional
blocks that reference .Values.broker.rabbitmq.prefetch_count and
.Values.broker.rabbitmq.prefetch_size with hasKey checks (e.g. hasKey
.Values.broker.rabbitmq "prefetch_count"), and apply the same change for
max_outstanding_messages and num_goroutines so those keys are rendered when set
to 0.
In `@docs/running-sentinel.md`:
- Around line 600-601: The example YAML still uses the old flat service key;
update the snippet so the hyperfleet_api entry is nested under a clients mapping
and uses base_url instead of endpoint — i.e., replace the flat "hyperfleet_api:
base_url: ..." block with a "clients:" parent key containing a "hyperfleet_api:"
child that defines "base_url:
http://hyperfleet-api.hyperfleet-system.svc.cluster.local:8080" so the loader
sees the new clients nesting (look for the hyperfleet_api, endpoint and base_url
symbols to locate where to change).
In `@internal/client/client.go`:
- Around line 51-63: NewHyperFleetClient currently only sets the User-Agent and
ignores the configured default headers; change the constructor to accept the
configured default headers (e.g., a map[string]string from the config layer) and
in the openapi.WithRequestEditorFn used when calling
openapi.NewClientWithResponses, iterate the default headers and set them on
req.Header (ensuring User-Agent stays set by userAgent unless explicitly
overridden), so that clients.hyperfleet_api.default_headers are applied to every
request; update NewHyperFleetClient signature and its request editor closure
(references: NewHyperFleetClient, userAgent, openapi.NewClientWithResponses,
openapi.WithRequestEditorFn) accordingly.
---
Duplicate comments:
In `@cmd/sentinel/main.go`:
- Around line 354-358: The config-dump currently marshals and prints the
unredacted merged config (cfg), which exposes secrets; change the marshaling to
use cfg.RedactedCopy() by default (i.e., call yaml.Marshal(cfg.RedactedCopy()))
and print that redacted output, and if raw output is still required add/obey an
explicit opt-in flag (e.g., --raw) that, when set, marshals the original cfg via
yaml.Marshal(cfg) so raw secrets are only emitted when explicitly requested;
update the code paths around yaml.Marshal and fmt.Print to reference
cfg.RedactedCopy() and the new --raw guard.
In `@internal/config/config.go`:
- Around line 360-365: RedactedCopy() currently copies c.MessageData only at the
top level (md[k]=v), leaving nested maps/slices shared; change it to perform a
deep copy of MessageData so mutations on cp.MessageData cannot affect the
original. Implement and call a helper like deepCopyValue/deepCopyMap that
recursively copies map[string]interface{} and []interface{} (and preserves
primitives), then assign the result to cp.MessageData instead of the shallow md;
ensure the helper is used for each value when populating cp.MessageData in
RedactedCopy().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f587e82a-96bb-4ab7-aa99-7025b415c050
📒 Files selected for processing (27)
MakefileREADME.mdcharts/README.mdcharts/templates/configmap.yamlcharts/templates/deployment.yamlcharts/values.yamlcmd/sentinel/main.goconfigs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamldocs/configuration.mddocs/multi-instance-deployment.mddocs/running-sentinel.mdinternal/client/client.gointernal/client/client_test.gointernal/config/config.gointernal/config/config_loading_test.gointernal/config/config_test.gointernal/config/testdata/full-workflow.yamlinternal/config/testdata/message-data-blank-id.yamlinternal/config/testdata/minimal.yamlinternal/config/testdata/unknown-field.yamlinternal/config/testdata/valid-complete.yamlinternal/sentinel/sentinel.gointernal/sentinel/sentinel_test.gotest/integration/integration_test.gotest/integration/test-config-loading.sh
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/sentinel/sentinel.go
- internal/config/testdata/message-data-blank-id.yaml
- README.md
- internal/config/testdata/unknown-field.yaml
- internal/config/testdata/minimal.yaml
- configs/dev-example.yaml
- internal/config/testdata/valid-complete.yaml
- configs/rabbitmq-example.yaml
- internal/config/config_loading_test.go
- charts/templates/deployment.yaml
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
from
2806de1 to
a1439f5
Compare
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
from
a1439f5 to
c492a1f
Compare
This may have been a leftover, but now is not present anymore |
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
from
c492a1f to
ec1023a
Compare
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
from
ec1023a to
550f021
Compare
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
from
550f021 to
4f6c4dc
Compare
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
2 times, most recently
from
195b9d1 to
5f6f8a2
Compare
|
The following file is NOT in this PR but may need updating due to changes in the diff:
|
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
from
5f6f8a2 to
be73f8b
Compare
| cfg.Output = parsed | ||
| } | ||
|
|
||
| // TRACING_ENABLED=true enables tracing |
There was a problem hiding this comment.
Category: Bug
Lines 187-194 are an exact duplicate of lines 196-203 — both parse
TRACING_ENABLED and set cfg.OTel.Enabled. Looks like the first block was
introduced during the refactor and the original wasn't removed. Delete one of
them.
This could be caused during the rebase.
| # Should match the actual message broker being used for accurate observability. | ||
| # Valid values: rabbitmq, gcp_pubsub, kafka, activemq, aws_sqs, etc. | ||
| # Can be overridden via MESSAGING_SYSTEM environment variable. | ||
| messaging_system: "gcp_pubsub" |
There was a problem hiding this comment.
Category: Bug
messaging_system appears twice in this file (lines 60 and 66). Same issue in
configs/rabbitmq-example.yaml (lines 60, 66) and configs/dev-example.yaml
(lines 57, 63). Looks like the old block wasn't removed when the new
structure was added — just delete the duplicate in each file.
4 participants
Summary
names both in config files and in config-dump output.
inspect the effective config at runtime or in CI.
hyperfleet-sentinel/<version> (<name>).loads correctly from all three sources (file, env var, CLI flag) and that CLI > env > file precedence is respected.
freshly cross-compiled Linux binary, so the full config path is exercised on Linux in CI.
flat schema.
Test plan
request logs
Summary by CodeRabbit
New Features
Configuration
Documentation
Tests