HYPERFLEET-549 - feat: standard configuration#71
HYPERFLEET-549 - feat: standard configuration#71rh-amarin wants to merge 1 commit 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 |
internal/config/config.go
Outdated
| } | ||
|
|
||
| // Legacy broker env vars without HYPERFLEET_ prefix (kept for compatibility) | ||
| if os.Getenv(EnvPrefix+"_BROKER_SUBSCRIPTION_ID") == "" { |
There was a problem hiding this comment.
Priority: Inconsistency
Lines 217-224 use LOG_LEVEL, LOG_FORMAT, LOG_OUTPUT without HYPERFLEET_ prefix, but the
logging specification table says HYPERFLEET_LOG_LEVEL, HYPERFLEET_LOG_FORMAT,
HYPERFLEET_LOG_OUTPUT.
There was a problem hiding this comment.
Changed all of them
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
2 times, most recently
from
d30dabd to
a4e1d5f
Compare
| HyperfleetAPI: &HyperFleetAPIConfig{ | ||
| Version: "v1", | ||
| Timeout: 10 * time.Second, | ||
| RetryAttempts: 3, |
There was a problem hiding this comment.
Interesting, so we load in these configs, but looks like we hardcode them, is there a follow up for this?, looking at your template values they are the same, just wondering maybe this was missed
There was a problem hiding this comment.
Those are default values, so if no file, env variable or command line parameter are set, these values are used.
But, to check it I tried changing "v1" and found out that is not used in the code, since /api/hyperfleet/v1/clusters comes hardcoded in the generated code from openapi.yaml
I will leave this since we will change to a generic client in the near future and then it will be used.
| cfg := NewSentinelConfig() | ||
|
|
||
| // Load from YAML file | ||
| if configFile == "" { |
There was a problem hiding this comment.
Category: Standards
The HyperFleet configuration standard says:
The config file location is flexible and can be determined by:
- Path specified via --config flag (if provided)
- Path specified via HYPERFLEET_CONFIG environment variable
- Default values (/etc/hyperfleet/config.yaml or ./configs/config.yaml)
Currently configFile is only set via --config / -c (main.go:97), and LoadConfig returns an
error if it's empty (config.go:177) without checking HYPERFLEET_CONFIG or trying default
paths.
Suggestion — in main.go, resolve the config path before calling LoadConfig:
if configFile == "" {
configFile = os.Getenv("HYPERFLEET_CONFIG")
}
if configFile == "" {
for _, p := range []string{"/etc/hyperfleet/config.yaml", "./configs/config.yaml"} {
if _, err := os.Stat(p); err == nil {
configFile = p
break
}
}
}Also update docs/configuration.md lines 10-13 to document the env var and default paths.
There was a problem hiding this comment.
We have hardcoded in the deployment.yaml
args:
- serve
- --config=/etc/sentinel/config.yaml
There was a problem hiding this comment.
But I added it nonetheless, so all apps should behave as the standard says
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
from
a4e1d5f to
361a1cd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
cmd/sentinel/main.go (1)
379-379:⚠️ Potential issue | 🟠 Major
config-dumpcurrently emits merged config in plaintext.Marshaling
cfgdirectly can leak sensitive values (for example from header maps) into terminal/CI logs. Defaulting to a redacted copy is safer.Suggested 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` at line 379, The config-dump currently marshals cfg directly which can leak secrets; instead create and marshal a redacted copy (e.g., call a helper like redactConfig(cfg) or cfg.Redacted()) that copies the config and replaces sensitive fields (authorization/header maps, API keys, tokens, secret strings, TLS private keys, etc.) with placeholder values before calling yaml.Marshal; update the code around the data, err := yaml.Marshal(cfg) line to use the redacted copy so terminal/CI logs never contain raw secrets.
🧹 Nitpick comments (1)
charts/values.yaml (1)
109-113: Unused field may confuse users.
config.clients.broker.topicis defined here but never used incharts/templates/configmap.yaml. The template at line 40 sources the topic from.Values.broker.topic(line 159) instead.Consider either:
- Removing this field entirely, or
- Using this field in the configmap template instead of
.Values.broker.topicThis would prevent users from editing the wrong field when configuring their topic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/values.yaml` around lines 109 - 113, The values.yaml defines config.clients.broker.topic but the configmap template (charts/templates/configmap.yaml) is reading .Values.broker.topic, causing the former to be unused and confusing users; either remove config.clients.broker.topic from values.yaml or update the configmap template to read the topic from .Values.config.clients.broker.topic (replace occurrences of .Values.broker.topic with .Values.config.clients.broker.topic) so the intended field is actually applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configs/dev-example.yaml`:
- Line 26: Update the stale example comment that reads "Logging configuration
(can be overridden by LOG_* env vars or --log-* flags)" to reference the actual
env-var prefix "HYPERFLEET_LOG_*" (and, if relevant, the CLI flag prefix) so the
comment becomes something like "Logging configuration (can be overridden by
HYPERFLEET_LOG_* env vars or --log-* flags)"; locate the string in the file (the
exact comment text) and replace the LOG_* token with HYPERFLEET_LOG_*.
In `@configs/gcp-pubsub-example.yaml`:
- Line 21: Update the logging comment text so the env-var prefix matches the
current config contract: replace the referenced `LOG_*` token in the comment
string with `HYPERFLEET_LOG_*` (the comment that currently reads "Logging
configuration (can be overridden by LOG_* env vars or --log-* flags)") so
operators see the correct env-var prefix; leave the rest of the comment and the
`--log-*` flag text unchanged.
In `@internal/config/config.go`:
- Around line 72-75: BrokerConfig currently only defines Topic but the tests
expect broker.subscription_id; add a SubscriptionID string field to the
BrokerConfig struct (alongside Topic) and annotate it with
yaml:"subscription_id,omitempty" and mapstructure:"subscription_id" so config
file, env and flag loading (tests referencing broker.subscription_id,
HYPERFLEET_BROKER_SUBSCRIPTION_ID / BROKER_SUBSCRIPTION_ID, and CLI flags) can
populate it; update the struct definition in internal/config/config.go
(BrokerConfig) accordingly.
In `@test/integration/test-config-loading.sh`:
- Around line 68-69: The ROOT_DIR calculation is wrong (it only ascends one
level from SCRIPT_DIR); update how ROOT_DIR is derived so it points to the
repository root—for example, change the computation of ROOT_DIR (currently using
SCRIPT_DIR and "..") to ascend two levels (../..) or use a repo-root command
(e.g., git rev-parse --show-toplevel) so references to ROOT_DIR (used to call
$ROOT_DIR/bin/sentinel and run make build) resolve correctly; update the
assignment where ROOT_DIR is defined to use the corrected path resolution.
---
Duplicate comments:
In `@cmd/sentinel/main.go`:
- Line 379: The config-dump currently marshals cfg directly which can leak
secrets; instead create and marshal a redacted copy (e.g., call a helper like
redactConfig(cfg) or cfg.Redacted()) that copies the config and replaces
sensitive fields (authorization/header maps, API keys, tokens, secret strings,
TLS private keys, etc.) with placeholder values before calling yaml.Marshal;
update the code around the data, err := yaml.Marshal(cfg) line to use the
redacted copy so terminal/CI logs never contain raw secrets.
---
Nitpick comments:
In `@charts/values.yaml`:
- Around line 109-113: The values.yaml defines config.clients.broker.topic but
the configmap template (charts/templates/configmap.yaml) is reading
.Values.broker.topic, causing the former to be unused and confusing users;
either remove config.clients.broker.topic from values.yaml or update the
configmap template to read the topic from .Values.config.clients.broker.topic
(replace occurrences of .Values.broker.topic with
.Values.config.clients.broker.topic) so the intended field is actually applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ace1ed10-b248-4ffa-8bff-8c54bcc57c88
📒 Files selected for processing (24)
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
| # Debug configuration - log merged config on startup | ||
| debug_config: false | ||
|
|
||
| # Logging configuration (can be overridden by LOG_* env vars or --log-* flags) |
There was a problem hiding this comment.
Update logging env-var prefix in comment to match current config contract.
Line 21 still references LOG_*; this should be HYPERFLEET_LOG_* to avoid operator confusion.
Suggested fix
-# Logging configuration (can be overridden by LOG_* env vars or --log-* flags)
+# Logging configuration (can be overridden by HYPERFLEET_LOG_* env vars or --log-* flags)📝 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.
| # Logging configuration (can be overridden by LOG_* env vars or --log-* flags) | |
| # Logging configuration (can be overridden by HYPERFLEET_LOG_* env vars or --log-* flags) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@configs/gcp-pubsub-example.yaml` at line 21, Update the logging comment text
so the env-var prefix matches the current config contract: replace the
referenced `LOG_*` token in the comment string with `HYPERFLEET_LOG_*` (the
comment that currently reads "Logging configuration (can be overridden by LOG_*
env vars or --log-* flags)") so operators see the correct env-var prefix; leave
the rest of the comment and the `--log-*` flag text unchanged.
| // BrokerConfig contains broker configuration | ||
| type BrokerConfig struct { | ||
| Topic string `yaml:"topic,omitempty" mapstructure:"topic"` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for subscription_id references in the codebase
rg -n "subscription_id" --type go
rg -n "subscription_id" test/integration/test-config-loading.shRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 1611
🏁 Script executed:
sed -n '50,90p' internal/config/config.goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 1738
🏁 Script executed:
rg -B5 -A10 "type BrokerConfig" internal/config/config.goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 603
BrokerConfig is missing the subscription_id field.
The test file (test/integration/test-config-loading.sh) extensively tests broker.subscription_id across multiple sources—file configuration (line 303), environment variables (HYPERFLEET_BROKER_SUBSCRIPTION_ID and legacy BROKER_SUBSCRIPTION_ID on lines 305, 310, 312), and CLI flags (line 306)—but BrokerConfig struct (lines 72-75) only defines a Topic field. Add the missing SubscriptionID field with appropriate yaml and mapstructure tags.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/config.go` around lines 72 - 75, BrokerConfig currently only
defines Topic but the tests expect broker.subscription_id; add a SubscriptionID
string field to the BrokerConfig struct (alongside Topic) and annotate it with
yaml:"subscription_id,omitempty" and mapstructure:"subscription_id" so config
file, env and flag loading (tests referencing broker.subscription_id,
HYPERFLEET_BROKER_SUBSCRIPTION_ID / BROKER_SUBSCRIPTION_ID, and CLI flags) can
populate it; update the struct definition in internal/config/config.go
(BrokerConfig) accordingly.
f49550c to
30595a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
configs/gcp-pubsub-example.yaml (1)
21-21:⚠️ Potential issue | 🟡 MinorUpdate logging env-var prefix to match current contract.
Line 21 references
LOG_*; it should beHYPERFLEET_LOG_*.Suggested fix
-# Logging configuration (can be overridden by LOG_* env vars or --log-* flags) +# Logging configuration (can be overridden by HYPERFLEET_LOG_* env vars or --log-* flags)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/gcp-pubsub-example.yaml` at line 21, Update the logging env-var prefix in the comment that currently references "LOG_*" to the new contract "HYPERFLEET_LOG_*"; locate the comment string containing "Logging configuration (can be overridden by LOG_* env vars or --log-* flags)" and replace the "LOG_*" and any related "--log-*" flag mentions with "HYPERFLEET_LOG_*" and corresponding "--hyperfleet-log-*" phrasing to reflect the current env-var contract.configs/dev-example.yaml (1)
26-26:⚠️ Potential issue | 🟡 MinorFix stale logging env-var prefix in comment.
Line 26 still says
LOG_*, but the active prefix isHYPERFLEET_LOG_*, which can cause misconfiguration.Suggested fix
-# Logging configuration (can be overridden by LOG_* env vars or --log-* flags) +# Logging configuration (can be overridden by HYPERFLEET_LOG_* env vars or --log-* flags)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/dev-example.yaml` at line 26, Update the stale comment that mentions LOG_* to reflect the actual environment variable prefix HYPERFLEET_LOG_* so users don't misconfigure logging; locate the logging configuration comment containing the text "LOG_*" and replace it with "HYPERFLEET_LOG_*" (or reword to indicate that HYPERFLEET_LOG_* env vars or --log-* flags can override settings) so the documentation matches the active prefix.
🧹 Nitpick comments (2)
test/integration/integration_test.go (1)
148-148: Don’t ignoreNewHyperFleetClienterrors in tests.At Lines 148, 264, 375, and 478, discarding the error (
_) can hide client setup failures and lead to harder-to-debug test behavior.Suggested fix pattern
-hyperfleetClient, _ := client.NewHyperFleetClient(server.URL, 10*time.Second, "test-sentinel", "test") +hyperfleetClient, err := client.NewHyperFleetClient(server.URL, 10*time.Second, "test-sentinel", "test") +if err != nil { + t.Fatalf("NewHyperFleetClient failed: %v", err) +}Also applies to: 264-264, 375-375, 478-478
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/integration_test.go` at line 148, Tests are currently discarding errors returned by NewHyperFleetClient (calls that assign to hyperfleetClient, _), which can mask setup failures; update each invocation of NewHyperFleetClient to capture the error and fail the test on error—e.g., replace the blank identifier with err and call t.Fatalf or require.NoError to report the failure—so locate the calls creating hyperfleetClient (invocations of NewHyperFleetClient) and ensure you handle the returned error immediately after creation.internal/config/config.go (1)
318-329:RedactedCopydoes not deep-copyHyperfleetAPI, leavingDefaultHeadersshared.The method deep-copies
Brokerbut notHyperfleetAPI. IfDefaultHeaders(a map) is modified on the copy, it will mutate the original config. While not currently problematic, this inconsistency could cause subtle bugs if the copy is modified later.Consider also duplicating
HyperfleetAPIfor consistency:♻️ Suggested improvement
func (c *SentinelConfig) RedactedCopy() *SentinelConfig { cp := *c + if cp.Clients.HyperfleetAPI != nil { + api := *cp.Clients.HyperfleetAPI + cp.Clients.HyperfleetAPI = &api + } if cp.Clients.Broker != nil { b := *cp.Clients.Broker cp.Clients.Broker = &b } return &cp }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 318 - 329, RedactedCopy currently shallow-copies SentinelConfig and explicitly deep-copies Clients.Broker but leaves Clients.HyperfleetAPI as a shared struct so its DefaultHeaders map remains shared; update RedactedCopy to also duplicate the HyperfleetAPI value and make a new map copy for HyperfleetAPI.DefaultHeaders (copying keys/values) so modifications to the returned SentinelConfig won't mutate the original; locate the RedactedCopy method and add logic to clone cp.Clients.HyperfleetAPI (e.g., create a new HyperfleetAPI value and, if DefaultHeaders != nil, allocate a new map and copy entries) before returning &cp.
🤖 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/values.yaml`:
- Around line 80-83: The comment for the sentinel name says it's shard-aware but
the actual default template for the name field (hyperfleet-sentinel-{{
.Values.config.resource_type }}) only injects .Values.config.resource_type;
either update that comment to reflect the actual behavior or change the template
to include a shard or release identifier (for example add {{ .Values.shard }} or
{{ .Release.Name }} to the name template) so the generated name is
shard/release-distinguishable; modify the name key template or the preceding
comment accordingly to keep them consistent.
In `@configs/rabbitmq-example.yaml`:
- Line 21: Update the example comment string "Logging configuration (can be
overridden by LOG_* env vars or --log-* flags)" to use the HYPERFLEET prefix for
environment variables by replacing LOG_* with HYPERFLEET_LOG_* (i.e., comment
should read "can be overridden by HYPERFLEET_LOG_* env vars..."); locate and
edit that comment text in configs/rabbitmq-example.yaml to apply the change.
In `@docs/configuration.md`:
- Around line 39-40: The docs currently advertise
clients.hyperfleet_api.default_headers but the runtime only injects User-Agent;
either remove this config from docs or implement it end-to-end: update
internal/client.NewHyperFleetClient to accept a defaultHeaders
map[string]string, update cmd/sentinel/main.go to read
clients.hyperfleet_api.default_headers from config and pass it into
NewHyperFleetClient, and modify internal/client/client.go (request creation code
that currently sets User-Agent) to iterate and add all default headers to every
outgoing request; also ensure the config schema includes default_headers so
startup won’t fail.
In `@test/integration/test-config-loading.sh`:
- Around line 11-12: Remove the global abort-on-error behavior caused by "set
-euo pipefail" for cfg_dump calls by implementing a small helper (e.g.,
run_cfg_dump) that runs cfg_dump, captures stdout and stderr and the exit code,
and returns them (or writes them to temporary vars/files) instead of letting the
shell exit; update all direct $(cfg_dump ...) invocations to use run_cfg_dump
and pass its captured output and exit status into
assert_contains/assert_not_contains so those assertions can convert non-zero
exits into FAIL entries and preserve verbose stderr (don't redirect stderr to
/dev/null in the helper so --verbose can show errors).
---
Duplicate comments:
In `@configs/dev-example.yaml`:
- Line 26: Update the stale comment that mentions LOG_* to reflect the actual
environment variable prefix HYPERFLEET_LOG_* so users don't misconfigure
logging; locate the logging configuration comment containing the text "LOG_*"
and replace it with "HYPERFLEET_LOG_*" (or reword to indicate that
HYPERFLEET_LOG_* env vars or --log-* flags can override settings) so the
documentation matches the active prefix.
In `@configs/gcp-pubsub-example.yaml`:
- Line 21: Update the logging env-var prefix in the comment that currently
references "LOG_*" to the new contract "HYPERFLEET_LOG_*"; locate the comment
string containing "Logging configuration (can be overridden by LOG_* env vars or
--log-* flags)" and replace the "LOG_*" and any related "--log-*" flag mentions
with "HYPERFLEET_LOG_*" and corresponding "--hyperfleet-log-*" phrasing to
reflect the current env-var contract.
---
Nitpick comments:
In `@internal/config/config.go`:
- Around line 318-329: RedactedCopy currently shallow-copies SentinelConfig and
explicitly deep-copies Clients.Broker but leaves Clients.HyperfleetAPI as a
shared struct so its DefaultHeaders map remains shared; update RedactedCopy to
also duplicate the HyperfleetAPI value and make a new map copy for
HyperfleetAPI.DefaultHeaders (copying keys/values) so modifications to the
returned SentinelConfig won't mutate the original; locate the RedactedCopy
method and add logic to clone cp.Clients.HyperfleetAPI (e.g., create a new
HyperfleetAPI value and, if DefaultHeaders != nil, allocate a new map and copy
entries) before returning &cp.
In `@test/integration/integration_test.go`:
- Line 148: Tests are currently discarding errors returned by
NewHyperFleetClient (calls that assign to hyperfleetClient, _), which can mask
setup failures; update each invocation of NewHyperFleetClient to capture the
error and fail the test on error—e.g., replace the blank identifier with err and
call t.Fatalf or require.NoError to report the failure—so locate the calls
creating hyperfleetClient (invocations of NewHyperFleetClient) and ensure you
handle the returned error immediately after creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68e85cf8-d838-4511-850e-709595817072
📒 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 as they are similar to previous changes (7)
- internal/config/testdata/message-data-blank-id.yaml
- internal/config/testdata/minimal.yaml
- internal/client/client.go
- internal/config/testdata/unknown-field.yaml
- internal/config/config_loading_test.go
- docs/running-sentinel.md
- README.md
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
from
30595a6 to
d08733f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/integration/test-config-loading.sh (1)
11-12:⚠️ Potential issue | 🟠 MajorCapture
config-dumpfailures without aborting test reportingWith
set -euo pipefailon Line 11 plus direct$(cfg_dump ...)usage, a non-zeroconfig-dumpcan terminate the script before assertions record a FAIL. Also, Lines 84 and 88 discard stderr, so--verbosecannot show root-cause output. Routeconfig-dumpthrough a helper that captures stdout/stderr/exit code and lets assertions decide pass/fail.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 82-89
🤖 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 11 - 12, The script currently enables strict errexit with "set -euo pipefail" and invokes cfg_dump directly (and redirects stderr away in places), which causes the whole test to abort on non-zero config-dump and hides root-cause output; add a helper function (e.g., run_cfg_dump) that invokes cfg_dump capturing stdout, stderr and the exit code (without letting errexit terminate the script), return or populate variables like CFG_OUT, CFG_ERR, CFG_RC, and update places that call cfg_dump (including the calls that previously discarded stderr) to use this helper and make assertions check CFG_RC and include CFG_ERR when printing failures so --verbose can reveal the underlying error instead of the script exiting early.
🧹 Nitpick comments (3)
test/integration/integration_test.go (1)
148-149: HandleNewHyperFleetClienterrors explicitly in integration testsConstructor errors are being discarded with blank identifiers at lines 148, 264, 375, and 478. If client construction fails, tests continue with invalid setup and fail less clearly later. Use
t.Fatalf(...)to fail fast at these points.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/integration_test.go` around lines 148 - 149, The test currently discards errors from NewHyperFleetClient (called into hyperfleetClient) causing obscure failures; change each construct that uses the blank identifier (e.g., hyperfleetClient, _ := client.NewHyperFleetClient(...)) to capture the error (hyperfleetClient, err := client.NewHyperFleetClient(...)) and immediately fail the test on error with t.Fatalf("NewHyperFleetClient failed: %v", err). Apply the same pattern for every occurrence of NewHyperFleetClient in the file so the test fails fast on client construction failures.internal/sentinel/sentinel_test.go (1)
131-132: Consider adding explicit error handling in test setupThe ignored errors from
NewHyperFleetClientconstructor calls across test setup (lines 131, 208, 262, 319, 379, 445, 514) should be explicitly checked. This would make test failures more deterministic and easier to diagnose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sentinel/sentinel_test.go` around lines 131 - 132, The tests currently ignore the error returned by NewHyperFleetClient (e.g., hyperfleetClient, _ := client.NewHyperFleetClient(...)), which can hide setup failures; update each call (lines referencing NewHyperFleetClient) to capture the error, assert it is nil, and fail the test on error (e.g., use t.Fatalf or require.NoError) so failures are deterministic—locate the constructor calls to NewHyperFleetClient and replace the blank error discard with explicit error handling for the test's setup.internal/client/client_test.go (1)
85-85: Stop discardingNewHyperFleetClienterrors in tests.On Line 85 and similar call sites,
client, _ := ...hides constructor failures and makes test failures harder to diagnose.Suggested pattern
-client, _ := NewHyperFleetClient(server.URL, 10*time.Second, "test-sentinel", "test") +client, err := NewHyperFleetClient(server.URL, 10*time.Second, "test-sentinel", "test") +if err != nil { + t.Fatalf("NewHyperFleetClient: %v", err) +}As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 120-120, 141-141, 171-171, 216-216, 254-254, 275-275, 298-298, 327-327, 340-340, 357-357, 409-409, 599-599, 677-677
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/client/client_test.go` at line 85, Tests are currently discarding errors from NewHyperFleetClient (e.g., client, _ := NewHyperFleetClient(...)), which hides constructor failures; update each test invocation to capture the error (client, err := NewHyperFleetClient(...)) and assert it succeeded (use t.Fatalf("...: %v", err) or require.NoError(t, err)) before using client so test failures surface the real cause; apply the same change for all listed call sites that call NewHyperFleetClient and then use the returned client.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/config.go`:
- Around line 331-340: RedactedCopy currently only duplicates the top-level
struct and Clients.Broker, leaving other nested pointers, slices, and maps
shared; update SentinelConfig.RedactedCopy to produce a deep copy of all mutable
nested fields (not just Clients.Broker) by allocating new instances for any
pointer members, copying slice contents into new slices, and cloning maps into
new maps so mutations of the returned cp cannot affect the live config; ensure
you handle all pointer fields and common mutable types found on SentinelConfig
(e.g., Clients, any pointer-typed subfields, slices, and maps) when creating cp.
---
Duplicate comments:
In `@test/integration/test-config-loading.sh`:
- Around line 11-12: The script currently enables strict errexit with "set -euo
pipefail" and invokes cfg_dump directly (and redirects stderr away in places),
which causes the whole test to abort on non-zero config-dump and hides
root-cause output; add a helper function (e.g., run_cfg_dump) that invokes
cfg_dump capturing stdout, stderr and the exit code (without letting errexit
terminate the script), return or populate variables like CFG_OUT, CFG_ERR,
CFG_RC, and update places that call cfg_dump (including the calls that
previously discarded stderr) to use this helper and make assertions check CFG_RC
and include CFG_ERR when printing failures so --verbose can reveal the
underlying error instead of the script exiting early.
---
Nitpick comments:
In `@internal/client/client_test.go`:
- Line 85: Tests are currently discarding errors from NewHyperFleetClient (e.g.,
client, _ := NewHyperFleetClient(...)), which hides constructor failures; update
each test invocation to capture the error (client, err :=
NewHyperFleetClient(...)) and assert it succeeded (use t.Fatalf("...: %v", err)
or require.NoError(t, err)) before using client so test failures surface the
real cause; apply the same change for all listed call sites that call
NewHyperFleetClient and then use the returned client.
In `@internal/sentinel/sentinel_test.go`:
- Around line 131-132: The tests currently ignore the error returned by
NewHyperFleetClient (e.g., hyperfleetClient, _ :=
client.NewHyperFleetClient(...)), which can hide setup failures; update each
call (lines referencing NewHyperFleetClient) to capture the error, assert it is
nil, and fail the test on error (e.g., use t.Fatalf or require.NoError) so
failures are deterministic—locate the constructor calls to NewHyperFleetClient
and replace the blank error discard with explicit error handling for the test's
setup.
In `@test/integration/integration_test.go`:
- Around line 148-149: The test currently discards errors from
NewHyperFleetClient (called into hyperfleetClient) causing obscure failures;
change each construct that uses the blank identifier (e.g., hyperfleetClient, _
:= client.NewHyperFleetClient(...)) to capture the error (hyperfleetClient, err
:= client.NewHyperFleetClient(...)) and immediately fail the test on error with
t.Fatalf("NewHyperFleetClient failed: %v", err). Apply the same pattern for
every occurrence of NewHyperFleetClient in the file so the test fails fast on
client construction failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8dd31644-8ed4-4fbf-875b-cc79721d95a6
📒 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 as they are similar to previous changes (7)
- internal/sentinel/sentinel.go
- docs/running-sentinel.md
- configs/dev-example.yaml
- internal/config/testdata/message-data-blank-id.yaml
- internal/client/client.go
- README.md
- docs/configuration.md
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
from
d08733f to
6d23adb
Compare
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
from
6d23adb to
9a773c8
Compare
rh-amarin
force-pushed
the
HYPERFLEET-549
branch
from
9a773c8 to
2806de1
Compare
|
|
||
| // Initialize components | ||
| hyperfleetClient, err := client.NewHyperFleetClient(cfg.HyperFleetAPI.Endpoint, cfg.HyperFleetAPI.Timeout) | ||
| hyperfleetClient, err := client.NewHyperFleetClient(cfg.Clients.HyperfleetAPI.BaseURL, cfg.Clients.HyperfleetAPI.Timeout, cfg.Sentinel.Name, version) |
There was a problem hiding this comment.
Category: Bug
Line 219 creates the client with only BaseURL, Timeout, Name, and version — but the config
also defines retry_attempts, retry_backoff, base_delay, max_delay, and version (the API
version, not build version) that are never passed to the client. Meanwhile client.go has
hardcoded retry constants (DefaultInitialInterval = 500ms, DefaultMaxInterval = 8s) that don't
match the config defaults (base_delay: 1s, max_delay: 30s).
This means config-dump shows retry_attempts: 3 giving operators the impression retries are
configurable, but changing these values has zero effect. CodeRabbit flagged the same for
default_headers but the retry fields are more impactful since they have non-zero defaults.
Options: (1) wire these into NewHyperFleetClient and the backoff logic in FetchResources, (2)
remove them from config/docs/flags until implementation is ready, or (3) mark them as
"reserved — not yet implemented" in the docs.
|
Category: JIRA The PR description says it adds CONFIGURATION_MIGRATION.md for operators migrating from the |
|
|
||
| You can point the sentinel at a config file with: | ||
|
|
||
| - CLI: `--config` (or `-c`) |
There was a problem hiding this comment.
Category: Standards
HYPERFLEET_CONFIG env var works and is tested (unit test TestLoadConfig_HyperfleetConfigEnvVar
- integration test HYPERFLEET_CONFIG [selects file]), but docs/configuration.md doesn't
mention it. The "Config file location" section only documents --config / -c. Since every other
HYPERFLEET_* env var is documented, this one should be too:
Config file location
| - CLI: `--config` (or `-c`) | |
| - CLI: `--config` (or `-c`) | |
| - Env: `HYPERFLEET_CONFIG` | |
| - Default: `/etc/sentinel/config.yaml` |
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
| {{- if .Values.broker.rabbitmq.prefetch_count }} | ||
| prefetch_count: {{ .Values.broker.rabbitmq.prefetch_count }} | ||
| {{- end }} | ||
| {{- if .Values.broker.rabbitmq.prefetchSize }} | ||
| prefetch_size: {{ .Values.broker.rabbitmq.prefetchSize }} | ||
| {{- if .Values.broker.rabbitmq.prefetch_size }} | ||
| prefetch_size: {{ .Values.broker.rabbitmq.prefetch_size }} | ||
| {{- end }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the current state of configmap.yaml at and around lines 78-83
cd charts/templates && wc -l configmap.yamlRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 102
🏁 Script executed:
# Read the relevant section of configmap.yaml with context around lines 78-95
sed -n '70,100p' charts/templates/configmap.yaml | cat -nRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 1729
🏁 Script executed:
# Search for hasKey usage in configmap.yaml to verify the pattern mentioned
grep -n "hasKey" charts/templates/configmap.yamlRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 165
🏁 Script executed:
# Check for values.yaml or schema that might document these fields
find . -name "values.yaml" -o -name "*schema*" | head -20Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 104
🏁 Script executed:
# Search for any documentation about broker prefetch settings
find . -type f \( -name "*.md" -o -name "*.txt" \) | xargs grep -l "prefetch\|broker" 2>/dev/null | head -10Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 244
🏁 Script executed:
# Check if there are any comments in the configmap about these broker settings
sed -n '1,30p' charts/templates/configmap.yaml | cat -nRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 1323
🏁 Script executed:
# Check values.yaml for broker configuration defaults
grep -A 20 "broker:" charts/values.yaml | head -40Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 1394
🏁 Script executed:
# Look for documentation about broker settings and zero values
grep -A 5 -B 5 "prefetch\|max_outstanding" docs/configuration.mdRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 66
🏁 Script executed:
# Check if configuration.md mentions broker settings at all
grep -i "broker\|prefetch" docs/configuration.mdRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 483
🏁 Script executed:
# Get more of the broker configuration from values.yaml
grep -A 50 "rabbitmq:" charts/values.yaml | head -60Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 1922
🏁 Script executed:
# Check for googlepubsub broker config in values.yaml
grep -A 20 "googlepubsub:" charts/values.yamlRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 788
Preserve explicit zero values for broker tuning.
These truthy guards drop the key entirely when an operator sets 0, so prefetch_count, prefetch_size, max_outstanding_messages, and num_goroutines cannot be intentionally configured to zero. Use the same hasKey pattern you already use for create_topic_if_missing.
Suggested template change
- {{- if .Values.broker.rabbitmq.prefetch_count }}
+ {{- if hasKey .Values.broker.rabbitmq "prefetch_count" }}
prefetch_count: {{ .Values.broker.rabbitmq.prefetch_count }}
{{- end }}
- {{- if .Values.broker.rabbitmq.prefetch_size }}
+ {{- if hasKey .Values.broker.rabbitmq "prefetch_size" }}
prefetch_size: {{ .Values.broker.rabbitmq.prefetch_size }}
{{- end }}
- {{- if .Values.broker.googlepubsub.max_outstanding_messages }}
+ {{- if hasKey .Values.broker.googlepubsub "max_outstanding_messages" }}
max_outstanding_messages: {{ .Values.broker.googlepubsub.max_outstanding_messages }}
{{- end }}
- {{- if .Values.broker.googlepubsub.num_goroutines }}
+ {{- if hasKey .Values.broker.googlepubsub "num_goroutines" }}
num_goroutines: {{ .Values.broker.googlepubsub.num_goroutines }}
{{- end }}📝 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.
| {{- if .Values.broker.rabbitmq.prefetch_count }} | |
| prefetch_count: {{ .Values.broker.rabbitmq.prefetch_count }} | |
| {{- end }} | |
| {{- if .Values.broker.rabbitmq.prefetchSize }} | |
| prefetch_size: {{ .Values.broker.rabbitmq.prefetchSize }} | |
| {{- if .Values.broker.rabbitmq.prefetch_size }} | |
| prefetch_size: {{ .Values.broker.rabbitmq.prefetch_size }} | |
| {{- end }} | |
| {{- if hasKey .Values.broker.rabbitmq "prefetch_count" }} | |
| prefetch_count: {{ .Values.broker.rabbitmq.prefetch_count }} | |
| {{- end }} | |
| {{- if hasKey .Values.broker.rabbitmq "prefetch_size" }} | |
| prefetch_size: {{ .Values.broker.rabbitmq.prefetch_size }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/templates/configmap.yaml` around lines 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.
| hyperfleet_api: | ||
| endpoint: http://hyperfleet-api.hyperfleet-system.svc.cluster.local:8080 | ||
| base_url: http://hyperfleet-api.hyperfleet-system.svc.cluster.local:8080 |
There was a problem hiding this comment.
This troubleshooting example still misses the new clients nesting.
Changing endpoint to base_url is only half of the migration. As written, this YAML still uses the pre-migration shape and will fail to load.
Suggested doc fix
- hyperfleet_api:
- base_url: http://hyperfleet-api.hyperfleet-system.svc.cluster.local:8080
+ clients:
+ hyperfleet_api:
+ base_url: http://hyperfleet-api.hyperfleet-system.svc.cluster.local:8080📝 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.
| hyperfleet_api: | |
| endpoint: http://hyperfleet-api.hyperfleet-system.svc.cluster.local:8080 | |
| base_url: http://hyperfleet-api.hyperfleet-system.svc.cluster.local:8080 | |
| clients: | |
| hyperfleet_api: | |
| base_url: http://hyperfleet-api.hyperfleet-system.svc.cluster.local:8080 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/running-sentinel.md` around lines 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).
| func NewHyperFleetClient(endpoint string, timeout time.Duration, sentinelName, version string) (*HyperFleetClient, error) { | ||
| httpClient := &http.Client{ | ||
| Timeout: timeout, | ||
| } | ||
|
|
||
| client, err := openapi.NewClientWithResponses(endpoint, openapi.WithHTTPClient(httpClient)) | ||
| userAgent := fmt.Sprintf("hyperfleet-sentinel/%s (%s)", version, sentinelName) | ||
|
|
||
| client, err := openapi.NewClientWithResponses(endpoint, | ||
| openapi.WithHTTPClient(httpClient), | ||
| openapi.WithRequestEditorFn(func(_ context.Context, req *http.Request) error { | ||
| req.Header.Set("User-Agent", userAgent) | ||
| return nil | ||
| }), |
There was a problem hiding this comment.
default_headers config is currently a no-op in the HTTP client
NewHyperFleetClient only sets User-Agent, while the config/docs surface clients.hyperfleet_api.default_headers as request-wide headers. This mismatch breaks expected behavior for deployments requiring custom headers. Please wire configurable default headers into the request editor (alongside User-Agent) and pass them from the config layer. As per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Suggested fix (constructor wiring)
-func NewHyperFleetClient(endpoint string, timeout time.Duration, sentinelName, version string) (*HyperFleetClient, error) {
+func NewHyperFleetClient(
+ endpoint string,
+ timeout time.Duration,
+ sentinelName, version string,
+ defaultHeaders map[string]string,
+) (*HyperFleetClient, error) {
httpClient := &http.Client{
Timeout: timeout,
}
userAgent := fmt.Sprintf("hyperfleet-sentinel/%s (%s)", version, sentinelName)
client, err := openapi.NewClientWithResponses(endpoint,
openapi.WithHTTPClient(httpClient),
openapi.WithRequestEditorFn(func(_ context.Context, req *http.Request) error {
req.Header.Set("User-Agent", userAgent)
+ for k, v := range defaultHeaders {
+ req.Header.Set(k, v)
+ }
return nil
}),
)📝 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.
| func NewHyperFleetClient(endpoint string, timeout time.Duration, sentinelName, version string) (*HyperFleetClient, error) { | |
| httpClient := &http.Client{ | |
| Timeout: timeout, | |
| } | |
| client, err := openapi.NewClientWithResponses(endpoint, openapi.WithHTTPClient(httpClient)) | |
| userAgent := fmt.Sprintf("hyperfleet-sentinel/%s (%s)", version, sentinelName) | |
| client, err := openapi.NewClientWithResponses(endpoint, | |
| openapi.WithHTTPClient(httpClient), | |
| openapi.WithRequestEditorFn(func(_ context.Context, req *http.Request) error { | |
| req.Header.Set("User-Agent", userAgent) | |
| return nil | |
| }), | |
| func NewHyperFleetClient( | |
| endpoint string, | |
| timeout time.Duration, | |
| sentinelName, version string, | |
| defaultHeaders map[string]string, | |
| ) (*HyperFleetClient, error) { | |
| httpClient := &http.Client{ | |
| Timeout: timeout, | |
| } | |
| userAgent := fmt.Sprintf("hyperfleet-sentinel/%s (%s)", version, sentinelName) | |
| client, err := openapi.NewClientWithResponses(endpoint, | |
| openapi.WithHTTPClient(httpClient), | |
| openapi.WithRequestEditorFn(func(_ context.Context, req *http.Request) error { | |
| req.Header.Set("User-Agent", userAgent) | |
| for k, v := range defaultHeaders { | |
| req.Header.Set(k, v) | |
| } | |
| return nil | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/client/client.go` around lines 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.
|
|
||
| // ClientsConfig contains all client configurations | ||
| type ClientsConfig struct { | ||
| HyperfleetAPI *HyperFleetAPIConfig `yaml:"hyperfleet_api" mapstructure:"hyperfleet_api"` |
There was a problem hiding this comment.
Category: Inconsistency
Minor naming inconsistency: the field is HyperfleetAPI (lowercase "fleet") but the type is
HyperFleetAPIConfig (uppercase "Fleet"):
| HyperfleetAPI *HyperFleetAPIConfig `yaml:"hyperfleet_api" mapstructure:"hyperfleet_api"` | |
| HyperFleetAPI *HyperFleetAPIConfig `yaml:"hyperfleet_api" mapstructure:"hyperfleet_api"` |
3 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