Add more info to adapter framework task execution#51
Merged
openshift-merge-bot[bot] merged 1 commit intoopenshift-hyperfleet:mainfrom Feb 19, 2026
Merged
Conversation
rh-amarin
changed the title
WalkthroughAdds CEL helper functions ( Sequence Diagram(s)sequenceDiagram
participant Executor as ResourceExecutor
participant APICall as ApplyAPI
participant Discovery as DiscoveryTransport
participant Manifest as ManifestEnrichment
participant Storage as ExecutionContext.Resources
Executor->>APICall: Apply resource
APICall-->>Executor: Apply result
Executor->>Discovery: Discover resources after apply
alt Discovery Failure
Discovery-->>Executor: error
Executor->>Executor: mark result failed\nattach error to Result\nset ExecutionError
Executor-->>Executor: return executor error
else Discovery Success
Discovery-->>Executor: parent + nestedResults
Executor->>Storage: store parent resource (by resource name)
loop for each nested in nestedResults
Executor->>Manifest: EnrichWithResourceStatus(parent, nested)
Manifest-->>Executor: enriched nested resource
Executor->>Storage: store enriched nested (by nested discovery name)
Executor->>Executor: warn on key collisions if any
end
Executor-->>Executor: return success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/executor/utils.go (1)
135-175:⚠️ Potential issue | 🟠 MajorDebug logging of raw request payloads can leak sensitive data.
Request bodies often contain credentials or PII (tokens, passwords, secrets are present in the codebase); logging them verbatim at debug level risks exposing secrets in logs. No redaction helpers exist in the codebase. Consider logging only payload size instead of content.
🛡️ Safer logging (size‑only)
- log.Debugf(ctx, "API call payload: %s %s payload=%s", apiCall.Method, url, string(body)) + log.Debugf(ctx, "API call payload: %s %s payload_bytes=%d", apiCall.Method, url, len(body)) @@ - log.Debugf(ctx, "API call payload: %s %s payload=%s", apiCall.Method, url, string(body)) + log.Debugf(ctx, "API call payload: %s %s payload_bytes=%d", apiCall.Method, url, len(body)) @@ - log.Debugf(ctx, "API call payload: %s %s payload=%s", apiCall.Method, url, string(body)) + log.Debugf(ctx, "API call payload: %s %s payload_bytes=%d", apiCall.Method, url, len(body))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/executor/utils.go` around lines 135 - 175, The debug logs in the HTTP handlers (the log.Debugf calls in the POST/PUT/PATCH cases) and the POST error logging are printing raw request bodies which may contain secrets; change these to avoid logging payload content by logging only the payload size (len(body)) or a redacted placeholder instead. Specifically, update the log.Debugf invocations around apiClient.Post/Put/Patch and the error handling that uses logger.WithErrorField so they do not include string(body) — use the byte length or a constant like "<redacted>" and keep renderTemplateBytes/use of apiCall.Body/rendered body unchanged except for logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/testdata/dryrun/dryrun-cel-showcase-task-config.yaml`:
- Around line 308-310: The CEL expression for observed_generation references a
non-existent variable generationSpec; update the expression under
observed_generation (the expression field) to reference the actual parameter
name generationValue used in this config so CEL evaluates correctly (ensure the
expression string is changed from "generationSpec" to "generationValue").
In `@test/testdata/dryrun/dryrun-kubernetes-task-config.yaml`:
- Around line 184-186: The CEL expression for observed_generation uses an
undefined identifier "generationSpec"; update the expression to reference the
defined parameter name "generationValue" instead (i.e., change the expression on
observed_generation to use generationValue). Locate the observed_generation
block in the YAML (the key observed_generation and its expression) and replace
"generationSpec" with "generationValue" so CEL evaluation uses the correct
parameter.
In `@test/testdata/dryrun/dryrun-maestro-adapter-config.yaml`:
- Around line 26-28: The comment above the httpServerAddress entry is incorrect:
it says “HTTPS server address” while the value is http://localhost:8100; update
the sample so they match by either changing the comment to “HTTP server address
for REST API operations (optional)” or changing the value to use
https://localhost:8100, and ensure the httpServerAddress key and the Environment
variable reference (HYPERFLEET_MAESTRO_HTTP_SERVER_ADDRESS) remain consistent.
---
Outside diff comments:
In `@internal/executor/utils.go`:
- Around line 135-175: The debug logs in the HTTP handlers (the log.Debugf calls
in the POST/PUT/PATCH cases) and the POST error logging are printing raw request
bodies which may contain secrets; change these to avoid logging payload content
by logging only the payload size (len(body)) or a redacted placeholder instead.
Specifically, update the log.Debugf invocations around apiClient.Post/Put/Patch
and the error handling that uses logger.WithErrorField so they do not include
string(body) — use the byte length or a constant like "<redacted>" and keep
renderTemplateBytes/use of apiCall.Body/rendered body unchanged except for
logging.
Comment on lines
+184
to
+186
| observed_generation: | ||
| expression: "generationSpec" | ||
| observed_time: "{{ now | date \"2006-01-02T15:04:05Z07:00\" }}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, search for generationSpec and generationValue across the repository
rg -n "generationSpec" -S
rg -n "generationValue" -SRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 2554
🏁 Script executed:
# Read the specific section of the file around lines 184-186
sed -n '20,30p' test/testdata/dryrun/dryrun-kubernetes-task-config.yaml
sed -n '180,190p' test/testdata/dryrun/dryrun-kubernetes-task-config.yamlRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 729
observed_generation references undefined generationSpec.
The parameter is named generationValue (defined at line 23); the CEL expression at line 185 should reference generationValue instead. Using the undefined generationSpec will cause CEL evaluation to fail or produce empty output.
🛠️ Proposed fix
observed_generation:
- expression: "generationSpec"
+ expression: "generationValue"📝 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.
Suggested change
| observed_generation: | |
| expression: "generationSpec" | |
| observed_time: "{{ now | date \"2006-01-02T15:04:05Z07:00\" }}" | |
| observed_generation: | |
| expression: "generationValue" | |
| observed_time: "{{ now | date \"2006-01-02T15:04:05Z07:00\" }}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/testdata/dryrun/dryrun-kubernetes-task-config.yaml` around lines 184 -
186, The CEL expression for observed_generation uses an undefined identifier
"generationSpec"; update the expression to reference the defined parameter name
"generationValue" instead (i.e., change the expression on observed_generation to
use generationValue). Locate the observed_generation block in the YAML (the key
observed_generation and its expression) and replace "generationSpec" with
"generationValue" so CEL evaluation uses the correct parameter.
Comment on lines
+26
to
+28
| # HTTPS server address for REST API operations (optional) | ||
| # Environment variable: HYPERFLEET_MAESTRO_HTTP_SERVER_ADDRESS | ||
| httpServerAddress: "http://localhost:8100" |
There was a problem hiding this comment.
Align the comment with the actual scheme (HTTP vs HTTPS).
Line 26 says “HTTPS server address” but Line 28 uses http://…. This is a minor doc mismatch that can mislead readers of the sample config.
✏️ Suggested fix
- # HTTPS server address for REST API operations (optional)
+ # HTTP/HTTPS server address for REST API operations (optional)
# Environment variable: HYPERFLEET_MAESTRO_HTTP_SERVER_ADDRESS
httpServerAddress: "http://localhost:8100"📝 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.
Suggested change
| # HTTPS server address for REST API operations (optional) | |
| # Environment variable: HYPERFLEET_MAESTRO_HTTP_SERVER_ADDRESS | |
| httpServerAddress: "http://localhost:8100" | |
| # HTTP/HTTPS server address for REST API operations (optional) | |
| # Environment variable: HYPERFLEET_MAESTRO_HTTP_SERVER_ADDRESS | |
| httpServerAddress: "http://localhost:8100" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/testdata/dryrun/dryrun-maestro-adapter-config.yaml` around lines 26 -
28, The comment above the httpServerAddress entry is incorrect: it says “HTTPS
server address” while the value is http://localhost:8100; update the sample so
they match by either changing the comment to “HTTP server address for REST API
operations (optional)” or changing the value to use https://localhost:8100, and
ensure the httpServerAddress key and the Environment variable reference
(HYPERFLEET_MAESTRO_HTTP_SERVER_ADDRESS) remain consistent.
rafabene
reviewed
Feb 19, 2026
| @@ -1,4 +1,9 @@ | |||
| # Invalid task configuration for testing validate command error reporting | |||
Contributor
There was a problem hiding this comment.
Typo in the renamed filename: adatepr should be adapter.
rafabene
reviewed
Feb 19, 2026
rh-amarin
force-pushed
the
devx-enhancements
branch
from
1c1d8c5 to
393ce9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/executor/resource_executor_test.go (1)
417-421: Consider using safer type assertions in test verification.The chained type assertions (
sf["values"].([]interface{}),values[0].(map[string]interface{})) will panic if the structure differs from expectations. While this may be acceptable in tests (a panic still fails the test), usingrequireassertions would provide clearer failure messages.Safer assertion approach
- sf := nested.Object["statusFeedback"].(map[string]interface{}) - values := sf["values"].([]interface{}) - assert.Len(t, values, 1) - v0 := values[0].(map[string]interface{}) - assert.Equal(t, "data", v0["name"]) + sf, ok := nested.Object["statusFeedback"].(map[string]interface{}) + require.True(t, ok, "statusFeedback should be a map") + values, ok := sf["values"].([]interface{}) + require.True(t, ok, "statusFeedback.values should be a slice") + require.Len(t, values, 1) + v0, ok := values[0].(map[string]interface{}) + require.True(t, ok, "first value should be a map") + assert.Equal(t, "data", v0["name"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/executor/resource_executor_test.go` around lines 417 - 421, Change the direct chained type assertions to guarded checks using the comma-ok pattern and require helpers so failures produce clear test errors: verify that nested.Object["statusFeedback"] is present and is a map before assigning to sf, use the comma-ok style to extract sf["values"] and require.True/require.NotNil (and require.Len) instead of direct .([]interface{}), then check the first element exists and is a map with another comma-ok and require assertions before accessing v0["name"]; reference the variables sf, values and v0 and replace bare casts with these guarded checks (and require.Len(t, values, 1)) so type mismatches yield readable test failures rather than panics.internal/executor/utils.go (1)
143-143: Debug logging of request payloads is useful, but consider sensitive data exposure.The payload may contain credentials, tokens, or PII rendered from templates. While debug-level logging is typically disabled in production, ensure log configurations enforce this to prevent accidental exposure in non-production environments with elevated log levels.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/executor/utils.go` at line 143, The current debug log call log.Debugf(ctx, "API call payload: %s %s payload=%s", apiCall.Method, url, string(body)) may leak sensitive data; change it to redact or sanitize the request payload before logging (e.g., implement and call a helper like redactSensitivePayload(body) or maskKnownFields such as tokens/credentials/PII) and ensure the log is emitted only when debug is truly enabled (check the logger's debug level or use an IsDebugEnabled(ctx) guard). Update the logging call to use the sanitized payload and keep references to apiCall.Method and url intact so the context remains useful for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/testdata/dryrun/dryrun-maestro-adapter-task-config.yaml`:
- Around line 143-148: The manifest name/namespace and the resourceIdentifier
disagree when clusterId can contain uppercase letters: resourceIdentifier uses
"{{ .clusterId | lower }}" but the ConfigMap manifest uses "{{ .clusterId }}";
update them to be consistent—either remove the | lower from resourceIdentifier
or (recommended) make the manifest use the same lowercasing transformation by
changing the ConfigMap's name and namespace to "{{ .clusterId | lower }}" so the
manifest entries and resourceIdentifier (name/namespace) match exactly.
- Around line 209-211: The nested discovery for configmap0 uses byName: "{{
.clusterId }}-config-symbol2222" which is inconsistent with the
resourceIdentifier that lowercases clusterId; update the byName template in the
discovery block for "configmap0" to also apply | lower to .clusterId (i.e., use
the same lowercase transformation as resourceIdentifier) so discovery remains
consistent when clusterId contains uppercase characters.
---
Duplicate comments:
In `@test/testdata/dryrun/dryrun-cel-showcase-task-config.yaml`:
- Around line 307-310: The CEL expression for observed_generation incorrectly
references a non-existent symbol generationSpec; update the expression under
observed_generation (the expression field) to reference the defined parameter
generationValue instead (e.g., replace generationSpec with generationValue) so
CEL evaluation uses the correct parameter name; ensure any other references in
the same YAML to generationSpec are similarly updated to generationValue to
maintain consistency.
In `@test/testdata/dryrun/dryrun-kubernetes-task-config.yaml`:
- Around line 184-186: The CEL expression for observed_generation references a
non-existent symbol generationSpec; update the expression to reference the
defined parameter generationValue (i.e., change the expression on
observed_generation from "generationSpec" to "generationValue") so CEL
evaluation uses the correct identifier (look for observed_generation and the
parameter definition generationValue).
---
Nitpick comments:
In `@internal/executor/resource_executor_test.go`:
- Around line 417-421: Change the direct chained type assertions to guarded
checks using the comma-ok pattern and require helpers so failures produce clear
test errors: verify that nested.Object["statusFeedback"] is present and is a map
before assigning to sf, use the comma-ok style to extract sf["values"] and
require.True/require.NotNil (and require.Len) instead of direct
.([]interface{}), then check the first element exists and is a map with another
comma-ok and require assertions before accessing v0["name"]; reference the
variables sf, values and v0 and replace bare casts with these guarded checks
(and require.Len(t, values, 1)) so type mismatches yield readable test failures
rather than panics.
In `@internal/executor/utils.go`:
- Line 143: The current debug log call log.Debugf(ctx, "API call payload: %s %s
payload=%s", apiCall.Method, url, string(body)) may leak sensitive data; change
it to redact or sanitize the request payload before logging (e.g., implement and
call a helper like redactSensitivePayload(body) or maskKnownFields such as
tokens/credentials/PII) and ensure the log is emitted only when debug is truly
enabled (check the logger's debug level or use an IsDebugEnabled(ctx) guard).
Update the logging call to use the sanitized payload and keep references to
apiCall.Method and url intact so the context remains useful for debugging.
Comment on lines
+143
to
+148
| name: "{{ .clusterId }}-config-symbol2222" | ||
| namespace: "{{ .clusterId }}" | ||
| labels: | ||
| hyperfleet.io/cluster-id: "{{ .clusterId }}" | ||
| annotations: | ||
| hyperfleet.io/generation: "{{ .generationValue }}" | ||
| hyperfleet.io/generation: "{{ .generation }}" |
There was a problem hiding this comment.
ConfigMap manifest name/namespace may not match resourceIdentifier.
The ConfigMap manifest (lines 143-144) uses:
name: "{{ .clusterId }}-config-symbol2222"namespace: "{{ .clusterId }}"
But the resourceIdentifier (lines 187-188) uses:
namespace: "{{ .clusterId | lower }}"name: "{{ .clusterId | lower }}-config-symbol2222"
If clusterId contains uppercase characters, the manifest and resourceIdentifier will produce different values, causing the manifestConfig to not match the actual resource.
🛠️ Proposed fix
- apiVersion: v1
kind: ConfigMap
metadata:
- name: "{{ .clusterId }}-config-symbol2222"
- namespace: "{{ .clusterId }}"
+ name: "{{ .clusterId | lower }}-config-symbol2222"
+ namespace: "{{ .clusterId | lower }}"
labels:📝 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.
Suggested change
| name: "{{ .clusterId }}-config-symbol2222" | |
| namespace: "{{ .clusterId }}" | |
| labels: | |
| hyperfleet.io/cluster-id: "{{ .clusterId }}" | |
| annotations: | |
| hyperfleet.io/generation: "{{ .generationValue }}" | |
| hyperfleet.io/generation: "{{ .generation }}" | |
| name: "{{ .clusterId | lower }}-config-symbol2222" | |
| namespace: "{{ .clusterId | lower }}" | |
| labels: | |
| hyperfleet.io/cluster-id: "{{ .clusterId }}" | |
| annotations: | |
| hyperfleet.io/generation: "{{ .generation }}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/testdata/dryrun/dryrun-maestro-adapter-task-config.yaml` around lines
143 - 148, The manifest name/namespace and the resourceIdentifier disagree when
clusterId can contain uppercase letters: resourceIdentifier uses "{{ .clusterId
| lower }}" but the ConfigMap manifest uses "{{ .clusterId }}"; update them to
be consistent—either remove the | lower from resourceIdentifier or (recommended)
make the manifest use the same lowercasing transformation by changing the
ConfigMap's name and namespace to "{{ .clusterId | lower }}" so the manifest
entries and resourceIdentifier (name/namespace) match exactly.
Comment on lines
+209
to
+211
| - name: "configmap0" | ||
| discovery: | ||
| byName: "{{ .clusterId }}-symbol222" | ||
| byName: "{{ .clusterId }}-config-symbol2222" |
There was a problem hiding this comment.
Discovery byName should also use | lower for consistency.
The nested discovery at line 211 uses byName: "{{ .clusterId }}-config-symbol2222" without | lower, but the resourceIdentifier uses | lower. This inconsistency could cause discovery to fail if clusterId contains uppercase characters.
🛠️ Proposed fix
- name: "configmap0"
discovery:
- byName: "{{ .clusterId }}-config-symbol2222"
+ byName: "{{ .clusterId | lower }}-config-symbol2222"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/testdata/dryrun/dryrun-maestro-adapter-task-config.yaml` around lines
209 - 211, The nested discovery for configmap0 uses byName: "{{ .clusterId
}}-config-symbol2222" which is inconsistent with the resourceIdentifier that
lowercases clusterId; update the byName template in the discovery block for
"configmap0" to also apply | lower to .clusterId (i.e., use the same lowercase
transformation as resourceIdentifier) so discovery remains consistent when
clusterId contains uppercase characters.
ciaranRoche
approved these changes
Feb 19, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ciaranRoche 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 1ce63b5
into
openshift-hyperfleet:main
7 checks passed
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.
https://issues.redhat.com/browse/HYPERFLEET-653
Improvements to adapter framework
toJson,dig): Add helper functions to CEL expressions for safely traversing deeply nested resource structures and serializing objects to JSONtoJson: converts object to JSON string, useful for debugging adaptersdig: fetches a nested object with dot notation of returns falsemanifest: the previous functionalitystatusFeedback: to look for remote object statusconditions: maestro conditions on the resourcesExecutionError, mark the result asStatusFailed, and abort execution instead of silently logging a warningSummary by CodeRabbit
New Features
Improvements
Tests