HYPERFLEET-1104 - feat: adapt chart to expose rabbitmq#160
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR introduces explicit broker type configuration for the Helm chart. The Sequence Diagram(s)sequenceDiagram
participant HelmCLI as Helm CLI
participant ConfigMap as configmap-broker.yaml
participant Helpers as charts/templates/_helpers.tpl
HelmCLI->>ConfigMap: helm template (render ConfigMap)
ConfigMap->>Helpers: include hyperfleet-adapter.validateBrokerConfig
Helpers->>Helpers: hyperfleet-adapter.brokerType -> read .Values.broker.type
alt broker.type == "rabbitmq"
Helpers->>Helpers: check broker.rabbitmq.url & broker.rabbitmq.queue
Helpers-->>ConfigMap: fail() if missing
else other broker types
Helpers-->>ConfigMap: return without fail
end
ConfigMap-->>HelmCLI: rendered output or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/templates/_helpers.tpl`:
- Around line 287-299: The helper "hyperfleet-adapter.brokerType" currently
infers the broker from sub-keys; change it to require an explicit
.Values.broker.type instead. Replace the conditional inference logic in the
"hyperfleet-adapter.brokerType" template with a single return of
.Values.broker.type and use Helm's required helper (e.g., required "broker.type
must be set" .Values.broker.type) to fail the render if it's unset; remove the
else-if branches that reference .Values.broker.googlepubsub.projectId and
.Values.broker.rabbitmq.url.
In `@charts/templates/configmap-broker.yaml`:
- Line 7: The template currently calls the validation helper unconditionally via
include "hyperfleet-adapter.validateBrokerConfig" ., which causes field-level
checks to run even when the user provided a raw broker.yaml; change this to run
only when structured broker fields are being used (e.g., when
.Values.broker.yaml is empty or not set and/or when .Values.broker.rabbitmq is
present). Modify the template to wrap the include call in a conditional that
checks for absence of .Values.broker.yaml (and optionally presence of
.Values.broker.rabbitmq) so validation only executes when generating broker.yaml
from structured values.
In `@charts/values.yaml`:
- Around line 76-79: Update the comments in charts/values.yaml to state that
broker.type is required and must be explicitly set (e.g., "googlepubsub" or
"rabbitmq") instead of being inferred; modify the existing comment block that
references inference (the lines around the broker.type example) to clearly
declare broker.type as mandatory, remove any guidance about inferring from
populated sections, and add a short note to validate the chosen value against
HyperFleet architecture standards. Ensure the symbol referenced is broker.type
and keep the example value format (type: "googlepubsub" | "rabbitmq") for
clarity.
In `@Makefile`:
- Around line 225-243: Add a new negative Helm test block in the Makefile that
invokes "helm template test-release charts/" with --set broker.create=true and
--set broker.type=rabbitmq but intentionally omit --set broker.rabbitmq.queue
(or set it empty), then assert that the command fails (non-zero) to exercise the
validation path; use the same invocation pattern as the existing positive test
(the helm template call with --set image.* and adapterConfig.*) and check the
exit status (expect failure) rather than grepping for 'type: rabbitmq' so the CI
will catch missing RabbitMQ queue validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 65442820-5112-4bc9-ae25-bc960e4dcecd
📒 Files selected for processing (4)
Makefilecharts/templates/_helpers.tplcharts/templates/configmap-broker.yamlcharts/values.yaml
rh-amarin
force-pushed
the
adapt-to-rabbit
branch
from
f684385 to
5f6739d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/templates/deployment.yaml`:
- Line 94: The template currently sets $brokerType with default "" which
bypasses the chart's broker.type validation; replace the default-empty
assignment and instead obtain the value via the existing brokerType helper so
the helper's validation/requirements run (use the helper to set $brokerType and
then use $brokerType in the pod selector/template), and ensure the helper
enforces/returns a non-empty value per the HyperFleet architecture standards
referenced.
In `@Makefile`:
- Around line 269-280: The negative RabbitMQ validation block ending with "||
true" masks failures; replace the current chained command with an explicit shell
conditional: run the helm template command (the block starting with the echo
"Testing that rabbitmq without queue fails validation..." and the helm template
invocation with --set broker.type=rabbitmq and without broker.rabbitmq.queue),
and if it exits 0 then print the error message and exit 1 (e.g. use if helm
template ... >/dev/null 2>&1; then echo "ERROR: expected helm template to fail
when broker.rabbitmq.queue is missing"; exit 1; fi) so the test fails when the
template unexpectedly succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: aca5aede-3003-44a5-97fd-7808e1f37e78
📒 Files selected for processing (5)
Makefilecharts/templates/_helpers.tplcharts/templates/configmap-broker.yamlcharts/templates/deployment.yamlcharts/values.yaml
rh-amarin
force-pushed
the
adapt-to-rabbit
branch
from
5f6739d to
569d502
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Makefile (1)
269-280:⚠️ Potential issue | 🔴 Critical | ⚖️ Poor tradeoffNegative RabbitMQ validation test is currently non-failing due to shell logic.
Lines 269-280 always end in success because
|| truemasks the intentional validation failure. This means the negative test won't catch regressions when the broker validation logic breaks.The current logic:
- If
helm templatefails (expected):&&doesn't run,|| trueruns → make succeeds without validating the failure- If
helm templatesucceeds (bug):&&branch runsexit 1, but|| truemasks it → make still succeedsProposed fix
- `@helm` template test-release charts/ \ - --set image.registry=quay.io \ - --set image.repository=openshift-hyperfleet/hyperfleet-adapter \ - --set image.tag=test \ - --set adapterConfig.yaml="apiVersion: hyperfleet.redhat.com/v1alpha1" \ - --set adapterTaskConfig.yaml="apiVersion: hyperfleet.redhat.com/v1alpha1" \ - --set broker.create=true \ - --set broker.type=rabbitmq \ - --set broker.rabbitmq.url=amqp://guest:guest@rabbitmq:5672/ > /dev/null 2>&1 \ - && { echo "ERROR: expected helm template to fail when broker.rabbitmq.queue is missing"; exit 1; } \ - || true + `@if` helm template test-release charts/ \ + --set image.registry=quay.io \ + --set image.repository=openshift-hyperfleet/hyperfleet-adapter \ + --set image.tag=test \ + --set adapterConfig.yaml="apiVersion: hyperfleet.redhat.com/v1alpha1" \ + --set adapterTaskConfig.yaml="apiVersion: hyperfleet.redhat.com/v1alpha1" \ + --set broker.create=true \ + --set broker.type=rabbitmq \ + --set broker.rabbitmq.url=amqp://guest:guest@rabbitmq:5672/ > /dev/null 2>&1; then \ + echo "ERROR: expected helm template to fail when broker.rabbitmq.queue is missing"; \ + exit 1; \ + fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 269 - 280, The negative RabbitMQ validation currently always succeeds because the trailing "|| true" masks failures; replace this shell logic so the rule explicitly fails when "helm template ..." succeeds. Locate the block containing the helm template invocation and the error message "ERROR: expected helm template to fail when broker.rabbitmq.queue is missing" and remove the "|| true" fallback, turning the check into an explicit if/then: run the helm template command (silencing output), and if it returns success then print the ERROR message and exit 1, otherwise continue (no extra "|| true" at the end).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/templates/_helpers.tpl`:
- Around line 291-292: The helper currently only checks presence of
.Values.broker.type but not allowed values; update the brokerType helper to
validate that .Values.broker.type is one of the allowed list (googlepubsub,
rabbitmq) and emit a clear required-style error if not; locate the brokerType
template helper and replace the single required call with logic that first
ensures the value exists (required) then checks membership against the whitelist
and calls required (or fail) with a message like "broker.type must be one of:
googlepubsub, rabbitmq" when validation fails so all callers of brokerType
inherit the whitelist enforcement.
---
Duplicate comments:
In `@Makefile`:
- Around line 269-280: The negative RabbitMQ validation currently always
succeeds because the trailing "|| true" masks failures; replace this shell logic
so the rule explicitly fails when "helm template ..." succeeds. Locate the block
containing the helm template invocation and the error message "ERROR: expected
helm template to fail when broker.rabbitmq.queue is missing" and remove the "||
true" fallback, turning the check into an explicit if/then: run the helm
template command (silencing output), and if it returns success then print the
ERROR message and exit 1, otherwise continue (no extra "|| true" at the end).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 6f3dbed5-dcd4-41db-bfae-3f6231077805
📒 Files selected for processing (4)
Makefilecharts/templates/_helpers.tplcharts/templates/configmap-broker.yamlcharts/values.yaml
| # option3: Broker type identifier (googlepubsub, rabbitmq, etc.) - used as label | ||
| # Subscription/topic are used to set adapter broker config overrides | ||
| # option3: required broker type — must be set to "googlepubsub" or "rabbitmq". | ||
| # type: "googlepubsub" |
There was a problem hiding this comment.
The Helm chart conventions standard §11 defines broker.type as an active key. With it commented out, users won't see it in helm show values and will hit a render-time error instead of discovering the field they need to set.
| # type: "googlepubsub" | |
| type: "" |
There was a problem hiding this comment.
Good point
I changed it to be:
type: "CHANGEME"
So, it should be obvious that it has to be set
rh-amarin
force-pushed
the
adapt-to-rabbit
branch
from
569d502 to
ec8919d
Compare
Risk Score: 0 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 163 lines | +0 |
| Sensitive paths | none | +0 |
Computed by hyperfleet-risk-scorer
There was a problem hiding this comment.
♻️ Duplicate comments (1)
charts/templates/_helpers.tpl (1)
291-292:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCentralize broker type whitelist validation in
brokerTypehelper.The helper still only checks that
broker.typeis set but does not validate it against the allowed list. Withbroker.typedefaulting to"CHANGEME"in values.yaml (line 77), therequiredcheck passes but the value is invalid until whitelist validation runs inconfigmap-broker.yaml. This creates inconsistent behavior if the helper is reused in other templates.Proposed fix to centralize whitelist validation
{{- define "hyperfleet-adapter.brokerType" -}} -{{- required "broker.type must be set to one of: googlepubsub, rabbitmq" .Values.broker.type -}} +{{- $brokerType := required "broker.type must be set to one of: googlepubsub, rabbitmq" .Values.broker.type -}} +{{- $allowedTypes := list "googlepubsub" "rabbitmq" -}} +{{- if not (has $brokerType $allowedTypes) -}} +{{- fail (printf "broker.type must be one of %s (got %q)" (join ", " $allowedTypes) $brokerType) -}} +{{- end -}} +{{- $brokerType -}} {{- end }}This allows removal of lines 2-6 in
configmap-broker.yaml.As per coding guidelines, "Validate changes against HyperFleet architecture standards from the linked architecture repository."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/templates/_helpers.tpl` around lines 291 - 292, Update the brokerType helper to enforce a whitelist instead of only requiring existence: inside the brokerType helper, read .Values.broker.type, assert it is set and that its value is one of the allowed options (googlepubsub, rabbitmq, etc.), and return or render the value; replace the current plain required check there so callers get a validated broker type (this lets you remove the redundant whitelist logic in configmap-broker.yaml and prevents the default "CHANGEME" from being treated as valid). Use the brokerType helper name to locate the helper and update callers (e.g., configmap-broker.yaml) to rely on it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@charts/templates/_helpers.tpl`:
- Around line 291-292: Update the brokerType helper to enforce a whitelist
instead of only requiring existence: inside the brokerType helper, read
.Values.broker.type, assert it is set and that its value is one of the allowed
options (googlepubsub, rabbitmq, etc.), and return or render the value; replace
the current plain required check there so callers get a validated broker type
(this lets you remove the redundant whitelist logic in configmap-broker.yaml and
prevents the default "CHANGEME" from being treated as valid). Use the brokerType
helper name to locate the helper and update callers (e.g.,
configmap-broker.yaml) to rely on it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a14794a2-217b-41c9-8cd9-d3d0788e6227
📒 Files selected for processing (4)
Makefilecharts/templates/_helpers.tplcharts/templates/configmap-broker.yamlcharts/values.yaml
| @echo "Testing template with RabbitMQ broker..." | ||
| helm template test-release charts/ \ | ||
| --set image.registry=quay.io \ | ||
| --set image.repository=openshift-hyperfleet/hyperfleet-adapter \ |
There was a problem hiding this comment.
nit: This test only checks exit code while the "both sections" test below asserts on rendered content with grep -q 'type: rabbitmq'. Consider applying the same pattern here — it would catch rendering regressions (e.g., broken field names) that a success-only check would miss.
@output=$$(helm template test-release charts/ \
--set image.registry=quay.io \
--set image.repository=openshift-hyperfleet/hyperfleet-adapter \
--set image.tag=test \
--set adapterConfig.yaml="apiVersion: hyperfleet.redhat.com/v1alpha1" \
--set adapterTaskConfig.yaml="apiVersion: hyperfleet.redhat.com/v1alpha1" \
--set broker.create=true \
--set broker.type=rabbitmq \
--set broker.rabbitmq.url=amqp://guest:guest@rabbitmq:5672/ \
--set broker.rabbitmq.queue=test-queue \
--set broker.rabbitmq.exchange=test-exchange \
--set broker.rabbitmq.routingKey=test-key) \
&& echo "$$output" | grep -q 'type: rabbitmq' \
|| { echo "ERROR: expected rabbitmq broker type in output"; exit 1; }
rh-amarin
force-pushed
the
adapt-to-rabbit
branch
from
ec8919d to
d1c06a1
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/values.yaml (1)
76-92:⚠️ Potential issue | 🟠 MajorBump Helm chart version (and add NOTES guidance) for broker config behavior change
charts/Chart.yamlwas not version-bumped (no changes toversion:in this PR) despite the user-facingbroker.type+ RabbitMQ values behavior update.- There is no
NOTES.txtincharts/(or anywhere in the repo) to update with guidance—either addcharts/templates/NOTES.txtor document the behavioral change in the chart’s existing user-facing docs, alongside the version bump.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/values.yaml` around lines 76 - 92, Bump the chart version in Chart.yaml (update the version: field, and appVersion: if applicable) to reflect the user-facing change to the broker.type / RabbitMQ behavior, and add a user-facing NOTES file (create charts/templates/NOTES.txt) that documents the new broker.type semantics and the RabbitMQ defaults/expectations (referencing broker.type and the rabbitmq.* values from values.yaml), so users see guidance on upgrade and configuration when installing/upgrading the chart.
♻️ Duplicate comments (1)
charts/templates/_helpers.tpl (1)
290-292: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winCentralize broker-type whitelist validation inside
brokerType.
brokerTypeonly enforces presence, while allowed-values are enforced elsewhere. This splits the contract and can produce inconsistent behavior for other helper call sites. Validategooglepubsub|rabbitmqdirectly here.As per coding guidelines, "Validate changes against HyperFleet architecture standards from the linked architecture repository."Proposed fix
{{- define "hyperfleet-adapter.brokerType" -}} -{{- required "broker.type must be set to one of: googlepubsub, rabbitmq" .Values.broker.type -}} +{{- $brokerType := required "broker.type must be set to one of: googlepubsub, rabbitmq" .Values.broker.type -}} +{{- $allowedTypes := list "googlepubsub" "rabbitmq" -}} +{{- if not (has $brokerType $allowedTypes) -}} +{{- fail (printf "broker.type must be one of %s (got %q)" (join ", " $allowedTypes) $brokerType) -}} +{{- end -}} +{{- $brokerType -}} {{- end }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/templates/_helpers.tpl` around lines 290 - 292, The helper "hyperfleet-adapter.brokerType" currently only checks presence; update it to also validate allowed values by changing its required check to enforce the whitelist "googlepubsub|rabbitmq" (use a regex validation in the required call referencing .Values.broker.type) so all call sites rely on this single centralized validation in the define "hyperfleet-adapter.brokerType"; ensure the error message clearly lists the allowed values and matches HyperFleet architecture validation conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@charts/values.yaml`:
- Around line 76-92: Bump the chart version in Chart.yaml (update the version:
field, and appVersion: if applicable) to reflect the user-facing change to the
broker.type / RabbitMQ behavior, and add a user-facing NOTES file (create
charts/templates/NOTES.txt) that documents the new broker.type semantics and the
RabbitMQ defaults/expectations (referencing broker.type and the rabbitmq.*
values from values.yaml), so users see guidance on upgrade and configuration
when installing/upgrading the chart.
---
Duplicate comments:
In `@charts/templates/_helpers.tpl`:
- Around line 290-292: The helper "hyperfleet-adapter.brokerType" currently only
checks presence; update it to also validate allowed values by changing its
required check to enforce the whitelist "googlepubsub|rabbitmq" (use a regex
validation in the required call referencing .Values.broker.type) so all call
sites rely on this single centralized validation in the define
"hyperfleet-adapter.brokerType"; ensure the error message clearly lists the
allowed values and matches HyperFleet architecture validation conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 6ba97ab9-6d82-4c61-9201-c57aebc78cc8
📒 Files selected for processing (4)
Makefilecharts/templates/_helpers.tplcharts/templates/configmap-broker.yamlcharts/values.yaml
rh-amarin
force-pushed
the
adapt-to-rabbit
branch
from
d1c06a1 to
373918e
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pnguyen44 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 0e2e9f2
into
openshift-hyperfleet:main
2 participants
Summary
Allows to set rabbitmq as broker for adapters using helm.
Without this changes, the E2E environment can not be deployed with rabbitmq support and make use of completely local environments
With this in place you can deploy a completely local environment :
This:
make install-rabbitmq NAMESPACE=rabbitmq