HYPERFLEET-682 - fix: improve adapter configs and make Terraform directory configurable#16
Merged
rh-amarin merged 2 commits intoopenshift-hyperfleet:mainfrom Feb 27, 2026
Merged
Conversation
WalkthroughMakefile: TF_DIR assignment changed from unconditional ( Sequence Diagram(s)sequenceDiagram
participant Client
participant Adapter
participant Discovery
participant K8sAPI
participant StatusAggregator
Client->>Adapter: submit task / manifest
Adapter->>Discovery: request byName ({{ .clusterId }}-{{ .metadata.name }})
Discovery->>K8sAPI: lookup ManifestWork, Namespace, ConfigMap
K8sAPI-->>Discovery: return resources or not found
Discovery-->>Adapter: nestedDiscovery results (manifestwork, namespace0, configmap0)
Adapter->>StatusAggregator: compute Applied/Available/Health using adapter.executionStatus, errorReason, errorMessage, and fallbacks
StatusAggregator-->>Client: return aggregated status and messages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
xueli181114
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
helm/adapter1/adapter-task-config.yaml (1)
128-134:Healthfallback is internally inconsistent for unknown execution state.When status resolves to
"Unknown"(Line 128), reason/message still default to"Healthy"and a success message (Lines 131, 134). This can misreport non-terminal states.Proposed CEL adjustment
- adapter.?errorReason.orValue("") != "" ? adapter.?errorReason.orValue("") : "Healthy" + adapter.?executionStatus.orValue("") == "success" + ? "Healthy" + : (adapter.?executionStatus.orValue("") == "failed" + ? (adapter.?errorReason.orValue("") != "" ? adapter.?errorReason.orValue("") : "ExecutionFailed") + : "ExecutionStatusUnknown") - adapter.?errorMessage.orValue("") != "" ? adapter.?errorMessage.orValue("") : "All adapter operations completed successfully" + adapter.?executionStatus.orValue("") == "success" + ? "All adapter operations completed successfully" + : (adapter.?executionStatus.orValue("") == "failed" + ? (adapter.?errorMessage.orValue("") != "" ? adapter.?errorMessage.orValue("") : "Adapter execution failed") + : "Adapter execution status is not yet available")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/adapter1/adapter-task-config.yaml` around lines 128 - 134, The current CEL expressions use adapter.?executionStatus.orValue("") to set condition to "Unknown" but still default reason/message to "Healthy" and a success message; update the reason and message expressions (the adapter.?errorReason.orValue("") and adapter.?errorMessage.orValue("") branches) to first check executionStatus and return an "Unknown" (or appropriate non-healthy) reason/message when executionStatus is neither "success" nor "failed" — otherwise keep the existing behavior: if executionStatus == "success" use errorReason/orValue("Healthy") and errorMessage/orValue("All adapter operations completed successfully"), if executionStatus == "failed" use errorReason/orValue("") and errorMessage/orValue(""), and if executionStatus is unknown return a clear "Unknown execution state" (or similar) for both reason and message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/adapter2/adapter-task-config.yaml`:
- Line 88: The template uses the unavailable runtime field .metadata.name in the
resource name (name: "{{ .clusterId }}-{{ .metadata.name }}") which will cause
failures; replace uses of {{ .metadata.name }} in adapter2 with the same
hardcoded adapter identifier pattern used by adapter1/adapter3 (e.g., a literal
adapter name with a TODO referencing config.adapter.metadata.name) across all
occurrences so the template compiles at runtime, and ensure the TODO comment
remains to indicate future replacement once config.adapter.metadata.name is
available.
---
Nitpick comments:
In `@helm/adapter1/adapter-task-config.yaml`:
- Around line 128-134: The current CEL expressions use
adapter.?executionStatus.orValue("") to set condition to "Unknown" but still
default reason/message to "Healthy" and a success message; update the reason and
message expressions (the adapter.?errorReason.orValue("") and
adapter.?errorMessage.orValue("") branches) to first check executionStatus and
return an "Unknown" (or appropriate non-healthy) reason/message when
executionStatus is neither "success" nor "failed" — otherwise keep the existing
behavior: if executionStatus == "success" use errorReason/orValue("Healthy") and
errorMessage/orValue("All adapter operations completed successfully"), if
executionStatus == "failed" use errorReason/orValue("") and
errorMessage/orValue(""), and if executionStatus is unknown return a clear
"Unknown execution state" (or similar) for both reason and message.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Makefilehelm/adapter1/adapter-task-config.yamlhelm/adapter2/adapter-task-config.yaml
xueli181114
commented
Feb 26, 2026
- Fix adapter1 field references (clusterId -> cluster_id) and Health condition logic - Fix adapter2 hardcoded values, condition expressions, and data extraction - Correct adapter2 label from landing-zone to maestro type - Make Terraform directory configurable in Makefile Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/adapter2/adapter-task-config.yaml`:
- Around line 256-283: The CEL expressions for the Available checks use
has(c.type) inconsistently, which can throw if a condition lacks type; update
all condition existence predicates used in
resources.namespace0.conditions.exists(...) and
resources.configmap0.conditions.exists(...) so they consistently guard c.type
(i.e., add has(c.type) && before every c.type == "Available" occurrence inside
the exists(...) lambdas for namespace0 and configmap0 conditions) to ensure
null-safe evaluation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Makefilehelm/adapter1/adapter-task-config.yamlhelm/adapter2/adapter-task-config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
rafabene
reviewed
Feb 26, 2026
Contributor
|
Now that all {{ .adapterName }} references have been replaced with {{ .metadata.name }}, the |
- Remove redundant adapterName parameter definition - Fix inconsistent null-safety checks in Available condition expressions - Add missing has(c.type) guards to configmap0 condition checks for consistency with namespace0 Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Contributor
Author
Removed |
rh-amarin
reviewed
Feb 26, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helm/adapter2/adapter-task-config.yaml (1)
7-7:⚠️ Potential issue | 🟠 MajorAdapter type label is still
maestroinstead oflanding-zone.Line 7 still sets
hyperfleet.io/adapter-type: maestro, which conflicts with the PR objective and can break selector/routing logic expectinglanding-zone.Suggested fix
- hyperfleet.io/adapter-type: maestro + hyperfleet.io/adapter-type: landing-zone🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/adapter2/adapter-task-config.yaml` at line 7, The label key hyperfleet.io/adapter-type currently has the value "maestro"; update this to "landing-zone" so selectors and routing match the PR intent—specifically change the value for hyperfleet.io/adapter-type in adapter-task-config.yaml from "maestro" to "landing-zone" and verify related selectors/helm templates and any tests that reference hyperfleet.io/adapter-type are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/adapter2/adapter-task-config.yaml`:
- Line 82: The ManifestWork resource's name uses clusterId without lowercasing
("name: \"{{ .clusterId }}-{{ .metadata.name }}\"") which can violate DNS-1123;
update the ManifestWork name templates (both occurrences currently at the
ManifestWork declarations) to apply the | lower filter to .clusterId so the
generated names are lowercase and DNS-1123 compliant, mirroring how Namespace
and ConfigMap templates use | lower.
---
Outside diff comments:
In `@helm/adapter2/adapter-task-config.yaml`:
- Line 7: The label key hyperfleet.io/adapter-type currently has the value
"maestro"; update this to "landing-zone" so selectors and routing match the PR
intent—specifically change the value for hyperfleet.io/adapter-type in
adapter-task-config.yaml from "maestro" to "landing-zone" and verify related
selectors/helm templates and any tests that reference hyperfleet.io/adapter-type
are updated accordingly.
| metadata: | ||
| # ManifestWork name - must be unique within consumer namespace | ||
| name: "{{ .clusterId }}-adapter2" | ||
| name: "{{ .clusterId }}-{{ .metadata.name }}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, check if the file exists and display lines around 82 and 216
echo "=== Checking helm/adapter2/adapter-task-config.yaml ==="
if [ -f "helm/adapter2/adapter-task-config.yaml" ]; then
echo "File exists. Lines 80-84:"
sed -n '80,84p' "helm/adapter2/adapter-task-config.yaml" | cat -n
echo ""
echo "Lines 214-218:"
sed -n '214,218p' "helm/adapter2/adapter-task-config.yaml" | cat -n
else
echo "File not found"
fi
echo ""
echo "=== Searching for clusterId and naming patterns ==="
# Look for clusterId and event.id references
rg -n 'clusterId|event\.id' helm/ --type yaml --type go -A 2 -B 2
echo ""
echo "=== Checking ManifestWork naming patterns ==="
# Look for ManifestWork resources and their naming conventions
rg -n 'kind:\s*ManifestWork' helm/ --type yaml -A 10 | head -60Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 11765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for event.id or event ID validation ==="
rg -n 'event\.id|eventId|event_id|validation|sanitize|normalize|DNS|dns1123' helm/ --type yaml --type go --type md -B 2 -A 2 | head -100
echo ""
echo "=== Checking README or docs for naming requirements ==="
find . -maxdepth 2 -name "README*" -o -name "*.md" | xargs rg -l 'ManifestWork|event.id|clusterId|naming|DNS' 2>/dev/null | head -5
if [ -f "README.md" ]; then
echo "Searching README.md for relevant info:"
rg -n 'ManifestWork|event.id|clusterId|naming' README.md -B 1 -A 1
fi
echo ""
echo "=== Checking if there's any preprocessing or validation of clusterId ==="
rg -n 'clusterId|event\.id' --type go -B 3 -A 3 | head -80Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 2060
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Getting full validation check expressions ==="
sed -n '60,75p' helm/adapter2/adapter-task-config.yaml
echo ""
echo "=== Searching for Go code that validates or uses event.id ==="
find . -name "*.go" -type f | head -10
rg -n 'event.*id|EventID|Event.*ID' --type go -B 2 -A 5 2>/dev/null | head -60
echo ""
echo "=== Checking comments or documentation about ManifestWork ==="
rg -n 'ManifestWork|manifestwork' helm/adapter2/adapter-task-config.yaml -B 1 -A 5 | head -40
echo ""
echo "=== Checking if there's documentation about naming constraints ==="
find . -name "*.md" -o -name "*.txt" | xargs grep -l "ManifestWork\|Kubernetes.*name\|DNS" 2>/dev/null | head -5Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 763
🌐 Web query:
ManifestWork Kubernetes naming requirements DNS-1123
💡 Result:
For an OCM ManifestWork (a normal Kubernetes object), the key DNS‑1123 constraints are:
-
metadata.name: typically must be a DNS subdomain (RFC 1123) (Kubernetes “object name” rule):- max 253 characters
- only lowercase
a-z,0-9,-,. - must start and end with an alphanumeric character
- common regex:
^[a-z0-9]([a-z0-9-.]{0,251}[a-z0-9])?$[1]
-
metadata.namespace(the cluster namespace where you create theManifestWork, i.e., the target managed-cluster namespace on the hub): must be a DNS label (RFC 1123):- max 63 characters
- only lowercase
a-z,0-9,- - must start and end with an alphanumeric character
- common regex:
^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?$[1][2]
Sources: Kubernetes naming rules [1] and OCM ManifestWork concept docs (created in the managed cluster’s namespace) [2].
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for example event IDs or cluster IDs with uppercase ==="
rg -n 'event.id|clusterId' helm/ --type yaml | grep -i '[A-Z]' | head -20
echo ""
echo "=== Looking for any test data or examples ==="
find . -name "*test*" -o -name "*example*" -o -name "*fixture*" 2>/dev/null | xargs grep -l 'clusterId\|event.id' 2>/dev/null | head -5
echo ""
echo "=== Checking if adapter1 has ManifestWork (for comparison) ==="
rg -n 'kind:\s*ManifestWork' helm/adapter1/ -A 10 | head -30
echo ""
echo "=== Verifying the actual inconsistency one more time ==="
echo "adapter2 line 82 (ManifestWork name):"
sed -n '82p' helm/adapter2/adapter-task-config.yaml
echo ""
echo "adapter2 line 140 (Namespace name):"
sed -n '140p' helm/adapter2/adapter-task-config.yaml
echo ""
echo "adapter2 line 155 (ConfigMap name):"
sed -n '155p' helm/adapter2/adapter-task-config.yamlRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 2002
Apply | lower filter to clusterId in ManifestWork naming to ensure DNS-1123 compliance.
ManifestWork names (lines 82 and 216) must follow Kubernetes DNS-1123 rules requiring lowercase alphanumerics and hyphens only. Since clusterId is sourced from event.id without validation, uppercase characters would cause ManifestWork creation to fail. Other Kubernetes resources in this file (Namespace at line 140, ConfigMap at line 155) already correctly apply the | lower filter; this should be consistent for ManifestWork as well.
Suggested fix
- name: "{{ .clusterId }}-{{ .metadata.name }}"
+ name: "{{ .clusterId | lower }}-{{ .metadata.name }}"- byName: "{{ .clusterId }}-{{ .metadata.name }}"
+ byName: "{{ .clusterId | lower }}-{{ .metadata.name }}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@helm/adapter2/adapter-task-config.yaml` at line 82, The ManifestWork
resource's name uses clusterId without lowercasing ("name: \"{{ .clusterId }}-{{
.metadata.name }}\"") which can violate DNS-1123; update the ManifestWork name
templates (both occurrences currently at the ManifestWork declarations) to apply
the | lower filter to .clusterId so the generated names are lowercase and
DNS-1123 compliant, mirroring how Namespace and ConfigMap templates use | lower.
rh-amarin
reviewed
Feb 26, 2026
Contributor
|
/lgtm |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes Made
Makefile
TF_DIR := terraformtoTF_DIR ?= terraformto make it configurableadapter1 Configuration
clusterId→cluster_idadapter.errorReasonandadapter.errorMessageadapter2 Configuration
maestrotolanding-zone{{ .metadata.name }}{{ .clusterId }}-{{ .metadata.name }}Test plan
Summary by CodeRabbit
Chores
Refactor
Bug Fixes