HYPERFLEET-52 | feat: Implement executor for adapter #13
Conversation
WalkthroughAdds a complete CloudEvent executor subsystem (internal/executor) implementing a four‑phase pipeline: parameter extraction, precondition evaluation, Kubernetes resource management, and post‑actions. Introduces Executor, builder/handler integration, executors (PreconditionExecutor, ResourceExecutor, PostActionExecutor), param extraction from env/event/secret/configmap, CEL enhancements, rich error types, k8s client interface and data extractors, mock clients, expanded config_loader types (Parameter, Payload, LogAction), and extensive unit and integration tests including k8s and mock API server harnesses. Sequence Diagram(s)sequenceDiagram
participant Client
participant Executor
participant ParamExtractor
participant PreconditionExecutor
participant ResourceExecutor
participant PostActionExecutor
participant APIClient
participant K8sClient
participant Logger
Client->>Executor: Execute(ctx, event)
activate Executor
Executor->>ParamExtractor: extractConfigParams(execCtx)
activate ParamExtractor
ParamExtractor->>K8sClient: ExtractFromSecret/ConfigMap (when source=secret/configmap)
K8sClient-->>ParamExtractor: secret/configmap value
ParamExtractor-->>Executor: params populated
deactivate ParamExtractor
Executor->>PreconditionExecutor: ExecuteAll(preconditions, execCtx)
activate PreconditionExecutor
PreconditionExecutor->>APIClient: ExecuteAPICall (optional)
APIClient-->>PreconditionExecutor: response
PreconditionExecutor->>Logger: ExecuteLogAction (optional)
PreconditionExecutor-->>Executor: precondition results
deactivate PreconditionExecutor
alt Preconditions Met
Executor->>ResourceExecutor: ExecuteAll(resources, execCtx)
activate ResourceExecutor
ResourceExecutor->>K8sClient: Discover/Get/Create/Update/Delete
K8sClient-->>ResourceExecutor: resource state / errors
ResourceExecutor->>Logger: log resource ops
ResourceExecutor-->>Executor: resource results
deactivate ResourceExecutor
else Preconditions Not Met
Executor->>Executor: mark resources skipped
end
Executor->>PostActionExecutor: ExecuteAll(postConfig, execCtx)
activate PostActionExecutor
PostActionExecutor->>Executor: buildPostPayloads (CEL + templates)
PostActionExecutor->>APIClient: ExecuteAPICall (post-actions)
APIClient-->>PostActionExecutor: response
PostActionExecutor->>Logger: ExecuteLogAction (optional)
PostActionExecutor-->>Executor: post-action results
deactivate PostActionExecutor
Executor->>Logger: aggregate results & finalize
Executor-->>Client: ExecutionResult
deactivate Executor
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 6
🧹 Nitpick comments (23)
internal/executor/README.md (1)
25-35: Add language identifier to fenced code block.The ASCII diagram should have a language identifier for the fenced code block. Use
textorplaintextto satisfy markdown linting rules.-``` +```text ┌──────────────────────────────────────────────────────────────────────┐ │ Event Processing Pipeline │internal/config_loader/types.go (2)
49-68: Consider addingValidate()method toParameterfor consistency.The
Parameterstruct now hasBuildandBuildReffields with similar mutual exclusivity semantics asPayload(as described in the comment on lines 50-52). However, unlikePayload,Parameterlacks aValidate()method to enforce this constraint.For consistency and to catch configuration errors early:
// Validate checks that the Parameter configuration is valid. func (p *Parameter) Validate() error { hasBuild := p.Build != nil hasBuildRef := p.BuildRef != "" hasSource := p.Source != "" // Source-based parameters vs Build-based parameters if hasSource && (hasBuild || hasBuildRef) { return fmt.Errorf("source and build/buildRef are mutually exclusive") } if hasBuild && hasBuildRef { return fmt.Errorf("build and buildRef are mutually exclusive") } return nil }
216-220: Consider validatingLogAction.Levelvalues.The comment documents valid levels (
debug, info, warning, error) with a default ofinfo, but there's no validation to catch typos like"warn"or"err".// Validate checks that the LogAction configuration is valid. func (l *LogAction) Validate() error { if l.Level != "" { switch l.Level { case "debug", "info", "warning", "error": // valid default: return fmt.Errorf("invalid log level %q; must be one of: debug, info, warning, error", l.Level) } } return nil }internal/k8s_client/client.go (1)
208-215: Minor:Resourcefield is redundant withKindfor list operations.For
ListResources, bothResourceandKindare set togvk.Kind. Consider using an empty string or a more descriptive value forResourcesince list operations don't target a specific resource by name.return nil, &K8sOperationError{ Operation: "list", - Resource: gvk.Kind, + Resource: "", // list operations don't target a specific resource Kind: gvk.Kind, Namespace: namespace,internal/executor/executor_test.go (1)
802-873: Consider expanding post-action tests to cover API call scenarios.Currently only tests log-type post actions. The test infrastructure with
mockResponseandmockErroris in place but unused. Consider adding test cases for API call post actions to improve coverage.internal/k8s_client/error.go (1)
37-59: Consider usingerrors.Asfor idiomatic error unwrapping.The manual unwrap loop works correctly, but Go's
errors.Asprovides the same functionality more concisely and is the idiomatic approach:func IsK8sOperationError(err error) (*K8sOperationError, bool) { if err == nil { return nil, false } - // Try direct type assertion first - if k8sErr, ok := err.(*K8sOperationError); ok { - return k8sErr, true - } - // Then try unwrapping - for err != nil { - if k8sErr, ok := err.(*K8sOperationError); ok { - return k8sErr, true - } - if unwrapper, ok := err.(interface{ Unwrap() error }); ok { - err = unwrapper.Unwrap() - } else { - break - } - } - return nil, false + var k8sErr *K8sOperationError + if errors.As(err, &k8sErr) { + return k8sErr, true + } + return nil, false }This would also require adding
"errors"to the imports.test/integration/executor/main_test.go (2)
163-164: Silent error handling may mask unexpected failures.While ignoring "already exists" errors is expected, the current code ignores all errors. Consider checking specifically for the "already exists" case:
- _, _ = client.CreateResource(ctx, defaultNS) // Ignore error if already exists + _, err = client.CreateResource(ctx, defaultNS) + if err != nil { + // Only ignore "already exists" errors + if k8sErr, ok := k8s_client.IsK8sOperationError(err); !ok || k8sErr.Operation != "create" { + println(" ⚠️ Warning: failed to create default namespace:", err.Error()) + } + }Alternatively, you could use a Kubernetes API error check for
IsAlreadyExists.
177-199: Consider optimizing the polling loop to avoid repeated client/logger creation.The function creates a new logger and attempts to create a new client on each iteration. For efficiency, create the logger once outside the loop:
func waitForAPIServerReady(config *rest.Config, timeout time.Duration) error { deadline := time.Now().Add(timeout) ctx := context.Background() log := logger.NewLogger(ctx) + var lastErr error for time.Now().Before(deadline) { - // Try to create a client client, err := k8s_client.NewClientFromConfig(ctx, config, log) if err == nil { - // Try to list namespaces to verify API server is responsive gvk := schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Namespace"} - _, err = client.ListResources(ctx, gvk, "", "") - if err == nil { - return nil // API server is ready + if _, err = client.ListResources(ctx, gvk, "", ""); err == nil { + return nil } } - + lastErr = err time.Sleep(500 * time.Millisecond) } - return fmt.Errorf("timeout waiting for API server to be ready") + return fmt.Errorf("timeout waiting for API server to be ready: %w", lastErr) }This also wraps the last error for better debugging.
internal/executor/param_extractor.go (1)
140-154: TODO stubs are appropriate for incremental development.Clear "not yet implemented" errors prevent silent failures. Consider tracking these TODOs in an issue if not already done.
Would you like me to open an issue to track the implementation of secret and configmap parameter extraction?
internal/executor/precondition_executor.go (2)
227-231: Silent timeout parse failure in precondition executor.Unlike the post-action executor (lines 326-332 of
post_action_executor.go) which logs when timeout parsing fails, this code silently ignores invalid timeout values. This could lead to confusing behavior.// Set timeout if specified if apiCall.Timeout != "" { timeout, err := time.ParseDuration(apiCall.Timeout) - if err == nil { + if err != nil { + pe.log.Warning(fmt.Sprintf("Failed to parse timeout '%s': %v, using default", apiCall.Timeout, err)) + } else { opts = append(opts, hyperfleet_api.WithRequestTimeout(timeout)) } }
248-256: Duplicate body rendering logic across HTTP methods.The body rendering pattern is repeated for POST, PUT, and PATCH methods (lines 248-274). Consider extracting a helper function to reduce duplication.
// renderBody renders the body template if non-empty func renderBody(body string, params map[string]interface{}) ([]byte, error) { if body == "" { return []byte{}, nil } return renderTemplateBytes(body, params) }Then use:
case http.MethodPost: - body := []byte(apiCall.Body) - if apiCall.Body != "" { - body, err = renderTemplateBytes(apiCall.Body, execCtx.Params) - if err != nil { - return nil, fmt.Errorf("failed to render body template: %w", err) - } + body, err := renderBody(apiCall.Body, execCtx.Params) + if err != nil { + return nil, fmt.Errorf("failed to render body template: %w", err) } resp, err = pe.apiClient.Post(ctx, url, body, opts...)test/integration/executor/executor_k8s_integration_test.go (3)
64-68: Request body reading may be truncated for large payloads.The fixed 1MB buffer may truncate large request bodies, and the error from
r.Body.Readis silently ignored. This could lead to incomplete request capture in tests.var bodyStr string if r.Body != nil { - buf := make([]byte, 1024*1024) - n, _ := r.Body.Read(buf) - bodyStr = string(buf[:n]) + bodyBytes, err := io.ReadAll(r.Body) + if err != nil { + t.Logf("Warning: failed to read request body: %v", err) + } + bodyStr = string(bodyBytes) }Don't forget to add
"io"to imports.
133-134: Error fromjson.Marshalis silently ignored.While unlikely to fail for simple map data, ignoring the error could mask issues during test development.
- eventDataBytes, _ := json.Marshal(eventData) - _ = evt.SetData(event.ApplicationJSON, eventDataBytes) + eventDataBytes, err := json.Marshal(eventData) + if err != nil { + // This should never happen with simple map data + panic(fmt.Sprintf("failed to marshal test event data: %v", err)) + } + if err := evt.SetData(event.ApplicationJSON, eventDataBytes); err != nil { + panic(fmt.Sprintf("failed to set test event data: %v", err)) + }
778-811: Test assertions may be flaky due to conditional logic.The test uses
if len(statusResponses) == 1to conditionally assert, which means the test could silently pass without validating the error payload if no status response is received. Consider usingrequire.Lento fail fast.- if len(statusResponses) == 1 { - status := statusResponses[0] + require.Len(t, statusResponses, 1, "Should have 1 status response for K8s error") + status := statusResponses[0] - // ... assertions ... - } else { - t.Logf("Note: Expected status response for K8s error, got %d responses", len(statusResponses)) - } + // ... assertions ...This ensures the test actually validates the error reporting behavior.
internal/executor/post_action_executor.go (2)
279-290: Non-success status check after error check may be redundant.If
err != nilis returned fromexecuteAPICall, the response might already indicate failure. The separate non-success check (lines 279-290) creates a second error path that could be consolidated.The current flow has two places that can mark the action as failed:
- Lines 272-277: when
executeAPICallreturns an error- Lines 279-290: when response is not successful
Consider consolidating by having
executeAPICallreturn an error for non-2xx responses, similar to howPreconditionExecutor.executeAPICallwraps non-success asAPIError(lines 285-298 inprecondition_executor.go).
349-380: Duplicate body rendering logic with precondition executor.The HTTP method switch with body rendering (lines 349-380) is nearly identical to
precondition_executor.golines 243-279. Consider extracting a shared utility function.Create a shared helper in
utils.go:// ExecuteAPIRequest handles the common API call pattern for all executors func ExecuteAPIRequest(ctx context.Context, apiClient hyperfleet_api.Client, method, url string, body []byte, opts []hyperfleet_api.RequestOption) (*hyperfleet_api.Response, error) { switch strings.ToUpper(method) { case http.MethodGet: return apiClient.Get(ctx, url, opts...) case http.MethodPost: return apiClient.Post(ctx, url, body, opts...) // ... other methods default: return nil, fmt.Errorf("unsupported HTTP method: %s", method) } }internal/executor/resource_executor.go (3)
335-364:waitForDeletionhas unbounded polling with no upper limit.While the context provides a timeout, if a large context timeout is passed (e.g., 60s), this will poll at 100ms intervals (600 iterations). Consider adding a maximum poll count or explicit timeout.
func (re *ResourceExecutor) waitForDeletion(ctx context.Context, gvk schema.GroupVersionKind, namespace, name string) error { const pollInterval = 100 * time.Millisecond + const maxWaitTime = 30 * time.Second + // Create a sub-context with maximum wait time + waitCtx, cancel := context.WithTimeout(ctx, maxWaitTime) + defer cancel() + ticker := time.NewTicker(pollInterval) defer ticker.Stop() for { select { - case <-ctx.Done(): + case <-waitCtx.Done(): re.log.Warning(fmt.Sprintf("Context cancelled/timed out while waiting for deletion of %s/%s", gvk.Kind, name)) - return fmt.Errorf("context cancelled while waiting for resource deletion: %w", ctx.Err()) + return fmt.Errorf("timed out waiting for resource deletion: %w", waitCtx.Err())
356-358: Inconsistent log indentation.Line 357 has inconsistent indentation compared to surrounding code (appears to be a tab/space issue).
if apierrors.IsNotFound(err) { re.log.Infof("Resource deletion confirmed: %s/%s", gvk.Kind, name) return nil } // Any other error is unexpected - re.log.Error(fmt.Sprintf("Error checking resource deletion status for %s/%s: %v", gvk.Kind, name, err)) + re.log.Error(fmt.Sprintf("Error checking resource deletion status for %s/%s: %v", gvk.Kind, name, err)) return fmt.Errorf("error checking deletion status: %w", err)
636-652: String-based network error detection is fragile.Checking error messages for patterns like "connection refused" is locale-dependent and may not work across different Go versions or operating systems.
The type-based checks above (lines 617-634) are more robust. Consider removing or minimizing reliance on string matching:
- // Check error message for common network error patterns - errMsg := strings.ToLower(err.Error()) - networkPatterns := []string{ - "connection refused", - "connection reset", - "connection timed out", - "no such host", - "network is unreachable", - "i/o timeout", - "eof", - "broken pipe", - } - for _, pattern := range networkPatterns { - if strings.Contains(errMsg, pattern) { - return true - } - } + // The type-based checks above should catch most network errors. + // If additional patterns are needed, they should be added carefully + // as string matching is fragile across platforms/locales.If string matching is truly necessary, add a comment explaining why the type-based checks are insufficient.
test/integration/executor/executor_integration_test.go (2)
92-96: Body reading may truncate large payloads and doesn't close the body.The current approach reads up to 1MB but doesn't handle:
- Bodies larger than 1MB (silently truncated)
r.Bodyshould be closed after readingFor test purposes this is likely fine, but consider using
io.ReadAllfor correctness:- var bodyStr string - if r.Body != nil { - buf := make([]byte, 1024*1024) - n, _ := r.Body.Read(buf) - bodyStr = string(buf[:n]) - } + var bodyStr string + if r.Body != nil { + defer r.Body.Close() + buf, _ := io.ReadAll(r.Body) + bodyStr = string(buf) + }
893-897: Context cancellation test lacks concrete assertions.The test cancels the context immediately but only logs the result without verifying expected behavior. Consider adding assertions:
result := exec.Execute(ctx, evt) // Should fail due to context cancellation - // Note: The exact behavior depends on where cancellation is checked - t.Logf("Result with cancelled context: status=%s, error=%v", result.Status, result.Error) + // Depending on implementation, should either fail or succeed quickly + if result.Status == executor.StatusFailed { + assert.Error(t, result.Error, "Should have error when context is cancelled") + // Optionally check for context.Canceled in the error chain + } + t.Logf("Result with cancelled context: status=%s, error=%v", result.Status, result.Error) }internal/executor/executor.go (1)
244-247: Comment references non-existentStatusSkipped.Line 245 mentions "StatusSkipped" but looking at
types.go, onlyStatusSuccessandStatusFailedare defined. The comment should referenceResourcesSkippedor be updated to match the actual implementation.- // StatusSkipped is not an error - preconditions not met is expected behavior + // Precondition not met (ResourcesSkipped=true) is not an error - it's expected behavior return nilinternal/executor/types.go (1)
387-391:IsPreconditionNotMetdoesn't handle wrapped errors.Using direct type assertion won't work if the error is wrapped (e.g., with
fmt.Errorf("...: %w", err)). Consider usingerrors.Asfor proper error chain traversal.+import "errors" + // IsPreconditionNotMet checks if an error is a PreconditionNotMetError func IsPreconditionNotMet(err error) bool { - _, ok := err.(*PreconditionNotMetError) - return ok + var target *PreconditionNotMetError + return errors.As(err, &target) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
internal/config_loader/types.go(3 hunks)internal/executor/README.md(1 hunks)internal/executor/executor.go(1 hunks)internal/executor/executor_test.go(1 hunks)internal/executor/param_extractor.go(1 hunks)internal/executor/post_action_executor.go(1 hunks)internal/executor/precondition_executor.go(1 hunks)internal/executor/resource_executor.go(1 hunks)internal/executor/types.go(1 hunks)internal/executor/utils.go(1 hunks)internal/k8s_client/client.go(6 hunks)internal/k8s_client/error.go(1 hunks)pkg/logger/logger.go(3 hunks)pkg/logger/logger_test.go(2 hunks)test/integration/executor/executor_integration_test.go(1 hunks)test/integration/executor/executor_k8s_integration_test.go(1 hunks)test/integration/executor/main_test.go(1 hunks)test/integration/executor/setup_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
internal/executor/utils.go (8)
internal/config_loader/types.go (3)
Condition(141-145)LogAction(217-220)Resource(181-188)internal/criteria/evaluator.go (1)
ConditionDef(291-295)internal/criteria/types.go (1)
Operator(8-8)internal/executor/types.go (2)
ExecutionContext(182-197)ExecutionError(247-266)pkg/logger/logger.go (1)
Logger(11-19)internal/executor/executor.go (1)
New(16-33)internal/config_loader/loader.go (1)
Parse(114-130)pkg/errors/api_error.go (1)
APIError(16-33)
test/integration/executor/executor_integration_test.go (4)
pkg/logger/logger.go (1)
Logger(11-19)internal/criteria/types.go (1)
Operator(8-8)internal/executor/types.go (7)
StatusSuccess(43-43)ExecutionContext(182-197)ExecutionStatus(39-39)ExecutionError(247-266)StatusFailed(45-45)PhaseParamExtraction(23-23)PhasePostActions(29-29)internal/criteria/cel_evaluator.go (1)
CELResult(24-39)
test/integration/executor/executor_k8s_integration_test.go (1)
internal/executor/types.go (6)
StatusSuccess(43-43)OperationCreate(148-148)OperationUpdate(150-150)OperationRecreate(152-152)StatusFailed(45-45)PhasePostActions(29-29)
pkg/logger/logger_test.go (1)
pkg/logger/logger.go (1)
EvtIDKey(167-167)
test/integration/executor/setup_test.go (2)
internal/k8s_client/client.go (1)
Client(20-23)pkg/logger/logger.go (1)
Logger(11-19)
internal/k8s_client/client.go (1)
internal/k8s_client/error.go (1)
K8sOperationError(7-20)
internal/executor/executor_test.go (5)
internal/executor/types.go (11)
NewExecutionContext(269-281)ExecutionStatus(39-39)StatusSuccess(43-43)StatusFailed(45-45)PhasePreconditions(25-25)PhasePostActions(29-29)PhaseResources(27-27)NewExecutorError(367-374)PreconditionNotMetError(377-381)IsPreconditionNotMet(388-391)ExecutionContext(182-197)internal/executor/executor.go (1)
New(16-33)internal/criteria/evaluator.go (1)
EvaluationResult(12-23)internal/criteria/types.go (3)
Operator(8-8)OperatorEquals(12-12)OperatorGreaterThan(22-22)internal/executor/param_extractor.go (1)
NewParamExtractor(19-24)
test/integration/executor/main_test.go (3)
test/integration/executor/setup_test.go (1)
K8sTestEnv(15-21)test/integration/testutil/container.go (1)
ContainerConfig(20-51)internal/k8s_client/client.go (2)
NewClientFromConfig(105-115)Client(20-23)
internal/executor/executor.go (8)
internal/executor/types.go (16)
ExecutorConfig(49-60)Executor(63-65)NewExecutorError(367-374)PhaseParamExtraction(23-23)ExecutionResult(68-95)StatusFailed(45-45)StatusSuccess(43-43)NewExecutionContext(269-281)ExecutionContext(182-197)PhasePreconditions(25-25)IsPreconditionNotMet(388-391)PhaseResources(27-27)PhasePostActions(29-29)PreconditionResult(98-119)ResourceResult(122-141)PostActionResult(160-179)internal/config_loader/types.go (1)
AdapterConfig(6-11)pkg/logger/logger.go (2)
Logger(11-19)EvtIDKey(167-167)internal/executor/param_extractor.go (1)
NewParamExtractor(19-24)internal/executor/precondition_executor.go (1)
NewPreconditionExecutor(25-30)internal/executor/resource_executor.go (1)
NewResourceExecutor(31-37)internal/executor/post_action_executor.go (1)
NewPostActionExecutor(25-30)internal/k8s_client/client.go (1)
Client(20-23)
internal/executor/types.go (7)
internal/config_loader/types.go (2)
AdapterConfig(6-11)Resource(181-188)internal/k8s_client/client.go (1)
Client(20-23)pkg/logger/logger.go (1)
Logger(11-19)internal/criteria/evaluator.go (1)
EvaluationResult(12-23)internal/criteria/cel_evaluator.go (1)
CELResult(24-39)pkg/errors/api_error.go (1)
APIError(16-33)internal/k8s_client/error.go (1)
K8sOperationError(7-20)
internal/executor/param_extractor.go (2)
internal/config_loader/types.go (3)
AdapterConfig(6-11)Parameter(53-68)Metadata(14-18)internal/executor/types.go (3)
ExecutionContext(182-197)NewExecutorError(367-374)PhaseParamExtraction(23-23)
🪛 markdownlint-cli2 (0.18.1)
internal/executor/README.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (48)
internal/executor/README.md (1)
1-446: Comprehensive and well-structured documentation.The README provides excellent coverage of the executor package including the four-phase execution pipeline, execution status vs business outcomes distinction, configuration examples, and error handling semantics. The examples are clear and the collapsible sections keep the document navigable.
pkg/logger/logger.go (2)
99-116: Good fix for shared state inV(), butExtra()still mutates in place.The
copyExtrahelper and its use inV()correctly prevent shared state bugs when creating a new logger with a different verbosity level. However,Extra()(lines 125-128) still mutatesl.extradirectly without copying, which means:log1 := NewLogger(ctx) log2 := log1.V(2) // log2 has its own copy of extra log1.Extra("k", "v") // mutates log1.extra // log2.extra is safe (separate copy)This is fine because
V()now copies. But if multiple goroutines share the same logger instance and callExtra()concurrently, there could be a data race on the map.Consider whether concurrent
Extra()calls on the same logger instance is a supported use case. If so,Extra()should also return a new logger with a copied map:func (l *logger) Extra(key string, value interface{}) Logger { - l.extra[key] = value - return l + newExtra := copyExtra(l.extra) + newExtra[key] = value + return &logger{ + context: l.context, + level: l.level, + extra: newExtra, + } }
170-184: Well-implementedWithEventIDhelper.The function correctly handles both the internal
*loggertype (creating a new instance with updated context) and other logger implementations (returning as-is). UsingcopyExtraensures the new logger doesn't share state with the original.pkg/logger/logger_test.go (2)
167-173: Test correctly updated for newEvtIDKeyconstant.The test case properly validates that the
EvtIDKeycontext key works as expected with the value "evt-12345".
290-293: Constant validation test updated correctly.The test properly verifies that
EvtIDKeyhas the expected string value "evtid", consistent with the constant definition inlogger.go.internal/config_loader/types.go (1)
90-105: Well-implemented validation forPayload.The
Validate()method correctly enforces the mutual exclusivity constraint betweenBuildandBuildRef, providing clear error messages for both violation cases.internal/k8s_client/client.go (2)
126-137: Consistent error wrapping pattern for CreateResource.The implementation correctly returns the raw error for
AlreadyExists(allowing callers to useapierrors.IsAlreadyExists()), while wrapping other errors withK8sOperationErrorfor richer context.
156-170: Consistent error handling across all operations.The pattern is applied consistently:
GetResource: UnwrapsNotFoundfor caller checksUpdateResource: UnwrapsConflictfor retry logicDeleteResource: Silently succeeds onNotFound(idempotent delete)PatchResource: UnwrapsNotFoundfor caller checksAll other errors are wrapped with
K8sOperationErrorproviding operation context. TheErrfield preserves the original error forerrors.Unwrap()chains.Also applies to: 250-262, 277-291, 340-354
internal/executor/executor_test.go (12)
15-27: LGTM on mock logger implementation.The mock logger correctly implements all methods of the
logger.Loggerinterface as no-ops, which is appropriate for unit testing where log output isn't the focus.
28-60: Mock API client is adequate for current tests.The implementation returns the same response/error for all HTTP methods, which works for basic testing. If more sophisticated per-method mocking is needed later, consider using a more flexible mock structure or a mocking library.
62-119: LGTM on constructor validation tests.Good table-driven test coverage for all required config fields.
121-147: LGTM on builder pattern test.The test validates the fluent builder API and DryRun flag propagation. Consider adding assertions for other builder fields (AdapterConfig, APIClient) in a future enhancement.
149-333: Comprehensive ExecutionContext test coverage.The tests thoroughly verify initial state, error propagation, evaluation tracking with both CEL and conditions types, phase filtering, and failed evaluation retrieval.
335-365: LGTM on error type tests.Error formatting and unwrapping behaviors are properly validated.
367-428: Well-structured param extraction integration test.Good use of
t.Setenvfor environment variable testing. The test validates extraction from both environment variables and event data sources.
430-523: Thorough ParamExtractor unit tests.Good coverage of extraction from multiple sources (env, event, nested), default value handling, and required field validation.
525-581: LGTM on template rendering tests.Good coverage of variable substitution, nested access, and error handling for missing variables.
583-707: Comprehensive sequential precondition execution tests.Excellent test coverage for stop-on-first-failure semantics, including verification of result counts, last executed precondition names, and skip reason propagation.
709-800: Appropriate resource execution tests for unit testing scope.Good coverage of sequential stop-on-failure behavior using dry-run mode. The comment correctly notes that full K8s testing is covered in integration tests.
875-955: LGTM on skip reason capture tests.Good verification that skip metadata (ResourcesSkipped, SkipReason) propagates correctly through the execution context.
internal/k8s_client/error.go (2)
7-20: Well-designed structured error type.The
K8sOperationErrorstruct captures all relevant context (Operation, Resource, Kind, Namespace, Message) needed for debugging Kubernetes operation failures.
22-30: LGTM on error formatting.Clear, informative error messages with conditional namespace inclusion.
test/integration/executor/main_test.go (2)
21-25: LGTM on constants definition.Clear constants for envtest configuration.
33-98: Well-structured TestMain with proper prerequisite checks.Good use of early-exit patterns when prerequisites (INTEGRATION_ENVTEST_IMAGE, Docker runtime) are unavailable. The cleanup is properly called after tests complete.
internal/executor/utils.go (4)
14-26: LGTM on ToConditionDefs conversion utility.Clean conversion function that centralizes the type mapping logic.
28-62: LGTM on log action execution.Proper handling of nil checks, template rendering, and log level selection. The
[config]prefix helps distinguish user-configured log messages from system logs.
64-83: LGTM on template rendering.Good optimization to skip parsing for non-template strings, and appropriate use of
missingkey=errorto catch template variable issues early.
94-133: LGTM on execution error serialization.Proper handling of optional error details (APIError, K8sError) with consistent map structure for CEL evaluation.
internal/executor/param_extractor.go (5)
12-24: LGTM on ParamExtractor structure and constructor.Clean struct design with appropriate dependencies.
26-60: LGTM on ExtractAll logic.Proper handling of required vs optional parameters with default value fallback. The upfront event data parsing is a good optimization.
81-102: LGTM on source routing logic.Clear switch-based routing with appropriate fallback behavior for unknown source types.
113-138: LGTM on event field extraction with dot notation.Good handling of both
map[string]interface{}andmap[interface{}]interface{}which can occur with YAML unmarshaling. Error messages clearly indicate the failed path.
156-174: LGTM on metadata parameter enrichment.Good addition of adapter and event context to the params map for use in templates.
internal/executor/precondition_executor.go (2)
18-30: LGTM!The struct definition and constructor follow idiomatic Go patterns with proper dependency injection of the API client and logger.
304-329:captureFieldFromDatadoesn't handle array indexing in paths.The current implementation only supports map traversal using dot notation. If API responses contain arrays (e.g.,
items.0.name), this will fail with a cryptic error message.Consider whether array indexing support is needed for your use cases. If so, you could extend the logic:
// Check for array index access if idx, parseErr := strconv.Atoi(part); parseErr == nil { if arr, ok := current.([]interface{}); ok { if idx < 0 || idx >= len(arr) { return nil, fmt.Errorf("array index %d out of bounds at path '%s'", idx, strings.Join(parts[:i+1], ".")) } current = arr[idx] continue } }test/integration/executor/executor_k8s_integration_test.go (2)
112-116: LGTM!The
GetStatusResponsesmethod properly uses mutex locking and returns a defensive copy of the slice to prevent race conditions.
297-420: LGTM!Comprehensive integration test covering the full flow with proper setup/teardown, namespace isolation, and thorough assertions on resource creation, data verification, and status reporting.
internal/executor/resource_executor.go (2)
535-558: LGTM!
BuildResourcesMapcorrectly implements collision detection for camelCase key conversion with clear error messages. The approach of tracking original names to detect collisions is sound.
264-278: Document or validate generation annotation consistency.The sorting logic (lines 268-276) assumes generation annotations are reliably set. If
getGenerationAnnotationValue()returns a default value (e.g., 0) for missing annotations, the fallback to name-based sorting becomes the primary determinant and may produce unexpected results. Verify that resources managed by this executor consistently have generation annotations, or explicitly document the behavior when generation is missing.test/integration/executor/executor_integration_test.go (2)
23-48: testLogger doesn't fully implement logger.Logger interface.Looking at
pkg/logger/logger.go, theLoggerinterface requires methods likeInfof,Warningf,Errorf, but yourtestLoggeronly implementsWarningfandErrorfat lines 38-43, not the other formatting methods that may be expected. However, I seelogCaptureLoggerat lines 1214-1244 does implement the same pattern.The implementation looks functional for the test purposes. No action needed if tests pass, but be aware that if the interface changes, both implementations need updates.
327-450: Comprehensive full flow test with good assertions.The test thoroughly validates:
- Parameter extraction
- Precondition evaluation and field capture
- Post-action execution and API calls
- Status payload structure validation
Good coverage of the happy path.
internal/executor/executor.go (4)
16-33: Solid constructor with proper validation.The constructor properly validates all required dependencies and returns meaningful errors. The use of
NewExecutorErrorprovides structured error information.
37-90: Well-structured phase execution with proper early exit.The parameter extraction phase correctly:
- Handles nil events
- Adds event ID to context for tracing
- Creates an event-specific logger
- Distinguishes between extraction failure (skip all phases, ACK event) and success
The design decision to ACK invalid events (not retry) is appropriate for poison message handling.
96-118: Good semantic distinction between precondition outcomes.The code correctly differentiates:
PreconditionNotMet→ business outcome, resources skipped, status remains success- Runtime errors → execution failed, error reason set
Both paths continue to post-actions, allowing error reporting. This is a clean design.
251-296: Clean builder pattern implementation.The fluent builder pattern is well-implemented with:
- Chainable methods for all configuration options
- Validation deferred to
Build()→New()for single point of validation- Clear method naming conventions
internal/executor/types.go (3)
246-266: Well-structured ExecutionError with proper error type reuse.The
ExecutionErrorstruct appropriately:
- Reuses
apierrors.APIErrorandk8s_client.K8sOperationErrorinstead of duplicating- Uses JSON tags for serialization
- Provides clear documentation for when each error type is populated
67-95: Comprehensive ExecutionResult captures all execution phases.The struct provides complete observability into the execution pipeline with per-phase results, timing, and error information. Including
ExecutionContextfor debugging is a good design choice.
32-36: Verify domain usage in AdapterConfig before applying this suggestion.The annotation key uses
hyperfleet.io, but the claim thatAdapterConfiguseshyperfleet.redhat.comrequires verification. Confirm both domain values and their intended purposes before considering this a consistency issue worth refactoring.
xueli181114
force-pushed
the
executor-new
branch
from
7e33b39 to
9e7b691
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/executor/post_action_executor.go (1)
198-205: CEL expression errors should propagate rather than return nil silently.When CEL evaluation fails (lines 200-204), the error is logged as a warning but
nilis returned without propagating the error. This matches the concern raised in a previous review. Silent failures could lead to unexpected nil values in payloads, causing issues in downstream API calls.Consider propagating the error instead:
if expr, ok := val["expression"].(string); ok { // Evaluate CEL expression result, err := evaluator.EvaluateCEL(strings.TrimSpace(expr)) if err != nil { - pae.log.Warning(fmt.Sprintf("CEL expression evaluation failed: %v", err)) - return nil, nil // Return nil for failed expressions + return nil, fmt.Errorf("CEL expression evaluation failed: %w", err) } return result.Value, nil }If returning
nilis intentional for resilience, add a clear comment documenting why this behavior is safe and when it's appropriate.internal/executor/types.go (1)
336-345: SetSkipped overwrites reason with message, losing the reason parameter.The
SetSkippedmethod assignsreasontoSkipReason(line 341) but then unconditionally overwrites it withmessageif message is non-empty (lines 342-344). This matches the concern from a previous review. The originalreasonparameter is lost, which may be unintentional.Consider preserving both reason and message:
func (ec *ExecutionContext) SetSkipped(reason, message string) { ec.Adapter.ExecutionStatus = string(StatusSuccess) ec.Adapter.ResourcesSkipped = true - ec.Adapter.SkipReason = reason - if message != "" { - ec.Adapter.SkipReason = message // Use message if provided for more detail + if message != "" { + ec.Adapter.SkipReason = fmt.Sprintf("%s: %s", reason, message) + } else { + ec.Adapter.SkipReason = reason } }This preserves both the reason code and the detailed message.
🧹 Nitpick comments (15)
internal/executor/README.md (1)
25-35: Add language specifier to fenced code block.The ASCII diagram should have a language specifier (e.g.,
textorplaintext) to satisfy markdown linting rules.-``` +```text ┌──────────────────────────────────────────────────────────────────────┐ │ Event Processing Pipeline │internal/executor/executor_test.go (3)
15-27: Mock logger has extra methods not in the interface.The
mockLoggerdefinesWarningfandErrorfmethods (lines 20-21), but checking theLoggerinterface inpkg/logger/logger.go, it only definesWarning(message string)andError(message string)(no formatted variants). These extra methods are unused. Either remove them or verify if the interface was extended elsewhere.type mockLogger struct{} func (m *mockLogger) V(level int32) logger.Logger { return m } func (m *mockLogger) Infof(format string, args ...interface{}) {} -func (m *mockLogger) Warningf(format string, args ...interface{}) {} -func (m *mockLogger) Errorf(format string, args ...interface{}) {} func (m *mockLogger) Extra(key string, value interface{}) logger.Logger { return m } func (m *mockLogger) Info(message string) {} func (m *mockLogger) Warning(message string) {} func (m *mockLogger) Error(message string) {} func (m *mockLogger) Fatal(message string) {}
121-147: Consider adding builder validation test.The builder test validates successful construction. Consider adding a test case where
Build()is called without required components (e.g., withoutWithAdapterConfig) to verify the builder properly returns errors, similar toTestNewExecutor.
802-873: Consider adding API call failure test cases.The test struct defines
mockResponseandmockErrorfields but only tests log actions. Consider adding test cases for API call post-actions that fail to verify sequential stop-on-failure behavior for API-based post-actions.test/integration/executor/main_test.go (2)
154-165: Consider checking for "already exists" error specifically.The code ignores all errors from
CreateResource, but the comment says "Ignore error if already exists". If the creation fails for a different reason (e.g., API server issue), this could mask setup problems.println(" Creating default namespace...") defaultNS := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "Namespace", "metadata": map[string]interface{}{"name": "default"}, }, } defaultNS.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Namespace"}) - _, _ = client.CreateResource(ctx, defaultNS) // Ignore error if already exists + _, err = client.CreateResource(ctx, defaultNS) + if err != nil && !k8serrors.IsAlreadyExists(err) { + sharedContainer.Cleanup() + return nil, fmt.Errorf("failed to create default namespace: %w", err) + }This requires importing
k8s.io/apimachinery/pkg/api/errorsask8serrors.
177-199: Minor: Logger created on each poll iteration.The logger is created inside the loop on each iteration. Consider moving it outside the loop for slight efficiency improvement.
func waitForAPIServerReady(config *rest.Config, timeout time.Duration) error { deadline := time.Now().Add(timeout) ctx := context.Background() + log := logger.NewLogger(ctx) for time.Now().Before(deadline) { - // Try to create a client - client, err := k8s_client.NewClientFromConfig(ctx, config, log) + client, err := k8s_client.NewClientFromConfig(ctx, config, log)internal/k8s_client/error.go (1)
37-59: Consider usingerrors.Asinstead of manual unwrapping.The
IsK8sOperationErrorfunction manually traverses the error chain. The standard library'serrors.Asdoes this automatically and handles edge cases (likeUnwrap() []errorfor multi-errors).+import ( + "errors" + "fmt" +) + // IsK8sOperationError checks if an error is a K8sOperationError and returns it. // This function supports wrapped errors. func IsK8sOperationError(err error) (*K8sOperationError, bool) { - if err == nil { - return nil, false - } - // Try direct type assertion first - if k8sErr, ok := err.(*K8sOperationError); ok { - return k8sErr, true - } - // Then try unwrapping - for err != nil { - if k8sErr, ok := err.(*K8sOperationError); ok { - return k8sErr, true - } - if unwrapper, ok := err.(interface{ Unwrap() error }); ok { - err = unwrapper.Unwrap() - } else { - break - } - } - return nil, false + var k8sErr *K8sOperationError + if errors.As(err, &k8sErr) { + return k8sErr, true + } + return nil, false }internal/executor/param_extractor.go (2)
18-24: Consider nil checks for constructor parameters.The constructor doesn't validate that
configandeventCtxare non-nil. While callers are expected to pass valid values, defensive programming would prevent nil pointer dereferences later.func NewParamExtractor(config *config_loader.AdapterConfig, eventCtx *ExecutionContext) *ParamExtractor { + if config == nil || eventCtx == nil { + panic("config and eventCtx must not be nil") + } return &ParamExtractor{ config: config, eventCtx: eventCtx, } }
140-154: Unimplemented features with clear TODO markers.The secret and configmap extraction functions are placeholders returning explicit errors. This is acceptable for the initial implementation.
Would you like me to open an issue to track the implementation of secret and configmap extraction?
internal/executor/precondition_executor.go (4)
184-187: Consider documenting the "no conditions = matched" default behavior.When neither structured conditions nor CEL expression is specified, the precondition is considered matched. This implicit behavior could be unexpected.
} else { - // No conditions specified - consider it matched + // No conditions specified - consider it matched (precondition acts as API call only) result.Matched = true + pe.log.V(2).Infof("Precondition '%s' has no conditions - auto-matched", precond.Name) }
227-232: Silent failure on invalid timeout duration.If
apiCall.Timeoutcontains an invalid duration string, the error is silently ignored and no timeout is set. This could hide configuration issues.// Set timeout if specified if apiCall.Timeout != "" { timeout, err := time.ParseDuration(apiCall.Timeout) - if err == nil { - opts = append(opts, hyperfleet_api.WithRequestTimeout(timeout)) + if err != nil { + return nil, fmt.Errorf("invalid timeout duration '%s': %w", apiCall.Timeout, err) } + opts = append(opts, hyperfleet_api.WithRequestTimeout(timeout)) }
248-274: Duplicate body template rendering logic across HTTP methods.POST, PUT, and PATCH have identical body rendering code. Consider extracting to a helper.
+ // Render body template if present + var body []byte + if apiCall.Body != "" { + body, err = renderTemplateBytes(apiCall.Body, execCtx.Params) + if err != nil { + return nil, fmt.Errorf("failed to render body template: %w", err) + } + } + // Execute request based on method var resp *hyperfleet_api.Response switch strings.ToUpper(apiCall.Method) { case http.MethodGet: resp, err = pe.apiClient.Get(ctx, url, opts...) case http.MethodPost: - body := []byte(apiCall.Body) - if apiCall.Body != "" { - body, err = renderTemplateBytes(apiCall.Body, execCtx.Params) - if err != nil { - return nil, fmt.Errorf("failed to render body template: %w", err) - } - } resp, err = pe.apiClient.Post(ctx, url, body, opts...) // ... similar for PUT, PATCH
304-329: Code duplication with extractFromEvent in param_extractor.go.This function is nearly identical to
extractFromEventinparam_extractor.go. Consider extracting to a shared utility inutils.go.+// In utils.go: +// NavigatePath navigates a nested map structure using dot notation +func NavigatePath(data map[string]interface{}, path string) (interface{}, error) { + parts := strings.Split(path, ".") + var current interface{} = data + for i, part := range parts { + switch v := current.(type) { + case map[string]interface{}: + val, ok := v[part] + if !ok { + return nil, fmt.Errorf("field '%s' not found at path '%s'", part, strings.Join(parts[:i+1], ".")) + } + current = val + case map[interface{}]interface{}: + val, ok := v[part] + if !ok { + return nil, fmt.Errorf("field '%s' not found at path '%s'", part, strings.Join(parts[:i+1], ".")) + } + current = val + default: + return nil, fmt.Errorf("cannot access field '%s': parent is not a map (got %T)", part, current) + } + } + return current, nil +}test/integration/executor/executor_k8s_integration_test.go (1)
59-99: Mock server request body reading may truncate large payloads.The request body reading uses a fixed 1MB buffer and reads once. For very large payloads, this may truncate data. Also,
r.Body.Readdoesn't guarantee reading the full body in one call.mock.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { mock.mu.Lock() defer mock.mu.Unlock() var bodyStr string if r.Body != nil { - buf := make([]byte, 1024*1024) - n, _ := r.Body.Read(buf) - bodyStr = string(buf[:n]) + bodyBytes, _ := io.ReadAll(r.Body) + bodyStr = string(bodyBytes) }Note: You'll need to add
"io"to the imports.internal/config_loader/types.go (1)
216-220: Consider validating the Level field in LogAction.The
LogActionstruct accepts aLevelfield with a default of "info", but there's no validation to ensure only valid log levels are used (debug, info, warning, error). Invalid levels could lead to runtime issues or unexpected behavior.Consider adding a validation method:
// Validate checks that the LogAction configuration is valid. func (la *LogAction) Validate() error { if la.Level == "" { return nil // defaults to info } validLevels := []string{"debug", "info", "warning", "error"} for _, valid := range validLevels { if la.Level == valid { return nil } } return fmt.Errorf("invalid log level '%s'; must be one of: debug, info, warning, error", la.Level) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
internal/config_loader/types.go(3 hunks)internal/executor/README.md(1 hunks)internal/executor/executor.go(1 hunks)internal/executor/executor_test.go(1 hunks)internal/executor/param_extractor.go(1 hunks)internal/executor/post_action_executor.go(1 hunks)internal/executor/precondition_executor.go(1 hunks)internal/executor/resource_executor.go(1 hunks)internal/executor/types.go(1 hunks)internal/executor/utils.go(1 hunks)internal/k8s_client/client.go(6 hunks)internal/k8s_client/error.go(1 hunks)pkg/logger/logger.go(3 hunks)pkg/logger/logger_test.go(2 hunks)test/integration/executor/executor_integration_test.go(1 hunks)test/integration/executor/executor_k8s_integration_test.go(1 hunks)test/integration/executor/main_test.go(1 hunks)test/integration/executor/setup_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/logger/logger_test.go
- test/integration/executor/setup_test.go
- internal/executor/resource_executor.go
- test/integration/executor/executor_integration_test.go
🧰 Additional context used
🧬 Code graph analysis (7)
internal/executor/param_extractor.go (2)
internal/config_loader/types.go (3)
AdapterConfig(6-11)Parameter(53-68)Metadata(14-18)internal/executor/types.go (3)
ExecutionContext(182-197)NewExecutorError(367-374)PhaseParamExtraction(23-23)
test/integration/executor/main_test.go (3)
test/integration/executor/setup_test.go (1)
K8sTestEnv(16-22)test/integration/testutil/container.go (1)
ContainerConfig(20-51)internal/k8s_client/client.go (2)
NewClientFromConfig(105-115)Client(20-23)
internal/k8s_client/client.go (1)
internal/k8s_client/error.go (1)
K8sOperationError(7-20)
test/integration/executor/executor_k8s_integration_test.go (4)
internal/config_loader/types.go (7)
Parameter(53-68)APICall(118-126)Resource(181-188)DiscoveryConfig(191-195)PostConfig(203-207)PostAction(210-214)SelectorConfig(198-200)internal/k8s_client/discovery.go (1)
Discovery(14-29)internal/hyperfleet_api/client.go (2)
WithTimeout(66-70)WithRetryAttempts(73-77)internal/executor/types.go (6)
StatusSuccess(43-43)OperationCreate(148-148)OperationUpdate(150-150)OperationRecreate(152-152)StatusFailed(45-45)PhasePostActions(29-29)
internal/executor/types.go (7)
internal/config_loader/types.go (2)
AdapterConfig(6-11)Resource(181-188)internal/k8s_client/client.go (1)
Client(20-23)pkg/logger/logger.go (1)
Logger(11-19)internal/criteria/evaluator.go (1)
EvaluationResult(12-23)internal/criteria/cel_evaluator.go (1)
CELResult(24-39)pkg/errors/api_error.go (1)
APIError(16-33)internal/k8s_client/error.go (1)
K8sOperationError(7-20)
internal/executor/executor.go (8)
internal/executor/types.go (11)
ExecutorConfig(49-60)Executor(63-65)NewExecutorError(367-374)NewExecutionContext(269-281)ExecutionContext(182-197)PhasePreconditions(25-25)PhaseResources(27-27)PhasePostActions(29-29)PreconditionResult(98-119)ResourceResult(122-141)PostActionResult(160-179)internal/config_loader/types.go (1)
AdapterConfig(6-11)pkg/logger/logger.go (3)
Logger(11-19)EvtIDKey(167-167)WithEventID(172-185)internal/executor/param_extractor.go (1)
NewParamExtractor(19-24)internal/executor/precondition_executor.go (1)
NewPreconditionExecutor(25-30)internal/executor/resource_executor.go (1)
NewResourceExecutor(31-37)internal/executor/post_action_executor.go (1)
NewPostActionExecutor(25-30)internal/k8s_client/client.go (1)
Client(20-23)
internal/executor/executor_test.go (6)
internal/executor/types.go (12)
ExecutorConfig(49-60)NewExecutionContext(269-281)ExecutionStatus(39-39)StatusSuccess(43-43)StatusFailed(45-45)PhasePreconditions(25-25)EvaluationType(221-221)EvaluationTypeCEL(225-225)EvaluationTypeConditions(227-227)PhasePostActions(29-29)PhaseResources(27-27)ExecutionContext(182-197)internal/config_loader/types.go (6)
AdapterConfig(6-11)Metadata(14-18)AdapterConfigSpec(21-29)Precondition(108-115)Resource(181-188)PostAction(210-214)internal/executor/executor.go (1)
New(16-33)internal/criteria/evaluator.go (1)
EvaluationResult(12-23)internal/criteria/types.go (3)
Operator(8-8)OperatorEquals(12-12)OperatorGreaterThan(22-22)internal/executor/param_extractor.go (1)
NewParamExtractor(19-24)
🪛 markdownlint-cli2 (0.18.1)
internal/executor/README.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (50)
pkg/logger/logger.go (3)
99-117: Good addition ofcopyExtrato prevent shared state.The deep-copy mechanism in
V()is a solid fix. However, theExtra()method (lines 125-128) still mutatesl.extradirectly. If a caller does:log1 := log.V(2) log1.Extra("key", "val") // mutates log1.extraThis is fine since
V()now copies. But if someone chains differently or reuses the base logger's extra map elsewhere, there could still be surprises. Consider whetherExtra()should also return a new logger with a copied map for full immutability.
170-185: LGTM!The
WithEventIDfunction is well-designed with proper type assertion handling. The fallback for non-internal loggers is reasonable and documented.
60-62: Consistent key rename.The change from
OpIDKeytoEvtIDKeyis applied consistently in bothprepareLogPrefixandprepareLogPrefixf.internal/executor/README.md (1)
7-19: Excellent documentation on status semantics.The clear distinction between runtime execution status and business outcomes is valuable. Making explicit that "Precondition not met is a successful execution" will prevent confusion in monitoring and alerting.
internal/executor/executor_test.go (3)
197-276: LGTM!Thorough test coverage for evaluation tracking, covering both CEL and conditions evaluation types with proper field result verification.
583-707: LGTM!Excellent coverage of sequential precondition execution. The tests properly verify the stop-on-failure behavior and correctly assert that precondition not met results in
StatusSuccesswithResourcesSkipped=true.
709-800: LGTM!Good unit test coverage for resource sequential execution in dry-run mode. The comment acknowledging that integration tests with real K8s API would provide better coverage is appropriate.
test/integration/executor/main_test.go (2)
33-98: LGTM!The
TestMainfunction has good defensive coding with progressive prerequisite checks (short mode, env var, container runtime). The cleanup in the deferred section ensures containers are properly terminated.
27-31:setupErris assigned but tests don't check it. Individual tests that depend onsharedEnvshould skip or fail gracefully ifsetupErr != nilto prevent nil pointer dereferences or using an incomplete test environment.internal/k8s_client/error.go (1)
5-35: LGTM!The
K8sOperationErrortype is well-designed with clear fields for debugging. TheError()method properly handles both namespaced and cluster-scoped resources, andUnwrap()enables proper error chain traversal.internal/k8s_client/client.go (6)
127-138: LGTM! Consistent error wrapping for CreateResource.The error handling correctly preserves
AlreadyExistserrors unwrapped (allowing callers to useapierrors.IsAlreadyExists()), while wrapping other failures with structuredK8sOperationErrorcontaining operation context.
157-170: LGTM! Properly preserves NotFound errors for GetResource.The implementation correctly leaves
NotFounderrors unwrapped so callers can check withapierrors.IsNotFound(), while other errors get the structured context.
208-215: LGTM! List operation error wrapping is correct.For list operations, setting
Resourcetogvk.Kindis a reasonable choice since there's no specific resource name. The structured error provides useful context for debugging.
249-262: LGTM! UpdateResource preserves Conflict errors appropriately.The implementation correctly keeps
Conflicterrors unwrapped for optimistic concurrency handling, while other failures get structured context.
277-291: LGTM! DeleteResource handles idempotency correctly.Returning
nilforNotFounderrors makes delete idempotent, which is the expected behavior. Other failures are properly wrapped.
340-354: LGTM! PatchResource error wrapping follows the established pattern.NotFound errors are preserved for caller checks, and other failures receive structured context.
internal/executor/param_extractor.go (4)
37-57: Potential issue: Non-required params with no default and failed extraction are silently skipped.When extraction fails for a non-required parameter without a default, the parameter is not added to the result map at all (line 48
continue). If downstream code expects all parameter names to be present (even with nil values), this could cause issues.Consider whether nil values should be explicitly stored:
if err != nil { if param.Required { return nil, NewExecutorError(PhaseParamExtraction, param.Name, fmt.Sprintf("failed to extract required parameter: %s", param.Source), err) } // Use default for non-required params if param.Default != nil { result[param.Name] = param.Default } + // Note: param with no default and failed extraction is intentionally omitted continue }
63-79: LGTM! Robust handling of nil/empty event data.The function correctly handles nil events and empty data by returning empty maps, preventing nil pointer issues downstream.
114-138: LGTM! extractFromEvent handles nested navigation correctly.The implementation properly handles both
map[string]interface{}andmap[interface{}]interface{}(from YAML parsing), with clear error messages for missing fields and type mismatches.
156-174: LGTM! Metadata enrichment is straightforward.The function correctly adds adapter and event metadata. Using
Time().String()produces a readable timestamp, which is appropriate for templates.internal/executor/utils.go (4)
16-26: LGTM! Clean conversion utility.
ToConditionDefsprovides a clean abstraction for converting between configuration and evaluation types, centralizing the conversion logic.
64-83: LGTM! Well-designed template rendering utility.The short-circuit for non-template strings and the
missingkey=erroroption provide both performance and safety. Error messages are descriptive.
94-133: LGTM! Comprehensive error serialization for CEL evaluation.The function properly serializes all error details including nested API and K8s error information, making rich error context available in CEL expressions.
48-59: Minor inconsistency in log method usage.Debug and Info levels use
Infofformat method, while Warning and Error usefmt.Sprintfwith non-format methods. For consistency, consider usingWarningfandErrorfif available on the logger interface:switch level { case "debug": log.V(2).Infof("[config] %s", message) case "info": log.Infof("[config] %s", message) case "warning", "warn": - log.Warning(fmt.Sprintf("[config] %s", message)) + log.Warningf("[config] %s", message) case "error": - log.Error(fmt.Sprintf("[config] %s", message)) + log.Errorf("[config] %s", message) default: log.Infof("[config] %s", message) }internal/executor/precondition_executor.go (2)
32-56: LGTM! Clear precondition orchestration with proper short-circuit behavior.The
ExecuteAllmethod correctly accumulates results and short-circuits on the first error or unmatched precondition, returning the accumulated results for debugging.
69-75: Good practice: Log action failures are non-fatal.Correctly treating log action failures as warnings rather than fatal errors ensures that debugging/auditing issues don't block the main workflow.
test/integration/executor/executor_k8s_integration_test.go (5)
139-295: LGTM! Comprehensive test fixture setup.The
createK8sTestConfigfunction creates a realistic adapter configuration that exercises multiple features: parameter extraction, preconditions with API calls and capture, resource creation with templates, and post-actions with CEL expressions.
297-420: LGTM! Comprehensive end-to-end integration test.This test thoroughly validates the full execution flow including K8s resource creation, data verification, label checking, and post-action status reporting. The assertions on status payload conditions verify the CEL expression evaluation.
737-812: Good failure scenario testing with status verification.The test correctly verifies that:
- Execution fails when creating resources in non-existent namespace
- Post-actions still execute to report errors
- Status payload contains error information from CEL expressions
The defensive assertions checking for non-default error values are particularly useful.
814-945: Well-documented limitation with TODO for future implementation.The test clearly documents that discovery-based update logic is not yet implemented and focuses on what currently works. The TODO provides a clear path for future enhancement.
947-1027: LGTM! Critical test for post-action execution after precondition failure.This test validates an important behavior: post-actions execute even when preconditions don't match, allowing error status to be reported. The assertions verify that error information flows through the CEL expressions correctly.
internal/config_loader/types.go (1)
70-105: LGTM!The
Payloadstruct and itsValidate()method properly enforce the mutual exclusivity constraint betweenBuildandBuildReffields. The validation logic and error messages are clear and correct.internal/executor/post_action_executor.go (4)
34-86: LGTM!The
ExecuteAllmethod properly orchestrates payload building and sequential post-action execution. Error handling is comprehensive, with appropriate differentiation between API errors and generic execution errors. The early termination on failure (lines 80-81) is clearly intentional and well-commented.
89-154: LGTM!The
buildPostPayloadsmethod correctly constructs an evaluation context with all necessary data (params, resources, adapter metadata), evaluates payload definitions, and marshals results to JSON. Error handling is thorough and logging is appropriate.
243-297: LGTM!The
executePostActionmethod is well-structured with proper timing, logging, and error handling. The decision to treat log action failures as non-fatal (line 257) is appropriate, and the comprehensive capture of API response details (even on error, lines 266-270) is good practice for debugging.
299-395: LGTM!The
executeAPICallmethod properly handles all aspects of HTTP requests: template rendering for URLs/headers/bodies, timeout and retry configuration, method-specific dispatching, and comprehensive error handling that preserves response details even on failures (lines 382-394).internal/executor/executor.go (8)
16-33: LGTM!The constructor properly validates all required configuration fields (AdapterConfig, APIClient, Logger) while correctly allowing K8sClient to be optional. Error messages are clear and appropriate.
37-90: LGTM!The initial setup and parameter extraction phase is well-implemented. The design decision to ACK (not retry) parameter extraction failures (lines 76-77, 84-90) is correct—it prevents infinite retry loops for malformed events. Event ID propagation through context and logger (lines 56-59) enables proper tracing.
92-118: LGTM!The preconditions phase correctly distinguishes between business outcomes (precondition not met) and runtime errors. Treating
PreconditionNotMetas a success with skipped resources (lines 97-105) is the right design—it's a valid business outcome, not a failure. The execution context is appropriately updated in both cases.
120-137: LGTM!The resources phase properly checks execution status and skip conditions (line 122) before proceeding. The design decision to continue to post-actions even on resource failures (line 131) is correct—it allows post-actions to report failures or perform cleanup.
139-173: LGTM!The post-actions phase and finalization are well-implemented. Post-actions always execute (even on failure, lines 139-140), which is correct for reporting and cleanup. The logic to preserve existing failure status (lines 146-149) prevents masking earlier errors. Final logging (lines 159-170) appropriately differentiates between success, skip, and failure outcomes.
175-229: LGTM!The phase execution helper methods properly delegate to specialized executor components while handling early returns for empty configurations. The K8s client availability check in
executeResources(line 213) correctly allows for dry-run mode when no client is configured.
233-248: LGTM!The
CreateHandlermethod provides a clean integration point for broker consumers. The decision to ACK parameter extraction failures (lines 238-241) is consistent with the design inExecuteand prevents infinite retry loops for invalid events. The comment at line 246 appropriately clarifies that skipped resources are not an error condition.
251-296: LGTM!The
ExecutorBuilderimplements a clean fluent interface for constructing anExecutor. All builder methods follow the standard pattern, and theBuildmethod properly delegates validation to theNewconstructor.internal/executor/types.go (6)
18-65: LGTM!The execution phase constants, status values, and configuration types are well-defined with clear documentation. The
ExecutorConfigappropriately marksK8sClientas optional (line 54), supporting dry-run scenarios.
67-179: LGTM!The result types comprehensively capture outcomes at each execution phase. The distinction between runtime errors (Error/ErrorReason) and business outcomes (ResourcesSkipped/SkipReason) in
ExecutionResultis well-designed. All result types appropriately include duration tracking.
181-228: LGTM!The
ExecutionContextand evaluation tracking types are well-designed. The reuse ofcriteria.EvaluationResultinEvaluationRecord.FieldResults(line 213) avoids duplication. The evaluation tracking provides a comprehensive audit trail for debugging and testing.
230-266: LGTM!The
AdapterMetadataandExecutionErrortypes effectively distinguish runtime failures from business outcomes. The reuse of existing error types (apierrors.APIError,k8s_client.K8sOperationError) inExecutionErrorprovides rich context without duplication.
268-327: LGTM!The
ExecutionContextconstructor properly initializes all maps and slices. The convenience methods (AddCELEvaluation,AddConditionsEvaluation) and query methods (GetEvaluationsByPhase,GetFailedEvaluations) provide a clean API for tracking and inspecting evaluation results.
347-391: LGTM!The error types follow Go conventions.
ExecutorErrorproperly implements error wrapping (lines 355-364), andPreconditionNotMetErroris correctly distinguished as a business outcome rather than a runtime error. TheIsPreconditionNotMethelper (lines 387-391) provides clean type checking.
xueli181114
force-pushed
the
executor-new
branch
from
9e7b691 to
3b29581
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
test/integration/executor/executor_integration_test.go (1)
834-836: Directos.Unsetenvmay cause test pollution.This was flagged in a previous review. Using
os.Unsetenvdirectly doesn't get cleaned up after the test, unliket.Setenv. Consider adding cleanup or restructuring the test.+ t.Cleanup(func() { + os.Setenv("HYPERFLEET_API_BASE_URL", mockAPI.URL()) + }) os.Unsetenv("HYPERFLEET_API_BASE_URL")internal/executor/post_action_executor.go (1)
20-25: Add validation for nil apiClient parameter.A previous review suggested adding nil validation to prevent potential nil pointer dereferences when methods are called.
func NewPostActionExecutor(apiClient hyperfleet_api.Client) *PostActionExecutor { + if apiClient == nil { + panic("apiClient is required") + } return &PostActionExecutor{ apiClient: apiClient, } }internal/executor/resource_executor.go (1)
391-415: Deep-copy fallback to shallow copy still risks subtle aliasing issues
deepCopyMapstill silently falls back to a shallow copy on marshal/unmarshal failures. If such a failure happens, later modifications to the “copied” manifest can mutate the original, which is exactly what this helper is trying to prevent and can be very hard to debug.Consider either:
- Logging the error before falling back, or
- Using a dedicated deep‑copy approach (e.g. a library like
copystructure) and only falling back to shallow copy as a clearly logged last resort.Example minimal change to at least surface the problem:
func deepCopyMap(m map[string]interface{}) map[string]interface{} { // Use JSON marshaling for a simple deep copy data, err := json.Marshal(m) if err != nil { - // Fallback to shallow copy + // Fallback to shallow copy – log so unexpected types are visible + // (manifests should normally be JSON-serializable). + // TODO: consider replacing this with a more robust deep copy. result := make(map[string]interface{}) for k, v := range m { result[k] = v } return result } @@ var result map[string]interface{} if err := json.Unmarshal(data, &result); err != nil { - // Fallback to shallow copy + // Fallback to shallow copy – log so unexpected shapes are visible. result := make(map[string]interface{}) for k, v := range m { result[k] = v } return result }internal/executor/types.go (1)
320-329:SetSkippedstill overwritesreasonwithmessage, losing the original code
SetSkippedfirst assignsSkipReason = reasonand then overwrites it whenmessageis non-empty:ec.Adapter.SkipReason = reason if message != "" { ec.Adapter.SkipReason = message }This throws away the structured
reason(which acts like a reason code) whenever a more detailed message is provided, making it harder to distinguish machine-readable reason from human-readable explanation.If you want to preserve both, consider either:
- Treating
reasonas the code andmessageas detail, e.g.:func (ec *ExecutionContext) SetSkipped(reason, message string) { ec.Adapter.ExecutionStatus = string(StatusSuccess) ec.Adapter.ResourcesSkipped = true - ec.Adapter.SkipReason = reason - if message != "" { - ec.Adapter.SkipReason = message // Use message if provided for more detail - } + if message != "" && reason != "" { + ec.Adapter.SkipReason = fmt.Sprintf("%s: %s", reason, message) + } else if message != "" { + ec.Adapter.SkipReason = message + } else { + ec.Adapter.SkipReason = reason + } }
- Or, if you need them separately, introduce a dedicated
SkipMessagefield and keepSkipReasonas the stable code.
🧹 Nitpick comments (11)
pkg/logger/logger.go (1)
99-116: Good addition ofcopyExtrafor immutability inV().The deep copy prevents shared state bugs when chaining logger methods. However, the
Extra()method at line 125-128 mutatesl.extradirectly, which can cause shared state issues whenExtra()is called afterV().Consider whether
Extra()should also return a new logger instance with a copied map for full immutability:func (l *logger) Extra(key string, value interface{}) Logger { - l.extra[key] = value - return l + newExtra := copyExtra(l.extra) + newExtra[key] = value + return &logger{ + context: l.context, + level: l.level, + extra: newExtra, + } }internal/executor/README.md (1)
25-35: ASCII diagram is appropriate without language specifier.The static analysis tool flags the missing language identifier, but for ASCII art diagrams, this is acceptable. Adding a language specifier like
```textwould be optional.internal/k8s_client/error.go (1)
41-49: Consider removing redundantIsK8sOperationErrorhelper.Since
K8sOperationErrorimplementsUnwrap(), callers can use the standarderrors.Asdirectly:var k8sErr *k8s_client.K8sOperationError if errors.As(err, &k8sErr) { // use k8sErr }The helper function duplicates this pattern without adding value. If you prefer to keep it for API convenience, that's acceptable, but it's not strictly necessary.
internal/executor/param_extractor.go (2)
21-35: Potential redundant default value handling.Lines 22-24 set the default when extraction fails for non-required params. Lines 28-31 also apply the default when value is nil. This could lead to confusion:
- If extraction fails with error → default is set (lines 22-24)
- If extraction succeeds with nil value → default is also set (lines 28-31)
Consider consolidating:
if err != nil { if param.Required { return NewExecutorError(PhaseParamExtraction, param.Name, fmt.Sprintf("failed to extract required parameter: %s", param.Source), err) } - // Use default for non-required params - if param.Default != nil { - execCtx.Params[param.Name] = param.Default - } + // Use default for non-required params when extraction fails + value = param.Default - continue } - - // Apply default if value is nil - if value == nil && param.Default != nil { - value = param.Default - } + // Apply default if value is nil (extraction returned nil or failed) + if value == nil { + value = param.Default + } if value != nil { execCtx.Params[param.Name] = value } }
100-114: Stub implementations are appropriate for MVP.The TODO comments clearly indicate these features are planned. Ensure tracking issues exist for implementing secret and configmap extraction.
Do you want me to open issues to track the implementation of secret and configmap extraction?
internal/executor/executor_test.go (1)
392-393: Consider checking json.Marshal errors in test setup.While unlikely to fail for simple map structures, ignoring marshal errors could mask test setup issues.
- eventDataBytes, _ := json.Marshal(eventData) - _ = evt.SetData(event.ApplicationJSON, eventDataBytes) + eventDataBytes, err := json.Marshal(eventData) + if err != nil { + t.Fatalf("failed to marshal event data: %v", err) + } + if err := evt.SetData(event.ApplicationJSON, eventDataBytes); err != nil { + t.Fatalf("failed to set event data: %v", err) + }internal/executor/precondition_executor.go (1)
200-225: Field capture handles nested maps correctly but could benefit from array support.The
captureFieldFromDatafunction correctly handles nestedmap[string]interface{}andmap[interface{}]interface{}types. However, it doesn't support array indexing in paths (e.g.,items.0.name). If this is a known limitation, consider adding a comment.test/integration/executor/executor_k8s_integration_test.go (2)
63-68: Request body reading may truncate large payloads.The fixed 1MB buffer could truncate larger request bodies. Consider using
io.ReadAllfor complete body reading.var bodyStr string if r.Body != nil { - buf := make([]byte, 1024*1024) - n, _ := r.Body.Read(buf) - bodyStr = string(buf[:n]) + body, err := io.ReadAll(r.Body) + if err == nil { + bodyStr = string(body) + } }
318-321: Consider handling errors from NewClient calls.Multiple calls to
hyperfleet_api.NewClient()ignore the returned error. While unlikely to fail with valid options, checking errors ensures test robustness.- apiClient, _ := hyperfleet_api.NewClient( + apiClient, err := hyperfleet_api.NewClient( hyperfleet_api.WithTimeout(10*time.Second), hyperfleet_api.WithRetryAttempts(1), ) + require.NoError(t, err, "Failed to create API client")Also applies to: 471-471, 577-577, 647-647, 704-704, 752-752, 907-907, 970-970
test/integration/executor/executor_integration_test.go (1)
95-101: Request body reading may truncate large payloads.Same issue as in the K8s integration test - consider using
io.ReadAll.var bodyStr string if r.Body != nil { - buf := make([]byte, 1024*1024) - n, _ := r.Body.Read(buf) - bodyStr = string(buf[:n]) + body, err := io.ReadAll(r.Body) + if err == nil { + bodyStr = string(body) + } }internal/executor/utils.go (1)
36-66: Consider better fallback when log message template rendering failsRight now, if
renderTemplatefails, the error is logged butmessageremains empty, so you end up logging"[config] "with no content. It may be more useful to either:
- Fall back to the raw template string, or
- Return early and skip logging altogether on template failure.
For example:
message, err := renderTemplate(logAction.Message, execCtx.Params) if err != nil { log.Error(fmt.Sprintf("failed to render log message: %v", err)) - } - - // Log at the specified level (default: info) + // Fall back to raw template so something still appears in logs. + message = logAction.Message + } + + // Log at the specified level (default: info)This preserves useful context even when templating fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
configs/adapter-config-template.yaml(1 hunks)internal/config_loader/accessors.go(1 hunks)internal/config_loader/types.go(3 hunks)internal/executor/README.md(1 hunks)internal/executor/executor.go(1 hunks)internal/executor/executor_test.go(1 hunks)internal/executor/param_extractor.go(1 hunks)internal/executor/post_action_executor.go(1 hunks)internal/executor/precondition_executor.go(1 hunks)internal/executor/resource_executor.go(1 hunks)internal/executor/types.go(1 hunks)internal/executor/utils.go(1 hunks)internal/k8s_client/client.go(6 hunks)internal/k8s_client/error.go(1 hunks)pkg/logger/logger.go(3 hunks)pkg/logger/logger_test.go(2 hunks)test/integration/executor/executor_integration_test.go(1 hunks)test/integration/executor/executor_k8s_integration_test.go(1 hunks)test/integration/executor/main_test.go(1 hunks)test/integration/executor/setup_test.go(1 hunks)test/integration/executor/testdata/test-adapter-config.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/config_loader/accessors.go
🚧 Files skipped from review as they are similar to previous changes (3)
- test/integration/executor/setup_test.go
- internal/config_loader/types.go
- internal/k8s_client/client.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-04T14:06:51.656Z
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 13
File: internal/executor/post_action_executor.go:198-205
Timestamp: 2025-12-04T14:06:51.656Z
Learning: Logger refactor is tracked in HYPERFLEET-304 for the hyperfleet-adapter repository, which will address how CEL evaluation failures and similar errors are handled and logged.
Applied to files:
internal/executor/post_action_executor.go
🧬 Code graph analysis (7)
test/integration/executor/executor_integration_test.go (8)
pkg/logger/logger.go (1)
Logger(11-19)internal/hyperfleet_api/types.go (1)
Request(73-88)internal/config_loader/types.go (4)
Precondition(98-105)Metadata(14-18)PostAction(199-203)Resource(171-178)internal/k8s_client/client.go (1)
NewClient(58-101)internal/executor/executor.go (1)
NewBuilder(245-249)internal/executor/types.go (7)
StatusSuccess(42-42)ExecutionContext(176-195)ExecutionStatus(38-38)StatusFailed(44-44)ExecutionResult(70-95)PhaseParamExtraction(22-22)PhasePostActions(28-28)internal/criteria/cel_evaluator.go (1)
CELResult(24-39)internal/criteria/types.go (1)
Operator(8-8)
test/integration/executor/main_test.go (3)
test/integration/executor/setup_test.go (1)
K8sTestEnv(16-22)test/integration/testutil/container.go (1)
ContainerConfig(20-51)internal/k8s_client/client.go (2)
NewClientFromConfig(105-115)Client(20-23)
internal/executor/param_extractor.go (2)
internal/config_loader/types.go (3)
AdapterConfig(6-11)Parameter(51-58)Metadata(14-18)internal/executor/types.go (3)
ExecutionContext(176-195)NewExecutorError(351-358)PhaseParamExtraction(22-22)
internal/k8s_client/error.go (1)
internal/config_loader/types.go (1)
Resource(171-178)
test/integration/executor/executor_k8s_integration_test.go (6)
internal/hyperfleet_api/types.go (1)
Request(73-88)internal/config_loader/types.go (11)
AdapterConfig(6-11)Metadata(14-18)AdapterConfigSpec(21-29)AdapterInfo(32-34)HyperfleetAPIConfig(37-42)Parameter(51-58)Precondition(98-105)Resource(171-178)PostConfig(193-196)PostAction(199-203)SelectorConfig(188-190)internal/criteria/types.go (1)
Operator(8-8)internal/k8s_client/discovery.go (1)
Discovery(14-29)test/integration/executor/setup_test.go (1)
SetupK8sTestEnv(25-38)internal/executor/types.go (6)
StatusSuccess(42-42)OperationCreate(144-144)OperationUpdate(146-146)OperationRecreate(148-148)StatusFailed(44-44)PhasePostActions(28-28)
internal/executor/utils.go (8)
internal/config_loader/types.go (3)
Condition(131-135)LogAction(206-209)APICall(108-116)internal/criteria/evaluator.go (1)
ConditionDef(291-295)internal/criteria/types.go (1)
Operator(8-8)internal/executor/types.go (2)
ExecutionContext(176-195)AdapterMetadata(227-240)pkg/logger/logger.go (1)
Logger(11-19)internal/hyperfleet_api/types.go (7)
Response(159-172)RequestOption(91-91)WithHeaders(94-103)WithRequestTimeout(134-138)WithRequestRetryAttempts(141-145)BackoffStrategy(13-13)WithRequestRetryBackoff(148-152)pkg/errors/api_error.go (2)
NewAPIError(123-134)IsAPIError(144-150)internal/config_loader/loader.go (1)
Parse(114-130)
internal/executor/executor.go (7)
internal/executor/types.go (9)
ExecutorConfig(48-59)Executor(62-67)PhaseParamExtraction(22-22)StatusFailed(44-44)StatusSuccess(42-42)PhasePreconditions(24-24)PhaseResources(26-26)PhasePostActions(28-28)ExecutionContext(176-195)internal/config_loader/types.go (1)
AdapterConfig(6-11)pkg/logger/logger.go (3)
Logger(11-19)EvtIDKey(167-167)WithEventID(172-185)internal/executor/precondition_executor.go (1)
NewPreconditionExecutor(21-25)internal/executor/resource_executor.go (1)
NewResourceExecutor(30-35)internal/executor/post_action_executor.go (1)
NewPostActionExecutor(21-25)internal/k8s_client/client.go (1)
Client(20-23)
🪛 markdownlint-cli2 (0.18.1)
internal/executor/README.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (49)
pkg/logger/logger.go (1)
170-185: LGTM! Well-designed API with appropriate fallback.The type assertion approach correctly handles the internal logger while gracefully degrading for other implementations. The comment on lines 182-183 appropriately documents the behavior for test loggers.
pkg/logger/logger_test.go (2)
167-173: LGTM! Test correctly updated for new event ID key.The test case properly validates the EvtIDKey constant and expected value "evt-12345".
290-293: LGTM! Constant validation updated correctly.The test properly asserts that
EvtIDKeyequals"evt_id".internal/executor/README.md (1)
1-48: LGTM! Comprehensive documentation for the executor package.The README effectively documents the four-phase execution pipeline, separates execution status from business outcomes, and provides a clear component overview. This will be valuable for onboarding and maintenance.
internal/k8s_client/error.go (1)
10-38: LGTM! Well-structured K8s operation error type.The error type provides good context with Operation, Resource, Kind, and Namespace fields. The
Unwrap()implementation correctly enableserrors.Isanderrors.Assupport for error chain traversal.configs/adapter-config-template.yaml (1)
390-395: LGTM! Good addition of resilience configuration for post-actions.The timeout, retry attempts, and exponential backoff settings provide appropriate resilience for the status reporting API call. The header configuration follows a clear name/value pattern.
test/integration/executor/testdata/test-adapter-config.yaml (1)
1-89: LGTM! Well-structured test configuration.The test config provides good coverage for the executor integration tests:
- Short timeouts appropriate for testing
- Empty resources array for dry-run mode
- CEL expressions exercising adapter state evaluation
- Clear parameter extraction from environment and event data
test/integration/executor/main_test.go (3)
181-203: Consider adding jitter or exponential backoff to API server readiness check.The fixed 500ms sleep interval is acceptable for tests, but if tests become flaky, consider using exponential backoff. Current implementation is fine for now.
35-99: LGTM! Well-structured TestMain with proper prerequisite checks.Good pattern for integration test setup:
- Proper short mode and environment variable checks
- Container runtime availability verification before expensive setup
- Clean separation of setup errors vs test execution
- Proper cleanup after all tests
165-168: Verify error unwrapping forCreateResourcewithK8sOperationError.Given the new
K8sOperationErrorwrapper ininternal/k8s_client/error.go, the error returned byCreateResourcemay be wrapped. Ensureapierrors.IsAlreadyExistsproperly detects the underlying error when checking:_, err = client.CreateResource(ctx, defaultNS) if err != nil && !apierrors.IsAlreadyExists(err) {If
CreateResourcewraps errors inK8sOperationError, verify thatK8sOperationErrorimplementsUnwrap()so that Kubernetes client-go'sapierrors.IsAlreadyExistscan traverse the error chain usingerrors.Asinternally. Otherwise, you may need to explicitly unwrap the error first.internal/executor/param_extractor.go (2)
74-98: LGTM! Robust nested map traversal.Good handling of both
map[string]interface{}andmap[interface{}]interface{}types, which correctly handles YAML unmarshaling edge cases. Error messages are clear and include the full path for debugging.
126-133: Consider consistent nil safety for Event metadata.The nil check for
execCtx.Eventis good. However, if any of the Event methods (ID(),Type(), etc.) could return empty strings, consider whether you want to include them or filter them out.internal/executor/executor_test.go (4)
15-60: Mock implementations are well-structured.The mock logger and API client implementations provide good test isolation. The mockAPIClient correctly implements all interface methods and allows configurable responses/errors for testing different scenarios.
62-119: Good coverage of NewExecutor validation cases.The table-driven tests comprehensively cover nil config, missing adapter config, missing API client, missing logger, and valid config scenarios. This ensures proper validation at construction time.
571-695: Comprehensive sequential execution tests for preconditions.The test cases effectively verify stop-on-first-failure behavior, proper result counting, and correct handling of the skip reason. The distinction between execution errors and "not met" outcomes is properly tested.
697-788: Resource sequential execution tests appropriately use dry-run mode.The comment correctly notes that real K8s integration is tested elsewhere. The tests verify sequential stop-on-failure behavior for resources with invalid manifests.
internal/executor/precondition_executor.go (1)
27-62: ExecuteAll implements proper sequential execution with clear outcome types.The function correctly distinguishes between execution errors (API failures) and business outcomes (precondition not met), returning appropriate outcome structures. The stop-on-first-failure semantics are properly implemented.
test/integration/executor/executor_k8s_integration_test.go (3)
24-31: Mock API server structure is well-designed for concurrent access.The
k8sTestAPIServerproperly uses a mutex to protect shared state, preventing race conditions during concurrent test execution.
297-420: Comprehensive integration test for full K8s resource creation flow.The test thoroughly validates ConfigMap and Secret creation, data verification, label verification, and status payload structure. Good use of assertions to verify CEL expression evaluation in the status payload.
814-945: Test correctly documents pending discovery-based update logic.The TODO comment clearly explains that discovery-based update logic is not yet implemented, and the test is appropriately scoped to verify current behavior.
test/integration/executor/executor_integration_test.go (4)
25-26: Good use of go:embed for test configuration.Embedding the test adapter config from YAML file improves maintainability and follows the suggestion from a previous review comment.
Also applies to: 225-238
1127-1157: Log capture logger implementation is well-designed.The
logCaptureLoggerproperly uses mutex for thread-safe log capture and implements all Logger interface methods. This is useful for verifying log output in tests.
876-925: Thorough testing of invalid event handling.The test validates that invalid JSON events are properly handled with appropriate phase and status, and importantly verifies that the handler ACKs (not NACKs) invalid events to prevent retry loops.
1159-1257: Post-action API failure test comprehensively validates error propagation.The test verifies that HTTP 500 errors are properly captured in the result, ExecutionError is populated correctly, and the error message contains relevant details like status code and step name.
internal/executor/post_action_executor.go (4)
155-162: CEL expression errors silently return nil - tracked in HYPERFLEET-304.Per previous discussion, this behavior is acknowledged and the logger refactor is tracked in HYPERFLEET-304. The current warning log approach is an acceptable interim solution.
Based on learnings, the logger refactor is tracked in HYPERFLEET-304 which will address how CEL evaluation failures are handled and logged.
27-68: ExecuteAll implements proper sequential execution with payload building.The two-phase approach (build payloads first, then execute actions) is well-structured. Error propagation correctly populates ExecutionError and stops on first failure.
70-111: Payload building correctly handles multiple sources and CEL evaluation.The function properly supports both inline Build and BuildRefContent sources, converts payloads to JSON strings for template rendering, and stores them in params for use by post-actions.
200-224: executePostAction is clean and delegates appropriately.The function properly logs, executes optional log action, and delegates API calls to a separate method. The completion log at line 221 provides good observability.
internal/executor/executor.go (5)
15-36: NewExecutor properly validates required configuration.The constructor validates all essential dependencies (config, AdapterConfig, APIClient, Logger) and fails fast with descriptive errors. K8sClient is correctly optional.
38-172: Execute method implements clear phase-based processing with proper error handling.The execution flow correctly:
- Parses event data at the boundary (decoupling CloudEvent)
- Extracts parameters with early termination on failure
- Evaluates preconditions and distinguishes errors from "not met" outcomes
- Skips resources when preconditions fail but continues to post-actions
- Always executes post-actions for error reporting
- Provides comprehensive logging at each phase
The distinction between
result.Status == StatusFailed(error) andresult.ResourcesSkipped(business outcome) is properly maintained.
199-216: Handler correctly ACKs param extraction failures to prevent infinite retry loops.The logic at lines 206-210 intentionally returns
nil(ACK) for param extraction failures because invalid events cannot be fixed by retrying. This is the correct behavior for message queue semantics.
239-284: ExecutorBuilder provides clean fluent interface.The builder pattern allows flexible configuration with optional K8sClient and DryRun settings. All builder methods correctly return the builder for chaining.
219-237: parseEventData provides clean boundary for CloudEvent decoupling.The function correctly handles nil events, empty data, and JSON parsing errors. This separation makes the parameter extraction phase independent of CloudEvent specifics.
internal/executor/resource_executor.go (5)
55-143: Resource execution flow and context mutation look coherentThe orchestration of manifest build, optional discovery, operation selection (create/update/recreate/dry‑run), and ExecutionContext.Resources updates is consistent and side‑effect aware. Error wrapping via
NewExecutorErrorplus settingExecutionErrorfor K8s ops provides good observability.
193-263: Deterministic discovery-by-selector is well-structuredDiscovery correctly supports by‑name and by‑selector, renders templates for namespace/labels, and uses a deterministic sort (generation desc, then name) when multiple matches exist. Returning a synthetic
NotFoundon empty lists integrates cleanly with callers.
287-345: Recreate flow and deletion wait are robustThe delete → poll‑until‑
NotFound→ create sequence is clear and uses the provided context for cancellation, avoiding indefinite waits. Logging at each step should make debugging resource recreation straightforward.
541-589:isRetryableDiscoveryErrorclassification is sensible and conservativeThe split between clearly retryable (timeouts, 5xx, service unavailable, network issues) and clearly fatal (auth/validation) errors matches the intended “retry or fail‑fast” semantics for discovery. Defaulting unknowns to non‑retryable is a good safety choice so unexpected failures surface early.
516-539: Resources-to-CEL map construction and collision handling look good
BuildResourcesMapcleanly converts snake_case names to camelCase keys and detects collisions with a clear error message, while skippingnilresources. This should make CEL access predictable and debuggable.internal/executor/utils.go (4)
21-31: Condition conversion helper is concise and removes duplication
ToConditionDefsstraightforwardly maps config conditions to criteria condition defs and will help keep the rest of the code DRY.
196-233: API response validation helper is consistent withAPIErrorusage
ValidateAPIResponsenicely centralizes the “nil response” and “non-success status” checks and ensures everything surfaces asAPIError, while preserving an existingAPIErrorif one is already present.
235-263: Template rendering utilities are safe and reusable
renderTemplateshort-circuits non-template strings, enforcesmissingkey=error, and wraps parse/execute failures with context.renderTemplateBytesis a thin, appropriate wrapper. These are good shared utilities for the rest of the executors.
265-293: CEL mapping helpers from execution metadata look correct
executionErrorToMapandadapterMetadataToMapexpose just enough structured information (status, reasons, messages, nested executionError map) for CEL without leaking implementation specifics. Returning an empty map forniladapter is also convenient.internal/executor/types.go (7)
47-67: Executor wiring fields are clear and extensible
ExecutorConfigcleanly captures the dependencies (adapter config, API client, K8s client, logger, dry-run flag), andExecutorholds the configured sub-executors. This should scale well as more phases/features are added.
70-95: ExecutionResult structure provides a solid summary surfaceCapturing per-phase results plus overall status, phase, skip flags, and the full
ExecutionContextgives you a good separation between a compact “summary” view and the detailed context.
175-195: ExecutionContext layout and defaults are sensibleThe context aggregates event, parameters, responses, resources, adapter metadata, and evaluation history.
NewExecutionContextinitializes all maps/slices, and defaultsExecutionStatusto"success", which works well with laterSetError/SetSkippedadjustments.
268-311: Evaluation recording helpers are straightforward and useful for debugging
AddEvaluationwith timestamp plus the CEL/conditions convenience wrappers, along with the phase/failed filters, provide a clear audit trail for decisions made during execution.
313-318: Error metadata setter is appropriately minimal
SetErrorsets failure status and fills bothErrorReasonandErrorMessage, which is sufficient for CEL and external reporting. No issues here.
331-358: ExecutorError wrapper is well-structured for phase/step contextThe error wrapper carrying phase, step, and message (with
Unwrap) will make logging and troubleshooting across phases much clearer. No changes needed.
360-371: PreconditionsOutcome shape matches intended semanticsSeparating
AllMatched(business outcome) fromError(execution failure) and givingNotMetReasona dedicated field aligns with how preconditions are typically surfaced to callers.
xueli181114
force-pushed
the
executor-new
branch
from
3b29581 to
ad5c9c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (9)
test/integration/executor/executor_integration_test.go (1)
834-837: Directos.Unsetenvmay cause test pollution.Using
os.Unsetenvdirectly doesn't restore the environment after the test, unliket.Setenv. While the error is now checked, this could still affect parallel test runs.Consider using a cleanup function to restore the value:
// Unset the env var after executor creation - if err := os.Unsetenv("HYPERFLEET_API_BASE_URL"); err != nil { - t.Fatalf("Failed to unset env var: %v", err) - } + originalValue := os.Getenv("HYPERFLEET_API_BASE_URL") + t.Cleanup(func() { + os.Setenv("HYPERFLEET_API_BASE_URL", originalValue) + }) + os.Unsetenv("HYPERFLEET_API_BASE_URL")internal/executor/post_action_executor.go (1)
20-25: Consider adding nil validation to constructor.The constructor accepts
apiClientbut doesn't validate it's non-nil. This could lead to nil pointer dereferences whenexecuteAPICallis invoked.func NewPostActionExecutor(apiClient hyperfleet_api.Client) *PostActionExecutor { + if apiClient == nil { + panic("apiClient is required") + } return &PostActionExecutor{ apiClient: apiClient, } }internal/executor/utils.go (2)
191-194: Add defensive nil check for response.When
err == nil,respis assumed to be non-nil, but a buggy client could return(nil, nil). This would cause a panic at line 192.+ if resp == nil { + nilErr := fmt.Errorf("API client returned nil response without error") + return nil, url, apierrors.NewAPIError(apiCall.Method, url, 0, "", nil, 0, 0, nilErr) + } + log.Infof("API call completed: %d %s", resp.StatusCode, resp.Status) return resp, url, nil
134-151: Inconsistent URL return on body template errors for PUT/PATCH.POST body template errors return
url(line 130), but PUT and PATCH body template errors return empty string""(lines 139, 148). This inconsistency makes debugging harder.case http.MethodPut: body := []byte(apiCall.Body) if apiCall.Body != "" { body, err = renderTemplateBytes(apiCall.Body, execCtx.Params) if err != nil { - return nil, "", fmt.Errorf("failed to render body template: %w", err) + return nil, url, fmt.Errorf("failed to render body template: %w", err) } } resp, err = apiClient.Put(ctx, url, body, opts...) case http.MethodPatch: body := []byte(apiCall.Body) if apiCall.Body != "" { body, err = renderTemplateBytes(apiCall.Body, execCtx.Params) if err != nil { - return nil, "", fmt.Errorf("failed to render body template: %w", err) + return nil, url, fmt.Errorf("failed to render body template: %w", err) } } resp, err = apiClient.Patch(ctx, url, body, opts...)internal/executor/precondition_executor.go (2)
86-94: Inconsistent indentation in error handling block.The indentation is misaligned which affects readability. The struct literal fields and return statement are not properly indented within the if block.
if err != nil { result.Status = StatusFailed result.Error = err // Set ExecutionError for API call failure - execCtx.Adapter.ExecutionError = &ExecutionError{ - Phase: string(PhasePreconditions), - Step: precond.Name, - Message: err.Error(), - } - - return result, NewExecutorError(PhasePreconditions, precond.Name, "API call failed", err) + execCtx.Adapter.ExecutionError = &ExecutionError{ + Phase: string(PhasePreconditions), + Step: precond.Name, + Message: err.Error(), + } + + return result, NewExecutorError(PhasePreconditions, precond.Name, "API call failed", err) }
104-112: Inconsistent indentation in parse error handling block.Same indentation issue as above - the struct fields and return statement need proper indentation.
// Set ExecutionError for parse failure - execCtx.Adapter.ExecutionError = &ExecutionError{ - Phase: string(PhasePreconditions), - Step: precond.Name, - Message: err.Error(), - } - - return result, NewExecutorError(PhasePreconditions, precond.Name, "failed to parse API response", err) + execCtx.Adapter.ExecutionError = &ExecutionError{ + Phase: string(PhasePreconditions), + Step: precond.Name, + Message: err.Error(), + } + + return result, NewExecutorError(PhasePreconditions, precond.Name, "failed to parse API response", err) }internal/executor/resource_executor.go (2)
591-636: Bug:isNetworkErrorshort-circuits on non-timeoutnet.Error, missing connection errors.The
net.Errorcheck at line 603-605 returnsnetErr.Timeout()immediately, preventing the*net.OpErrorcheck from running for non-timeout network errors like connection refused.This was flagged in a previous review. The fix should reorder checks so
*net.OpErroris checked first, or change thenet.Errorbranch to returntruefor all network errors.
391-415: Silent fallback to shallow copy may cause subtle bugs.This was flagged in a previous review. The JSON-based deep copy silently falls back to shallow copy on marshal/unmarshal errors, which could lead to unexpected mutations of the original map.
internal/executor/types.go (1)
320-329:SetSkippedoverwritesreasonwithmessage, losing the original reason.This was flagged in a previous review. The logic at lines 326-328 unconditionally overwrites
SkipReasonwithmessagewhen provided, discarding thereasonparameter.
🧹 Nitpick comments (19)
internal/hyperfleet_api/client_test.go (1)
117-128: Consider completing the testify migration for consistency.While the rest of the file has been updated to use testify assertions, this test still uses manual error checking (
if err != nil,if client == nil). For consistency with the refactored tests, consider updating this function to userequire.NoErrorandrequire.NotNil.Apply this diff to align with the testify pattern:
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { client, err := NewClient(tt.opts...) - if err != nil { - t.Errorf("NewClient returned error: %v", err) - } - if client == nil { - t.Error("client is nil") - } + require.NoError(t, err, "NewClient returned error") + require.NotNil(t, client, "client is nil") }) }internal/criteria/types.go (1)
99-107: Consider defensively initializingDatabefore bulk-assign
SetVariablesFromMapassumesc.Datais non-nil; if anEvaluationContextis ever constructed withoutNewEvaluationContext, this will panic on the first assignment. You could optionally guard with a lazy init (similar to howNewEvaluationContextsets it) to make the method safe on zero-valued contexts.internal/executor/README.md (1)
25-35: Add a language identifier to the ASCII-art code fencemarkdownlint is flagging the pipeline diagram fence for missing a language. Consider changing it to something like:
┌──────────────────────────────────────────────────────────────────────┐ ... └──────────────────────────────────────────────────────────────────────┘to satisfy MD040 and keep tooling happy.
pkg/logger/logger.go (1)
166-168: AlignWithEventIDcomment with actual behavior
WithEventIDonly injectsevt_idinto context/log prefix when the underlying implementation is the internal*logger; for otherLoggerimplementations it’s a no-op. The comment (“works with any Logger implementation”) could be tightened to clarify that it only decorates the built-in logger and simply returns other implementations unchanged.Also applies to: 170-185
internal/executor/param_extractor.go (1)
100-114: Secret/ConfigMap sources are stubbed; ensure configs can’t use them yet
extractFromSecretandextractFromConfigMapdeliberately return “not yet implemented” errors. If adapter configs are allowed to specifysecret.*orconfigmap.*sources today, required params will cause hard failures at runtime. If that’s not desired, consider gating these source types in config validation until the extraction logic is implemented.internal/k8s_client/client.go (1)
356-359: PatchResource may return a misleading error after successful patch.If
c.client.Patchsucceeds but the subsequentGetResourcefails (e.g., due to a transient error), the caller receives aK8sOperationErrorwithOperation: "get"even though the patch was successful. This could lead to unnecessary retries of the patch operation.Consider returning the patched object directly or handling the GetResource failure differently:
c.log.Infof("Successfully patched resource: %s/%s", gvk.Kind, name) - // Get the updated resource to return - return c.GetResource(ctx, gvk, namespace, name) + // Get the updated resource to return (optional - the obj already contains the patched state) + updated, err := c.GetResource(ctx, gvk, namespace, name) + if err != nil { + // Patch succeeded but refresh failed - log and return the original patched object + c.log.Warning(fmt.Sprintf("Patch succeeded but failed to refresh resource: %v", err)) + return obj, nil + } + return updated, niltest/integration/executor/executor_integration_test.go (1)
96-101: Potential incomplete body read in mock server.The body reading uses a fixed 1MB buffer and single
Readcall, which may not read the entire body for larger payloads. Additionally, the body is not closed after reading.// Read body var bodyStr string if r.Body != nil { - buf := make([]byte, 1024*1024) - n, _ := r.Body.Read(buf) - bodyStr = string(buf[:n]) + defer r.Body.Close() + bodyBytes, err := io.ReadAll(r.Body) + if err == nil { + bodyStr = string(bodyBytes) + } }Add
"io"to the imports.internal/executor/resource_executor.go (2)
150-152: Clarify behavior when multiple manifest items exist.When
ManifestItemshas multiple entries, only the first is used. If this is intentional (e.g., array refs are not yet supported), consider adding a warning log or documentation comment.if len(resource.ManifestItems) > 0 { // Use first manifest item (loaded from ref file) + // Note: Multiple manifest items from array refs are not yet supported manifestData = resource.ManifestItems[0]
316-345: Consider documenting the expected context timeout.
waitForDeletionrelies entirely on context cancellation for timeout. While this is a valid pattern, users should ensure the context has an appropriate deadline set. Consider adding a comment or default timeout.// waitForDeletion polls until the resource is confirmed deleted or context times out // Returns nil when the resource is confirmed gone (NotFound), or an error otherwise +// Callers should ensure ctx has an appropriate deadline (e.g., 30s-60s for most resources) func (re *ResourceExecutor) waitForDeletion(ctx context.Context, gvk schema.GroupVersionKind, namespace, name string, log logger.Logger) error {test/integration/executor/executor_k8s_integration_test.go (10)
24-116: Mock API server is solid; consider safer body handling and cleanupThe mock server looks good and gives you nice visibility into calls. Two small robustness tweaks you might consider:
- Read the request body via
io.ReadAll(or ajson.Decoder) instead of a fixed 1 MiB buffer + singleRead, and handle any read error explicitly so malformed / partial JSON can be detected rather than silently ignored.- Close the request body (
defer r.Body.Close()) once you’ve read it, even in tests, to keep resource usage clean.These are non-blocking, but will make the test server more future‑proof and less surprising.
118-137: Event helper silently ignores marshaling / SetData errors
createK8sTestEventdrops errors fromjson.Marshalandevt.SetData. With current static payload this is very unlikely to fail, but if the helper evolves it could hide issues.If you want stricter tests, consider either:
- Returning
(*event.Event, error)and checking withrequire.NoErrorat call sites, or- Panicking on error inside the helper (acceptable in test-only code).
Not mandatory, but it would make failures more obvious if the payload ever changes.
139-295: Config helper: unusedapiBaseURLargument and hard‑codedresourcesCreatedThe config template is comprehensive and matches the executor pipeline well. Two small nits:
- The
apiBaseURLparameter ofcreateK8sTestConfigisn’t used inside the function; all URLs come from thehyperfleetApiBaseUrlparam (env). Either wireapiBaseURLintoHyperfleetAPI.BaseURL(if that’s meaningful) or drop the parameter from the signature + call sites to avoid confusion.resourcesCreatedin the status payload is hard‑coded to"2", but several tests later overrideSpec.Resourcesto a single resource. That makes the reported payload inconsistent with what actually happened. If this field is meant to be consumed, consider making it driven by the number of resources in the config (or by the execution result) instead of a constant.Both are clarity/accuracy improvements; behavior today is otherwise fine.
297-420: Create‑resources test has good coverage; you can assert a bit more on the status payloadThe end‑to‑end flow (preconditions → K8s resources → post‑action to mock API) is well covered here. To tighten it further, you could:
- Assert that
result.PostActionResultscontains the expected post‑action withStatusSuccess, in addition to checking the HTTP side effect.- When inspecting
statusfromGetStatusResponses, also assert on fields likeclusterIdandresourcesCreatedto ensure the payload builder wiring is correct.Not required, but would make this a very strong regression test for the happy path.
422-524: Update‑existing‑resource test: prefer failing fast on API client and data extractionThe update scenario is clear and valuable. A couple of spots could be made stricter:
apiClient, _ := hyperfleet_api.NewClient()ignores the error; if client construction ever fails you’ll get confusing downstream behavior. Usingrequire.NoError(t, err)(and possibly logging options) would fail fast and point directly at the root cause.- When reading
cmDatafrom the updated ConfigMap, you ignore thefoundanderrreturns. Usingrequire.NoErrorand assertingfoundwill give a clearer failure if the structure is not as expected, rather than relying on map lookups to fail assertions indirectly.These changes keep the test intent the same but improve diagnostics if something breaks.
526-601: Discovery‑by‑labels test: consider asserting K8s state (not just operation type)The label‑based discovery test nicely verifies that the second execution reports
OperationUpdate. To fully exercise the discovery logic, you might also:
- List ConfigMaps by the label selector after each run and assert that only one resource exists for the
clusterId, and- Optionally assert that the name of the updated resource is the same between runs.
That would catch cases where the executor accidentally creates a new resource instead of truly discovering and updating the existing one.
687-735: Multiple‑resource‑types test could verify kinds/names for extra safetyThis test cleanly validates that both resources are created and discoverable by label. If you want to harden it further, you could also assert:
- The expected
Kind(ConfigMapvsSecret) perResourceResult, and/or- The expected resource names derived from
clusterId.Given
TestExecutor_K8s_CreateResourcesalready checks those details, this is optional, but would catch regressions where result ordering changes or kinds are mis‑mapped.
737-812: Resource‑creation‑failure test: make HTTP status reporting a hard assertionThe failure path coverage is excellent: you assert
StatusFailed, phasePhasePostActions, and that post‑actions still ran.For the HTTP reporting part, though, you currently treat missing status responses as a log‑only condition:
statusResponses := mockAPI.GetStatusResponses() if len(statusResponses) == 1 { // assertions... } else { t.Logf("Note: Expected status response..., got %d", len(statusResponses)) }Given the test intent (“Post actions should still execute to report error”), it would be stronger to
require.Len(t, statusResponses, 1)and then assert on the payload. Otherwise, a regression where the post‑action doesn’t actually send the HTTP call would slip through.Everything else in this test looks good.
814-945: Outdated doc comment vs current behavior in multiple‑matching‑resources testThe top comment still says:
Expected behavior: returns the first matching resource (order is not guaranteed by K8s API)
But the test body and the later comments explicitly state that discovery‑based update logic is not implemented yet and that the test currently only verifies creating a new resource when others with similar labels exist.
To avoid confusion, it would be better to update/remove the “returns the first matching resource” comment so it matches the current behavior under test (create‑only, no discovery). The rest of the test logic (3 pre‑created + 1 new,
createdCount == 1) looks consistent.
947-1027: Precondition‑not‑met test: nice coverage; consider asserting adapter fields more explicitlyThis test does a good job of documenting the semantics for “precondition not met”: overall
StatusSuccess, resources skipped, but post‑actions still run and surface error information.If you want even clearer guarantees around the status payload contract, you might:
- Assert on specific
adapter.*-derived fields in the payload (e.g. check that the message/reason mention “precondition” rather than just not being the defaults), and/or- Add an assertion on
result.PostActionResults[0].Statusto ensure the post‑action is itself marked as having run successfully even though the adapter reports a “negative” condition.As written, the test is already valuable; these would just make regressions in the skip/error‑reporting path more visible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
configs/adapter-config-template.yaml(1 hunks)go.mod(1 hunks)internal/config_loader/accessors.go(1 hunks)internal/config_loader/types.go(3 hunks)internal/config_loader/validator_test.go(1 hunks)internal/criteria/types.go(1 hunks)internal/executor/README.md(1 hunks)internal/executor/executor.go(1 hunks)internal/executor/executor_test.go(1 hunks)internal/executor/param_extractor.go(1 hunks)internal/executor/post_action_executor.go(1 hunks)internal/executor/precondition_executor.go(1 hunks)internal/executor/resource_executor.go(1 hunks)internal/executor/types.go(1 hunks)internal/executor/utils.go(1 hunks)internal/hyperfleet_api/client_test.go(17 hunks)internal/k8s_client/client.go(6 hunks)internal/k8s_client/error.go(1 hunks)pkg/logger/logger.go(3 hunks)pkg/logger/logger_test.go(2 hunks)test/integration/executor/executor_integration_test.go(1 hunks)test/integration/executor/executor_k8s_integration_test.go(1 hunks)test/integration/executor/main_test.go(1 hunks)test/integration/executor/setup_test.go(1 hunks)test/integration/executor/testdata/test-adapter-config.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- configs/adapter-config-template.yaml
- test/integration/executor/main_test.go
- test/integration/executor/testdata/test-adapter-config.yaml
- test/integration/executor/setup_test.go
- internal/config_loader/types.go
- pkg/logger/logger_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-04T14:06:51.656Z
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 13
File: internal/executor/post_action_executor.go:198-205
Timestamp: 2025-12-04T14:06:51.656Z
Learning: Logger refactor is tracked in HYPERFLEET-304 for the hyperfleet-adapter repository, which will address how CEL evaluation failures and similar errors are handled and logged.
Applied to files:
internal/executor/post_action_executor.go
🧬 Code graph analysis (8)
internal/executor/precondition_executor.go (7)
internal/k8s_client/client.go (1)
Client(20-23)internal/config_loader/types.go (2)
Precondition(98-105)APICall(108-116)internal/executor/types.go (1)
ExecutionContext(176-195)pkg/logger/logger.go (1)
Logger(11-19)internal/executor/utils.go (4)
ExecuteLogAction(36-66)ToConditionDefs(21-31)ExecuteAPICall(72-194)ValidateAPIResponse(199-233)internal/criteria/types.go (2)
NewEvaluationContext(67-71)Operator(8-8)internal/criteria/cel_evaluator.go (1)
CELResult(24-39)
internal/k8s_client/client.go (2)
internal/k8s_client/error.go (1)
K8sOperationError(10-23)internal/config_loader/types.go (1)
Resource(171-178)
internal/executor/post_action_executor.go (7)
internal/k8s_client/client.go (1)
Client(20-23)internal/config_loader/types.go (4)
PostConfig(193-196)Payload(67-78)PostAction(199-203)APICall(108-116)internal/executor/types.go (6)
ExecutionContext(176-195)PostActionResult(156-173)ExecutionError(243-250)PhasePostActions(28-28)NewExecutorError(351-358)StatusFailed(44-44)pkg/logger/logger.go (1)
Logger(11-19)internal/criteria/types.go (1)
NewEvaluationContext(67-71)internal/criteria/evaluator.go (2)
NewEvaluator(48-55)Evaluator(38-45)internal/executor/utils.go (2)
ExecuteLogAction(36-66)ExecuteAPICall(72-194)
internal/executor/param_extractor.go (2)
internal/config_loader/types.go (3)
AdapterConfig(6-11)Parameter(51-58)Metadata(14-18)internal/executor/types.go (3)
ExecutionContext(176-195)NewExecutorError(351-358)PhaseParamExtraction(22-22)
internal/executor/resource_executor.go (5)
internal/k8s_client/client.go (1)
Client(20-23)internal/config_loader/types.go (2)
Resource(171-178)DiscoveryConfig(181-185)internal/executor/types.go (11)
ExecutionContext(176-195)ResourceResult(120-137)StatusSuccess(42-42)StatusFailed(44-44)NewExecutorError(351-358)PhaseResources(26-26)OperationDryRun(152-152)OperationRecreate(148-148)OperationUpdate(146-146)OperationCreate(144-144)AnnotationGeneration(34-34)pkg/logger/logger.go (1)
Logger(11-19)internal/k8s_client/discovery.go (2)
Discovery(14-29)BuildLabelSelector(105-122)
test/integration/executor/executor_integration_test.go (3)
internal/config_loader/types.go (10)
Precondition(98-105)APICall(108-116)CaptureField(125-128)Metadata(14-18)LogAction(206-209)Condition(131-135)PostConfig(193-196)PostAction(199-203)Resource(171-178)Payload(67-78)internal/executor/types.go (5)
ExecutionContext(176-195)ExecutionStatus(38-38)ExecutionError(243-250)StatusFailed(44-44)ExecutionResult(70-95)internal/criteria/cel_evaluator.go (1)
CELResult(24-39)
test/integration/executor/executor_k8s_integration_test.go (5)
internal/config_loader/types.go (16)
AdapterConfig(6-11)Metadata(14-18)AdapterConfigSpec(21-29)AdapterInfo(32-34)HyperfleetAPIConfig(37-42)Parameter(51-58)Precondition(98-105)APICall(108-116)CaptureField(125-128)Condition(131-135)Resource(171-178)DiscoveryConfig(181-185)PostConfig(193-196)Payload(67-78)PostAction(199-203)SelectorConfig(188-190)internal/k8s_client/discovery.go (1)
Discovery(14-29)internal/k8s_client/client.go (2)
NewClient(58-101)Client(20-23)internal/hyperfleet_api/client.go (2)
WithTimeout(66-70)WithRetryAttempts(73-77)internal/executor/types.go (5)
StatusSuccess(42-42)OperationCreate(144-144)OperationUpdate(146-146)OperationRecreate(148-148)StatusFailed(44-44)
internal/executor/types.go (8)
internal/config_loader/types.go (2)
AdapterConfig(6-11)Resource(171-178)internal/k8s_client/client.go (1)
Client(20-23)pkg/logger/logger.go (1)
Logger(11-19)internal/executor/precondition_executor.go (1)
PreconditionExecutor(16-18)internal/executor/resource_executor.go (1)
ResourceExecutor(24-27)internal/executor/post_action_executor.go (1)
PostActionExecutor(16-18)internal/criteria/evaluator.go (1)
EvaluationResult(12-23)internal/criteria/cel_evaluator.go (1)
CELResult(24-39)
🪛 markdownlint-cli2 (0.18.1)
internal/executor/README.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (43)
go.mod (2)
3-3: Consider updating Go to a newer patch version.Go 1.25.5 was released on 2025-12-02, and includes security fixes. The project specifies Go 1.25.0, which misses security fixes in 1.25.2 and later.
Update to at least
go 1.25.5for the latest patches:-go 1.25.0 +go 1.25.5
86-87: Verify whether Ginkgo/Gomega should be marked as direct dependencies.Ginkgo and Gomega are commonly explicitly imported in
_test.gofiles. If they are directly imported in the test code for this executor feature, they should be listed asdirectdependencies in therequireblock rather thanindirect. If they're pulled transitively through another dependency (e.g., testcontainers-go), theindirectmarking would be appropriate—confirm by checking the test imports.internal/hyperfleet_api/client_test.go (1)
16-17: LGTM! Testify imports added correctly.The addition of testify assertion helpers improves test readability and consistency.
pkg/logger/logger.go (2)
60-62: Event ID prefix usage looks consistentUsing
EvtIDKeyin the prefix produces[evt_id=<id>]as intended and aligns with the new key naming.
99-109:copyExtraavoids shared state between derived loggersCloning
extraincopyExtraand using it inVprevents accidental cross-contamination of structured fields between verbosity-scoped loggers, while keeping the original behavior ofExtraintact.Also applies to: 115-116
internal/config_loader/validator_test.go (1)
6-9: Nice cross-check between config validation and criteria operators
TestValidOperatorsensures the set of operators expected by config validation stays aligned withcriteria.IsValidOperator. That’s a useful safety net whenever operators are added or renamed, even if it requires updating this list.Also applies to: 812-822
internal/config_loader/accessors.go (1)
35-36: Comment now correctly describes post-phase variablesUpdating the comment to “Post payloads” matches the current implementation (
Spec.Post.Payloads) and avoids confusion.internal/k8s_client/error.go (1)
8-49: Structured K8s error type and helpers look solid
K8sOperationErrorplusUnwrapandIsK8sOperationErrorgive clear, contextual errors and integrate cleanly witherrors.Is/As. As long as callers populateMessagefrom the underlying error, this should provide enough detail for both logs and programmatic handling.internal/executor/executor_test.go (8)
17-63: Test doubles for logger and API client are appropriate
mockLoggerandmockAPIClientprovide minimal, side-effect-free implementations of the respective interfaces, which keeps executor unit tests isolated from real logging and HTTP behavior.
142-221: ExecutionContext tests pin down important initial and tracking behaviorThe tests around
NewExecutionContext,SetError, and evaluation tracking (CEL + field-based) give good coverage of the internal bookkeeping (status, error fields, evaluations by phase and failed evaluations). They should quickly catch regressions in how execution metadata is recorded.
284-345: Param extraction integration test exercises key sources and defaults
TestExecute_ParamExtractionvalidates env and event data extraction via the full executor path, including event ID propagation andParamspopulation. This is a useful end-to-end check on top of the more focused extractor tests.
347-441: Param extractor unit tests cover main edge cases
TestParamExtractorcovers env and event (including nested), default value application, and failure on missing required params. This is a good spread of scenarios forextractConfigParams/extractParam, and should keep changes to source parsing honest.
497-613: Precondition sequential-execution tests clearly encode “stop on first not met” semantics
TestSequentialExecution_Preconditionsverifies both the stop-on-first-not-met behavior and the distinction between process failure vs. “resources skipped” business outcome. That’s exactly the subtle contract described in the README.
615-706: Resource sequential-execution tests assert stop-on-first-failure logic
TestSequentialExecution_Resourcesfocuses on manifest validation behavior in dry-run mode and ensures resource processing halts on the first invalid manifest. This gives confidence in the resource phase’s short-circuiting.
708-779: Post-action tests cover basic sequential behavior for log actions
TestSequentialExecution_PostActionschecks that a series of log-only post actions all execute and produce the expected results without error, which is a good baseline for the post-actions phase.
781-855: Skip-reason tests validate “successful but skipped” outcome handling
TestSequentialExecution_SkipReasonCaptureasserts that when preconditions are not met, the executor reportsStatusSuccesswithResourcesSkippedand a non-emptySkipReason, and that this is reflected in theExecutionContext.Adaptermetadata. This aligns nicely with the documented separation of process status vs. business decision.internal/executor/param_extractor.go (3)
11-39: Core param extraction path (env/event/default) looks correct and matches tests
extractConfigParams+extractParamhandle env (env.*), event (event.*), empty-source defaults, and the “direct event path” fallback in a clear way, with required-param failures wrapped asExecutorErroratPhaseParamExtraction. The tests inexecutor_test.goline up with this behavior and should catch regressions.Also applies to: 41-62, 64-98
73-98: Event data traversal handles both string-key and interface-key maps
extractFromEvent’s dot-notation traversal overmap[string]interface{}andmap[interface{}]interface{}is robust for typical decoded JSON/YAML payloads and returns helpful error messages when fields are missing or parent types are not maps.
116-134: Metadata enrichment into params matches documented template variables
addMetadataParamspopulatesmetadataandeventMetadataentries inexecCtx.Params, which aligns with the README’s template examples ({{ .metadata.name }},{{ .eventMetadata.id }}). This provides a clean bridge between config-level templates and runtime context.internal/executor/precondition_executor.go (1)
196-221: LGTM - Field capture with dot notation.The
captureFieldFromDatafunction correctly handles nested map traversal with proper error messages for missing fields and type mismatches.test/integration/executor/executor_integration_test.go (1)
1-27: Well-structured integration test suite.The test file demonstrates good practices:
- Using
go:embedfor test config YAML- Thread-safe mock server with mutex protection
- Comprehensive coverage of success, failure, and edge cases
- Proper cleanup with
defer mockAPI.Close()internal/executor/post_action_executor.go (1)
148-196: Value processing logic handles complex structures correctly.The
processValuefunction properly handles:
- CEL expressions with
expressionkey- Template values with
valuekey- Recursive nested maps
- Arrays with proper error propagation
- String template rendering
The CEL error handling returning
(nil, nil)is tracked in HYPERFLEET-304 per learnings.internal/executor/utils.go (1)
265-293: LGTM - Adapter metadata conversion for CEL.The
executionErrorToMapandadapterMetadataToMapfunctions correctly serialize execution context for CEL evaluation, with proper nil handling.internal/executor/resource_executor.go (6)
29-35: LGTM!Constructor is straightforward and correctly initializes the executor with the provided client and dry-run flag.
193-263: LGTM!Discovery logic is well-implemented with proper template rendering and deterministic resource selection when multiple matches exist.
494-514: LGTM!The
toCamelCasefunction correctly handles edge cases including single-character segments and empty parts from consecutive underscores.
516-539: LGTM!Proper collision detection when converting snake_case resource names to camelCase for CEL access.
541-589: LGTM!Comprehensive error classification for discovery operations with appropriate retry semantics.
486-492: Verify for duplicateGetResourceAsMapdefinitions.Cannot verify the duplication claim without repository access. The original review references
GetResourceAsMapintypes.go(lines 170-184), but notes the actual file doesn't show this function. If a duplicate exists elsewhere in the codebase, it will cause compilation errors. Manual verification of the entire codebase is required to confirm whether this function is defined multiple times.internal/executor/executor.go (5)
15-36: LGTM!Constructor properly validates required configuration fields and initializes sub-executors. The naming follows Go conventions (
NewExecutor).
40-172: Well-structured execution pipeline with good error handling.The four-phase execution is cleanly implemented with appropriate error propagation and logging. Post-actions always execute for error reporting, which enables proper cleanup/notification.
199-216: LGTM!Smart handling of broker integration: ACKs invalid events to prevent retry loops while propagating retriable errors.
219-237: LGTM!Defensive handling of nil/empty events with proper JSON parsing and error propagation.
239-284: LGTM!Clean fluent builder pattern with proper delegation to
NewExecutorfor validation.internal/executor/types.go (8)
17-45: LGTM!Well-defined execution phases and status constants with clear documentation.
47-67: LGTM!
ExecutorConfigandExecutorstructs properly aggregate dependencies with clear documentation of optional fields (K8sClient).
69-95: LGTM!
ExecutionResultcomprehensively captures all execution outcomes including params, results per phase, errors, and skip reasons.
97-173: LGTM!Result types for preconditions, resources, and post-actions are well-structured with appropriate fields for observability.
175-266: LGTM!
ExecutionContextproperly encapsulates runtime state with clear initialization. The constructor correctly initializes all maps to avoid nil pointer issues.
268-311: LGTM!Evaluation tracking methods provide good debugging and auditing capabilities with convenient helper methods for CEL and structured conditions.
331-358: LGTM!
ExecutorErrorproperly implements the error interface withUnwrap()for error chain support.
360-372: LGTM!
PreconditionsOutcomecleanly separates business outcome (AllMatched) from execution errors.test/integration/executor/executor_k8s_integration_test.go (1)
603-685: Recreate‑on‑change scenario looks goodThe
recreateOnChangetest is well structured: you assert the initialcreate, then explicitly compare UIDs to verify a true delete+create happened on the second run. No issues spotted here.
ciaranRoche
left a comment
ciaranRoche
left a comment
There was a problem hiding this comment.
This PR had chrome consuming more memory then it normally does, serious work in this, thanks Xue Li
xueli181114
force-pushed
the
executor-new
branch
from
ad5c9c5 to
770d25a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
internal/executor/post_action_executor.go (1)
20-25: Consider adding nil validation for apiClient parameter.Similar to
PreconditionExecutor, the constructor doesn't validate thatapiClientis non-nil.internal/executor/precondition_executor.go (2)
104-112: Fix inconsistent indentation in parse error handling block.Same indentation issue as above - the struct literal and return statement are misaligned.
// Set ExecutionError for parse failure - execCtx.Adapter.ExecutionError = &ExecutionError{ - Phase: string(PhasePreconditions), - Step: precond.Name, - Message: err.Error(), - } - - return result, NewExecutorError(PhasePreconditions, precond.Name, "failed to parse API response", err) + execCtx.Adapter.ExecutionError = &ExecutionError{ + Phase: string(PhasePreconditions), + Step: precond.Name, + Message: err.Error(), + } + + return result, NewExecutorError(PhasePreconditions, precond.Name, "failed to parse API response", err) }
86-94: Fix inconsistent indentation in error handling block.The
ExecutionErrorstruct literal and subsequent return statement have misaligned indentation. This was flagged in a previous review.if err != nil { result.Status = StatusFailed result.Error = err // Set ExecutionError for API call failure - execCtx.Adapter.ExecutionError = &ExecutionError{ - Phase: string(PhasePreconditions), - Step: precond.Name, - Message: err.Error(), - } - - return result, NewExecutorError(PhasePreconditions, precond.Name, "API call failed", err) + execCtx.Adapter.ExecutionError = &ExecutionError{ + Phase: string(PhasePreconditions), + Step: precond.Name, + Message: err.Error(), + } + + return result, NewExecutorError(PhasePreconditions, precond.Name, "API call failed", err) }internal/executor/utils.go (2)
139-141: PUT/PATCH body-template errors return empty URL instead of rendered URL.This issue was flagged in a previous review. When body template rendering fails for PUT (line 140) and PATCH (line 149), an empty string is returned instead of the already-rendered
url. This makes debugging harder for callers.Apply this fix:
if apiCall.Body != "" { body, err = renderTemplateBytes(apiCall.Body, execCtx.Params) if err != nil { - return nil, "", fmt.Errorf("failed to render body template: %w", err) + return nil, url, fmt.Errorf("failed to render body template: %w", err) } }Also applies to: 148-150
193-195: Missing nil-check on success path could panic if client returns(nil, nil).The success path assumes
respis non-nil whenerr == nil. While rare, a buggy client returning(nil, nil)would cause a panic at line 193.Add a defensive check:
+ if resp == nil { + nilErr := fmt.Errorf("API client returned nil response without error") + return nil, url, apierrors.NewAPIError(apiCall.Method, url, 0, "", nil, 0, 0, nilErr) + } + log.Infof("API call completed: %d %s", resp.StatusCode, resp.Status) return resp, url, nilinternal/executor/types.go (1)
317-326:SetSkippedoverwritesreasonwithmessagewhen both are provided.This was flagged in a prior review. The current logic assigns
reasontoSkipReasonat line 322, then unconditionally overwrites it withmessageif non-empty (lines 323-325), losing the original reason.Consider combining both values:
func (ec *ExecutionContext) SetSkipped(reason, message string) { ec.Adapter.ExecutionStatus = string(StatusSuccess) ec.Adapter.ResourcesSkipped = true - ec.Adapter.SkipReason = reason - if message != "" { - ec.Adapter.SkipReason = message - } + if message != "" { + ec.Adapter.SkipReason = fmt.Sprintf("%s: %s", reason, message) + } else { + ec.Adapter.SkipReason = reason + } }
🧹 Nitpick comments (16)
pkg/errors/network_error_test.go (2)
230-261: Document potential enhancement for wrapped EOF handling.The test correctly documents that wrapped
io.EOFreturnsfalsedue toutilnet.IsProbableEOFchecking the error directly. Consider whetherIsNetworkErrorshould explicitly unwrap and check forio.EOFto provide consistent detection regardless of wrapping.This could be addressed by adding to
IsNetworkError:if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) { return true }
293-334: Consider DNS error classification.The test documents that
net.DNSErrorwithIsNotFound: truereturnsfalse. This is a reasonable design choice since DNS resolution failures differ from transport-layer errors. However, some users might expect DNS failures to be classified as network errors.If this distinction becomes confusing for callers, consider either:
- Adding a separate
IsDNSErrorhelper, or- Documenting this behavior prominently in
IsNetworkError's doc comment.pkg/errors/k8s_error.go (1)
160-192: Solid error classification logic for retry decisions.The function correctly categorizes transient errors (timeouts, 5xx, network issues) as retryable and permission/validation errors as non-retryable. The default-to-non-retryable approach safely surfaces unknown issues.
Consider whether
apierrors.IsConflict(409) should be retryable for discovery operations, as conflicts can sometimes be transient.internal/criteria/evaluator_test.go (1)
11-15: StrengthenData()test to verify defensive copy semanticsRight now the test only asserts
ctx.Data()is non-nil. SinceData()is meant to expose a safe view of the internal map, consider also asserting that mutating the returned map does not affect values retrieved viaGet:func TestNewEvaluationContext(t *testing.T) { ctx := NewEvaluationContext() assert.NotNil(t, ctx) - assert.NotNil(t, ctx.Data()) + data := ctx.Data() + assert.NotNil(t, data) + + // Verify mutations on returned map don't affect context + ctx.Set("key", "value") + data["key"] = "modified" + val, ok := ctx.Get("key") + assert.True(t, ok) + assert.Equal(t, "value", val) }This locks in the intended copy-vs-alias behavior.
internal/criteria/README.md (1)
25-56: README now matchesNewEvaluatorAPI; add language to the ASCII blockThe code examples correctly use
criteria.NewEvaluator(ctx, log), which matches the updated constructor. To satisfy markdownlint (MD040) and improve tooling support, add a language to the fenced block used for the pipeline/ASCII diagram, e.g.:-``` +```text // diagram here -``` +```No other changes needed from a code/API perspective.
Also applies to: 129-207
internal/config_loader/validator_schema.go (1)
13-18: Stricter resource name validation and duplicate detection look good (with one optional cleanup)
- The
validResourceNameRegexand associated checks give clear, CEL-friendly constraints (lowercase start, alnum + underscore, no hyphens), and the error message is explicit. Together with theseenmap, this will surface misconfigured or duplicated resource names early in config validation, which is a nice improvement.- Be aware this is a behavioral change: any existing configs using kebab-case or uppercase-start resource names will now fail validation. That’s probably desirable for CEL compatibility, but worth calling out in release notes/migration docs.
As a small refactor while you’re touching this function, consider avoiding taking
&resourcewhereresourceis the range variable:for i := range v.config.Spec.Resources { resource := &v.config.Spec.Resources[i] // use resource.Name, resource.Manifest, and pass resource directly }This removes the common Go gotcha of pointers to the range variable if future changes need to mutate the resource in-place.
Also applies to: 151-181
internal/executor/README.md (1)
25-35: Specify a language for the pipeline diagram fenced blockmarkdownlint is flagging the ASCII pipeline diagram fenced block because it uses a bare
```. To satisfy MD040 and improve rendering/tooling, give it a language, e.g.:-``` +```text ┌──────────────────────────────────────────────────────────────────────┐ ... -``` +```Everything else in this README looks consistent with the implemented executor behavior.
internal/executor/executor_test.go (1)
429-432: Fix inconsistent indentation.The
assert.Errorandreturnstatements are not aligned with the surrounding code blocks.if tt.expectError { - assert.Error(t, err) - return - } + assert.Error(t, err) + return + }Apply similar fix at lines 485-488.
Also applies to: 485-488
test/integration/executor/main_test.go (1)
181-203: Consider using the context with timeout for API server readiness check.The function creates a background context and polls in a loop. Consider accepting a context parameter or using
context.WithDeadlineto integrate with the timeout more idiomatically and allow cancellation.-func waitForAPIServerReady(config *rest.Config, timeout time.Duration) error { - deadline := time.Now().Add(timeout) - ctx := context.Background() +func waitForAPIServerReady(ctx context.Context, config *rest.Config, timeout time.Duration) error { + ctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() log := logger.NewLogger(ctx) - for time.Now().Before(deadline) { + ticker := time.NewTicker(500 * time.Millisecond) + defer ticker.Stop() + + for { // Try to create a client client, err := k8s_client.NewClientFromConfig(ctx, config, log) if err == nil { // Try to list namespaces to verify API server is responsive gvk := schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Namespace"} _, err = client.ListResources(ctx, gvk, "", "") if err == nil { return nil // API server is ready } } - time.Sleep(500 * time.Millisecond) + select { + case <-ctx.Done(): + return fmt.Errorf("timeout waiting for API server to be ready") + case <-ticker.C: + // Continue polling + } } - - return fmt.Errorf("timeout waiting for API server to be ready") }internal/k8s_client/client.go (1)
196-201: Inconsistent error type for label selector errors in ListResources.Label selector parsing errors at lines 197 and 201 use
apperrors.KubernetesError, while the actual list operation failure at lines 208-215 usesapperrors.K8sOperationError. For consistency with other operations in this file, consider usingK8sOperationErrorfor the label selector errors as well, since they are related to the list operation.if labelSelector != "" { selector, err := metav1.ParseToLabelSelector(labelSelector) if err != nil { - return nil, apperrors.KubernetesError("invalid label selector %s: %v", labelSelector, err) + return nil, &apperrors.K8sOperationError{ + Operation: "list", + Resource: gvk.Kind, + Kind: gvk.Kind, + Namespace: namespace, + Message: fmt.Sprintf("invalid label selector %s: %v", labelSelector, err), + Err: err, + } } parsedLabelSelector, err := metav1.LabelSelectorAsSelector(selector) if err != nil { - return nil, apperrors.KubernetesError("failed to convert label selector: %v", err) + return nil, &apperrors.K8sOperationError{ + Operation: "list", + Resource: gvk.Kind, + Kind: gvk.Kind, + Namespace: namespace, + Message: fmt.Sprintf("failed to convert label selector: %v", err), + Err: err, + } }test/integration/executor/executor_k8s_integration_test.go (2)
63-68: Incomplete body reading in mock server.The current implementation uses
Read()which may not read the entire body if it exceeds the buffer or requires multiple reads. Also, errors are silently ignored.var bodyStr string if r.Body != nil { - buf := make([]byte, 1024*1024) - n, _ := r.Body.Read(buf) - bodyStr = string(buf[:n]) + bodyBytes, err := io.ReadAll(r.Body) + if err != nil { + t.Logf("Mock API: failed to read body: %v", err) + } + bodyStr = string(bodyBytes) }You'll need to add
"io"to the imports.
717-717: Missing context timeout in test execution.This test uses
context.Background()without a timeout, unlike other tests that usecontext.WithTimeout(). If the execution hangs, the test will never complete.- result := exec.Execute(context.Background(), evt) + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + result := exec.Execute(ctx, evt)internal/criteria/types.go (1)
175-184: Minor: Variable name shadows builtincopyfunction.The variable
copyshadows Go's builtincopyfunction. While this works correctly here, it's a minor style issue that could cause confusion.func (c *EvaluationContext) Data() map[string]interface{} { c.mu.RLock() defer c.mu.RUnlock() // Return a copy to prevent race conditions during CEL evaluation - copy := make(map[string]interface{}, len(c.data)) + result := make(map[string]interface{}, len(c.data)) for k, v := range c.data { - copy[k] = v + result[k] = v } - return copy + return result }internal/config_loader/types.go (1)
205-209: Consider documenting or validating log level values.The comment mentions valid levels (debug, info, warning, error), but there's no validation. Consider adding validation or documenting this as part of the config loader validation phase.
// ValidLogLevels defines the allowed log levels for LogAction var ValidLogLevels = []string{"debug", "info", "warning", "error"} // Validate checks that the LogAction configuration is valid. func (l *LogAction) Validate() error { if l.Level != "" { valid := false for _, lvl := range ValidLogLevels { if l.Level == lvl { valid = true break } } if !valid { return fmt.Errorf("invalid log level %q; must be one of: %v", l.Level, ValidLogLevels) } } return nil }internal/executor/precondition_executor.go (1)
20-25: Consider adding nil validation for apiClient parameter.The constructor accepts
apiClientbut doesn't validate it's non-nil. A nil client would cause a panic whenexecuteAPICallis invoked.func NewPreconditionExecutor(apiClient hyperfleet_api.Client) *PreconditionExecutor { + if apiClient == nil { + panic("apiClient is required") + } return &PreconditionExecutor{ apiClient: apiClient, } }test/integration/executor/executor_integration_test.go (1)
834-837: Directos.Unsetenvmay cause test pollution in parallel test runs.Using
os.Unsetenvdirectly mutates global state without automatic cleanup. While the error handling is good, this could affect other tests if run in parallel.Consider restructuring the test to avoid the mutation or add cleanup:
// Unset the env var after executor creation - if err := os.Unsetenv("HYPERFLEET_API_BASE_URL"); err != nil { - t.Fatalf("Failed to unset env var: %v", err) - } + // Register cleanup to restore env var after test + t.Cleanup(func() { + os.Setenv("HYPERFLEET_API_BASE_URL", mockAPI.URL()) + }) + if err := os.Unsetenv("HYPERFLEET_API_BASE_URL"); err != nil { + t.Fatalf("Failed to unset env var: %v", err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (43)
README.md(1 hunks)configs/adapter-config-template.yaml(1 hunks)go.mod(1 hunks)internal/config_loader/accessors.go(1 hunks)internal/config_loader/types.go(3 hunks)internal/config_loader/validator_schema.go(2 hunks)internal/config_loader/validator_test.go(1 hunks)internal/criteria/README.md(7 hunks)internal/criteria/cel_evaluator.go(8 hunks)internal/criteria/cel_evaluator_test.go(12 hunks)internal/criteria/evaluator.go(6 hunks)internal/criteria/evaluator_scenarios_test.go(7 hunks)internal/criteria/evaluator_test.go(13 hunks)internal/criteria/evaluator_version_test.go(1 hunks)internal/criteria/types.go(2 hunks)internal/executor/README.md(1 hunks)internal/executor/executor.go(1 hunks)internal/executor/executor_test.go(1 hunks)internal/executor/param_extractor.go(1 hunks)internal/executor/post_action_executor.go(1 hunks)internal/executor/precondition_executor.go(1 hunks)internal/executor/resource_executor.go(1 hunks)internal/executor/resource_executor_test.go(1 hunks)internal/executor/types.go(1 hunks)internal/executor/utils.go(1 hunks)internal/executor/utils_test.go(1 hunks)internal/hyperfleet_api/client_test.go(17 hunks)internal/k8s_client/README.md(2 hunks)internal/k8s_client/client.go(11 hunks)internal/k8s_client/data_extractor.go(1 hunks)pkg/errors/cel_error.go(1 hunks)pkg/errors/k8s_error.go(1 hunks)pkg/errors/k8s_error_test.go(1 hunks)pkg/errors/network_error.go(1 hunks)pkg/errors/network_error_test.go(1 hunks)pkg/logger/logger.go(3 hunks)pkg/logger/logger_test.go(2 hunks)test/integration/config-loader/config_criteria_integration_test.go(8 hunks)test/integration/executor/executor_integration_test.go(1 hunks)test/integration/executor/executor_k8s_integration_test.go(1 hunks)test/integration/executor/main_test.go(1 hunks)test/integration/executor/setup_test.go(1 hunks)test/integration/executor/testdata/test-adapter-config.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/config_loader/validator_test.go
- internal/config_loader/accessors.go
- test/integration/executor/testdata/test-adapter-config.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-04T14:06:51.656Z
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 13
File: internal/executor/post_action_executor.go:198-205
Timestamp: 2025-12-04T14:06:51.656Z
Learning: Logger refactor is tracked in HYPERFLEET-304 for the hyperfleet-adapter repository, which will address how CEL evaluation failures and similar errors are handled and logged.
Applied to files:
internal/executor/post_action_executor.go
🧬 Code graph analysis (21)
internal/config_loader/validator_schema.go (1)
internal/config_loader/constants.go (3)
FieldSpec(9-9)FieldResources(20-20)FieldName(31-31)
test/integration/executor/executor_integration_test.go (7)
pkg/logger/logger.go (1)
Logger(11-19)internal/hyperfleet_api/types.go (1)
Request(73-88)internal/config_loader/types.go (9)
Precondition(98-105)APICall(108-116)CaptureField(125-128)Metadata(14-18)HyperfleetAPIConfig(37-42)LogAction(206-209)Condition(131-135)PostAction(199-203)Resource(171-178)internal/k8s_client/client.go (1)
NewClient(58-101)internal/executor/types.go (6)
StatusSuccess(42-42)ExecutionContext(176-193)ExecutionStatus(38-38)ExecutionResult(70-95)PhaseParamExtraction(22-22)PhasePostActions(28-28)internal/criteria/cel_evaluator.go (1)
CELResult(26-42)internal/criteria/types.go (1)
Operator(10-10)
internal/criteria/evaluator_scenarios_test.go (1)
internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
internal/k8s_client/client.go (3)
pkg/errors/error.go (1)
KubernetesError(203-205)pkg/errors/k8s_error.go (1)
K8sOperationError(16-29)internal/config_loader/types.go (1)
Resource(171-178)
test/integration/executor/main_test.go (4)
test/integration/executor/setup_test.go (1)
K8sTestEnv(16-22)pkg/logger/logger.go (1)
NewLogger(32-39)test/integration/testutil/container.go (1)
ContainerConfig(20-51)internal/k8s_client/client.go (2)
NewClientFromConfig(105-115)Client(20-23)
test/integration/config-loader/config_criteria_integration_test.go (1)
internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
pkg/errors/k8s_error_test.go (3)
pkg/errors/k8s_error.go (3)
IsRetryableDiscoveryError(160-192)K8sOperationError(16-29)IsK8sOperationError(48-54)pkg/errors/error.go (1)
New(107-123)internal/config_loader/types.go (1)
Resource(171-178)
internal/executor/resource_executor_test.go (1)
pkg/logger/logger.go (1)
NewLogger(32-39)
internal/criteria/evaluator_test.go (1)
internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
pkg/errors/cel_error.go (1)
internal/criteria/types.go (1)
Operator(10-10)
internal/criteria/cel_evaluator_test.go (3)
internal/criteria/cel_evaluator.go (1)
NewCELEvaluator(55-73)internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
internal/k8s_client/data_extractor.go (2)
pkg/errors/k8s_error.go (3)
NewK8sInvalidPathError(96-102)NewK8sResourceDataError(128-136)NewK8sResourceKeyNotFoundError(74-81)internal/k8s_client/client.go (1)
Client(20-23)
internal/executor/param_extractor.go (3)
internal/config_loader/types.go (3)
AdapterConfig(6-11)Parameter(51-58)Metadata(14-18)internal/executor/types.go (3)
ExecutionContext(176-193)NewExecutorError(348-355)PhaseParamExtraction(22-22)internal/k8s_client/client.go (1)
Client(20-23)
internal/executor/executor.go (7)
internal/executor/types.go (11)
ExecutorConfig(48-59)Executor(62-67)PhaseParamExtraction(22-22)ExecutionResult(70-95)StatusFailed(44-44)NewExecutionContext(251-263)StatusSuccess(42-42)PhasePreconditions(24-24)PhaseResources(26-26)PhasePostActions(28-28)ExecutionContext(176-193)internal/config_loader/types.go (1)
AdapterConfig(6-11)pkg/logger/logger.go (1)
Logger(11-19)internal/executor/precondition_executor.go (1)
NewPreconditionExecutor(21-25)internal/executor/resource_executor.go (1)
NewResourceExecutor(27-32)internal/executor/post_action_executor.go (1)
NewPostActionExecutor(21-25)internal/k8s_client/client.go (1)
Client(20-23)
internal/executor/utils_test.go (3)
internal/hyperfleet_api/types.go (1)
Response(159-172)internal/executor/utils.go (1)
ValidateAPIResponse(200-234)pkg/errors/api_error.go (2)
IsAPIError(144-150)NewAPIError(123-134)
pkg/logger/logger_test.go (1)
pkg/logger/logger.go (1)
EvtIDKey(167-167)
internal/executor/precondition_executor.go (7)
internal/config_loader/types.go (2)
Precondition(98-105)APICall(108-116)internal/executor/types.go (4)
ExecutionContext(176-193)PreconditionsOutcome(358-368)PreconditionResult(98-117)StatusFailed(44-44)internal/executor/utils.go (4)
ExecuteLogAction(36-67)ToConditionDefs(21-31)ExecuteAPICall(73-195)ValidateAPIResponse(200-234)internal/criteria/types.go (2)
NewEvaluationContext(75-80)Operator(10-10)internal/criteria/evaluator.go (2)
NewEvaluator(52-60)EvaluationResult(14-25)internal/criteria/cel_evaluator.go (1)
CELResult(26-42)internal/config_loader/constants.go (1)
FieldValue(72-72)
internal/executor/post_action_executor.go (7)
internal/k8s_client/client.go (1)
Client(20-23)internal/config_loader/types.go (3)
Payload(67-78)PostAction(199-203)APICall(108-116)internal/executor/types.go (3)
ExecutionContext(176-193)PostActionResult(156-173)StatusFailed(44-44)pkg/logger/logger.go (1)
Logger(11-19)internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (2)
NewEvaluator(52-60)Evaluator(40-49)internal/executor/utils.go (3)
ExecuteLogAction(36-67)ExecuteAPICall(73-195)ValidateAPIResponse(200-234)
test/integration/executor/setup_test.go (2)
internal/k8s_client/client.go (1)
Client(20-23)pkg/logger/logger.go (1)
Logger(11-19)
internal/executor/resource_executor.go (4)
internal/config_loader/types.go (2)
Resource(171-178)DiscoveryConfig(181-185)internal/executor/types.go (10)
ExecutionContext(176-193)ResourceResult(120-137)StatusSuccess(42-42)StatusFailed(44-44)NewExecutorError(348-355)PhaseResources(26-26)OperationDryRun(152-152)OperationRecreate(148-148)OperationUpdate(146-146)OperationCreate(144-144)internal/k8s_client/discovery.go (2)
Discovery(14-29)BuildLabelSelector(105-122)pkg/errors/k8s_error.go (1)
IsRetryableDiscoveryError(160-192)
pkg/errors/k8s_error.go (1)
pkg/errors/network_error.go (1)
IsNetworkError(24-75)
🪛 markdownlint-cli2 (0.18.1)
internal/executor/README.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (123)
go.mod (1)
86-87: Verify no CVEs in Ginkgo v2.25.1 and Gomega v1.38.2.Go 1.25 was released in August 2025, and go1.25.5 was released 2025-12-02. Go 1.25.0 is a major release of Go, so your choice is valid; you may want to consider updating to 1.25.5 if security fixes are important.
Both test framework versions are recent and mature:
- Ginkgo v2.25.1 was released on August 22.
- Gomega v1.38.2 is the latest release.
However, confirm there are no known CVEs or security advisories for these versions before shipping. The current supported major version of Ginkgo is v2, so v2.25.1 is on the supported line.
pkg/errors/network_error.go (3)
1-23: LGTM!Clear documentation listing all detected network error types, and appropriate imports for the detection logic.
24-59: LGTM!Solid layered detection approach: k8s utilnet helpers handle common patterns, while direct syscall checks provide additional coverage for edge cases. The slight overlap with
ETIMEDOUTis harmless and provides defense-in-depth.
61-75: LGTM!The recursive unwrapping of
net.OpErrorcorrectly handles nested error chains. Thenet.Errorcheck focusing onTimeout()aligns with the function's documented scope—earlier checks via utilnet already cover most non-timeout network errors.pkg/errors/network_error_test.go (7)
16-29: LGTM!Clean mock implementation. The
Temporary()method is deprecated since Go 1.18, but including it maintains fullnet.Errorinterface compatibility for comprehensive testing.
31-90: LGTM!Comprehensive table-driven tests covering all documented syscall error codes with appropriate negative cases for non-network errors.
92-121: LGTM!Good coverage of error wrapping patterns including
fmt.Errorf, nested wrappers, andos.SyscallError, validating propererrors.Asunwrapping behavior.
123-164: LGTM!Solid coverage of
net.OpErrorscenarios with realistic structures including syscall errors and timeout errors, validating the recursive unwrapping logic.
166-202: LGTM!Important coverage for HTTP client scenarios—
url.Errorwrapping nested network errors is a common real-world pattern.
204-228: LGTM!Clear verification of the
net.Error.Timeout()behavior, including the important negative case.
263-291: LGTM!Good negative test coverage ensuring the function correctly rejects non-network errors.
internal/k8s_client/README.md (1)
17-17: Documentation updates align with the new executor-based architecture.The references to
internal/executorfor high-level operations, best practices, and related packages are consistent with the PR's introduction of the executor package.Also applies to: 319-319, 323-324
internal/hyperfleet_api/client_test.go (2)
16-17: Good adoption of testify for test assertions.The switch to
requireandassertfrom testify improves test readability and provides better failure messages.
530-532: Format string issues from past reviews have been addressed.The
%splaceholders now have corresponding arguments passed (errStr,bodyStr,apiErr.ResponseBodyString()), so failure messages will display actual values.Also applies to: 547-547, 656-656
pkg/logger/logger.go (2)
99-109: Good addition of deep copy for the extra map.The
copyExtrahelper prevents shared state bugs when creating new logger instances viaV(). However, note this is a shallow copy of the map values themselves - ifinterface{}values are references (maps, slices), they'll still be shared.
170-185: WithEventID implementation handles both internal and external logger types appropriately.The type assertion approach ensures event ID injection works for the internal logger while gracefully falling back for other implementations (like test loggers).
pkg/logger/logger_test.go (1)
167-172: Test updates correctly reflect the EvtIDKey changes.The test cases properly verify the new
evt_idcontext key and its expected value, aligning with the production code changes.Also applies to: 290-293
internal/executor/resource_executor_test.go (3)
13-39: Comprehensive test coverage for deepCopyMap with good documentation.The test correctly documents JSON serialization behavior (int → float64) and verifies mutation isolation. The comment at line 28 explaining the type conversion is helpful.
131-155: Excellent test documenting the shallow copy fallback danger.This test clearly demonstrates that non-JSON-serializable types force a shallow copy where nested mutations affect the original. This is valuable documentation for developers using
deepCopyMap.
14-14: Verify mockLogger is defined in the executor package.The tests use
mockLoggerwhich should be defined elsewhere in the package (likely in a shared test helpers file).pkg/errors/k8s_error.go (1)
14-54: Well-designed K8sOperationError with proper error chain support.The struct captures all relevant context (operation, resource, kind, namespace) and implements
Unwrap()forerrors.Is/Ascompatibility. TheIsK8sOperationErrorhelper follows idiomatic Go error extraction patterns.pkg/errors/cel_error.go (3)
21-44: Well-structured CELError with type categorization and error chain support.The error type captures expression context and supports
errors.Is/AsviaUnwrap(). The type predicates (IsParse,IsProgram,IsEval) provide clean error classification.
66-77: Note: NewCELEvalError returns nil when err is nil.This differs from
NewCELParseErrorandNewCELProgramErrorwhich don't have this nil check. This is likely intentional for convenience, but callers should be aware the function can return nil.
138-198: CELConversionError handles multiple conversion failure scenarios.The type-specific constructors (
NewCELUnsupportedOperatorError,NewCELUnsupportedTypeError,NewCELConditionConversionError) and conditional error formatting provide clear error messages for different failure modes.configs/adapter-config-template.yaml (1)
385-395: Post-action HTTP configuration is consistent and sensibleThe per-call timeout, retry policy, and explicit JSON
Content-Typeheader align well with the other API calls and make this status report more robust. No changes needed here.internal/criteria/evaluator_test.go (1)
470-1050: Evaluator constructor test updates match the new APIAll the changes from
NewEvaluator(ctx)toNewEvaluator(ctx, nil)(andNewEvaluator(nil, nil)inTestNewEvaluatorWithNilContext) correctly track the new constructor signature and still exercise the same behaviors (field access, condition evaluation, null-safety). Using anillogger in tests is reasonable here.internal/criteria/evaluator_scenarios_test.go (1)
45-341: Scenario tests correctly updated toNewEvaluator(ctx, nil)The constructor changes to
NewEvaluator(ctx, nil)across these scenarios are consistent with the new API and keep the tests’ intent intact (realistic config-based conditions, map/array handling, and node count validation).test/integration/config-loader/config_criteria_integration_test.go (1)
63-229: Integration tests aligned with new evaluator constructorUpdating all integration sites to
criteria.NewEvaluator(ctx, nil)keeps these tests in sync with the public API while preserving their behavior (config-based preconditions, CEL conversion, and null-safety checks). Looks good.Also applies to: 315-413
pkg/errors/k8s_error_test.go (1)
21-424: Comprehensive Kubernetes error helper testsThis suite gives thorough coverage for
IsRetryableDiscoveryError,K8sOperationError, andIsK8sOperationError, including wrapped errors, network/syscall cases, and a clear retryability matrix. The behavior encoded here matches the documented intent (transient vs fatal vs unknown) and should make future regressions obvious.internal/criteria/cel_evaluator_test.go (5)
16-18: LGTM!Constructor call correctly updated to match the new
NewCELEvaluator(ctx, nil)signature.
99-118: LGTM!Good update to handle the new error semantics where parse errors are returned as actual errors while evaluation errors are captured in the result. The conditional logic properly distinguishes between these two cases.
170-197: LGTM!Test cases properly updated to reflect the new safe evaluation semantics where missing field errors are captured in the result (not returned as errors), and error messages now check for "no such key" instead of "not found".
235-241: LGTM!Good addition of explicit test case for parse errors, verifying they are returned as actual errors (not captured in result) and contain the expected "parse error" message.
523-597: LGTM!Comprehensive test coverage for
EvaluateSafeerror handling scenarios including existing paths, missing leaf/intermediate fields, andhas()macro behavior. The test structure withwantSuccess,wantMatched, andwantReasonfields provides clear expected outcomes.internal/executor/executor_test.go (7)
17-31: LGTM!Mock logger implementation is adequate for testing purposes, capturing warnings when needed while providing no-op implementations for other methods.
33-65: LGTM!Mock API client implementation correctly implements the
hyperfleet_api.Clientinterface with configurable responses for testing.
67-123: LGTM!Good test coverage for
NewExecutorvalidation, testing nil config and missing required dependencies (adapter config, API client, logger).
170-223: Thorough evaluation tracking tests.Good coverage of CEL and conditions evaluation tracking, including field results with criteria types. This aligns with the past review comment discussion about keeping these tests for exposing criteria match results.
499-615: LGTM!Comprehensive tests for sequential precondition execution with stop-on-first-failure behavior. Test cases cover all pass, first fail, middle fail, and last fail scenarios.
617-708: LGTM!Resource sequential execution tests appropriately focus on the stop-on-failure logic in dry-run mode, with a note that real K8s integration is tested elsewhere.
783-857: LGTM!Good test coverage for skip reason capture when preconditions are not met, verifying that execution status remains
Success(valid outcome) whileResourcesSkippedandSkipReasonare properly set.test/integration/executor/main_test.go (2)
35-99: LGTM!Well-structured
TestMainwith proper prerequisite checks (short mode, env var, container runtime), graceful degradation when setup fails, and cleanup after tests complete.
102-178: LGTM!Solid integration test environment setup with proper error handling and cleanup on failure paths. The container configuration with retry and wait strategies is well-designed for reliability.
internal/executor/param_extractor.go (4)
13-41: LGTM!Clean implementation of parameter extraction with proper handling of required vs optional parameters and default value fallback.
43-64: LGTM!Good routing logic for different source types with sensible fallback to event data for unrecognized sources.
75-100: LGTM!Robust nested map traversal supporting both
map[string]interface{}(JSON) andmap[interface{}]interface{}(YAML) types with clear error messages including the full path.
132-150: Potential key collision with user-defined parameters.If users define parameters named
metadataoreventMetadata, this function will overwrite them. Consider documenting these as reserved keys or using a different naming convention (e.g.,_metadata,_eventMetadata) to prevent accidental collisions.internal/executor/utils_test.go (5)
15-27: LGTM!Good baseline test for successful response validation.
43-66: LGTM!Important test verifying that existing
APIErrorinstances are returned as-is without re-wrapping, preserving the original context.
84-199: LGTM!Comprehensive table-driven tests covering all major HTTP error status codes with appropriate assertions on
APIErrorfields.
348-414: LGTM!Excellent coverage of
APIErrorhelper methods, testing each status code classification method individually.
1-430: Well-structured and comprehensive test file.This test file provides thorough coverage of the
ValidateAPIResponsefunction including edge cases, error wrapping, and theAPIErrorhelper methods. The table-driven test approach makes it easy to extend coverage.internal/k8s_client/client.go (1)
117-142: LGTM! Clean error handling pattern for CreateResource.The approach of passing through
AlreadyExistserrors unwrapped while wrapping other errors withK8sOperationErroris a good pattern that allows callers to useapierrors.IsAlreadyExists()checks while still providing structured error context for unexpected failures.test/integration/executor/executor_k8s_integration_test.go (1)
297-420: LGTM! Comprehensive integration test with thorough verification.The test covers the full flow: K8s setup, mock API, executor execution, resource creation verification, data validation, and post-action status reporting. The cleanup with defer ensures proper resource cleanup even on test failure.
internal/criteria/evaluator_version_test.go (2)
140-179: LGTM! Good test for evaluator caching optimization.Testing that the CEL evaluator is not unnecessarily recreated when the context hasn't changed is valuable for verifying the optimization. Using
assert.Sameandassert.NotSamecorrectly verifies pointer identity.
10-42: Well-structured version tracking test.This test effectively demonstrates the fix for "undeclared reference" errors when variables are added after the first CEL evaluation. The test verifies that the CEL evaluator is properly recreated when the context version changes.
internal/config_loader/types.go (1)
80-95: LGTM! Good validation for Payload mutual exclusivity.The
Validate()method correctly enforces that exactly one ofBuildorBuildRefmust be set, with clear error messages.internal/criteria/evaluator.go (3)
66-83: LGTM! Well-implemented versioned CEL evaluator caching.The implementation correctly tracks context version changes and recreates the CEL evaluator only when necessary. The mutex properly protects concurrent access to the cached evaluator.
One consideration: if
NewCELEvaluatoris slow, holding the lock during creation could block other goroutines. However, since CEL environment creation is typically fast and this is a reasonable tradeoff for simplicity, this is acceptable.
320-340: Clean operator dispatch pattern with negate wrapper.The
operatorFuncsmap combined with thenegatewrapper is an elegant way to handle operator evaluation.NotEqualsandNotInare cleanly implemented as negated versions of their positive counterparts, reducing code duplication.
238-267: Good use of generics for the withCELEvaluator helper.The generic helper
withCELEvaluator[T any]cleanly handles the common pattern of getting the CEL evaluator and applying a function to it, reducing boilerplate inEvaluateCEL,EvaluateCELBool, andEvaluateCELString.internal/executor/precondition_executor.go (5)
27-62: LGTM!The sequential execution logic with early exit on errors or unmatched conditions is well-structured. Good separation between execution errors (API failures) and business outcomes (precondition not met).
128-172: LGTM!The condition evaluation logic properly handles both structured conditions and CEL expressions, with appropriate recording of evaluation results. The default to
matched=truewhen no conditions are specified is a sensible design choice.
183-193: LGTM!Clean delegation to shared utilities for API call execution and response validation.
195-220: LGTM!The field capture logic properly handles nested path traversal with both
map[string]interface{}andmap[interface{}]interface{}types. Error messages include useful path context for debugging.
222-242: LGTM!Clear formatting of condition evaluation details for error messages, handling both CEL and structured condition failures.
internal/executor/post_action_executor.go (6)
27-68: LGTM!Well-structured execution flow with proper error handling. The early return on payload build failure prevents executing actions with invalid data, and
ExecutionErroris appropriately set for debugging.
70-109: LGTM!Clear payload building logic with proper source validation (Build vs BuildRefContent), CEL evaluation context setup, and JSON marshaling for template compatibility.
111-146: LGTM!Proper type handling for YAML-unmarshaled maps and recursive payload building with template-rendered keys.
148-204: LGTM!The value processing logic correctly handles:
- CEL expressions with appropriate error propagation for actual errors vs expected missing fields (per HYPERFLEET-304 tracking)
- Template-rendered string values
- Recursive processing for nested structures and lists
206-230: LGTM!Clean post-action execution with proper delegation to log and API call helpers.
232-258: LGTM!Defensive response handling (captures details even on error) and useful error context differentiation between call failures and non-success status responses.
internal/k8s_client/data_extractor.go (4)
14-33: LGTM!Well-designed path parsing that allows dots in key names (e.g.,
tls.crt), with clear error handling using structured error types.
35-51: LGTM!Clean data retrieval with proper error handling for resource fetch, data field access, and missing data scenarios.
53-85: LGTM!Correct handling of Kubernetes Secret data extraction with base64 decoding and comprehensive error handling at each step.
87-113: LGTM!Correct handling of ConfigMap data extraction (plain strings, no base64 decoding) with consistent error handling pattern.
test/integration/executor/executor_integration_test.go (6)
25-27: LGTM!Good use of
go:embedfor externalizing the test configuration to a separate YAML file, improving readability as suggested in a previous review.
28-53: LGTM!Simple test logger implementation that captures all log output for test inspection.
54-164: LGTM!Well-designed mock API server with thread-safe request recording, configurable failure modes, and clear route handling for integration testing.
225-238: LGTM!Test helper functions are clean. Using
panicfor YAML parse failure in test setup is acceptable as it provides a clear failure mode.
1129-1159: LGTM!Thread-safe log capture implementation for verifying log output in tests.
240-363: LGTM!Comprehensive full-flow test with thorough verification of params, preconditions, post-actions, API calls, and status payload structure including CEL-evaluated health conditions.
test/integration/executor/setup_test.go (7)
1-13: LGTM!Proper use of
k8s.io/apimachinery/pkg/api/errorsfor type-safe error checking, addressing the previous review feedback.
15-22: LGTM!Clean test environment struct with appropriate fields for K8s integration testing.
40-45: LGTM!Standard cleanup pattern with nil check.
47-70: LGTM!Idempotent namespace creation with proper labeling for test isolation. Tolerating "already exists" allows tests to be re-run without manual cleanup.
72-81: LGTM!Appropriate cleanup behavior that logs warnings on failure rather than failing the test, allowing other cleanup to proceed.
83-87: LGTM!Proper use of
apierrors.IsAlreadyExistsfor type-safe error detection, addressing the previous review feedback about avoiding fragile string matching.
24-38: LGTM. Proper test setup pattern that gracefully skips tests when the K8s integration environment is unavailable.internal/executor/utils.go (5)
19-31: LGTM!Clean conversion helper that centralizes the condition type mapping.
36-67: LGTM!Robust log action execution with proper nil checks, template rendering with error handling, and configurable log levels.
197-234: LGTM!Solid API response validation with proper APIError wrapping for all failure modes.
236-264: LGTM!Template rendering with appropriate
missingkey=erroroption and early bypass for non-template strings.
266-294: LGTM!Clean map conversion helpers for CEL evaluation with proper nil handling.
internal/executor/executor.go (9)
16-36: LGTM!Constructor validates all required dependencies and follows the
NewExecutornaming convention. Clean initialization of sub-executors.
40-72: LGTM!Well-structured setup phase with proper nil checks, event ID propagation via context, and clean initialization of execution context.
76-84: LGTM!Clean parameter extraction phase with early termination on error.
86-110: Good separation of execution errors vs business outcomes.The precondition phase correctly distinguishes between:
- Execution errors (API failures) →
StatusFailed- Business outcomes (conditions not met) →
ResourcesSkippedBoth cases continue to post-actions for error reporting, which is the intended behavior.
112-134: LGTM!Resource phase correctly skips execution when preconditions aren't met or previous errors occurred, with appropriate logging for each case.
136-151: LGTM!Post-actions always execute to enable error reporting, which is the correct design for observability.
199-216: Intentional ACK on param extraction failures prevents retry loops.The decision to ACK (return nil) for param extraction failures at lines 207-209 is correct - invalid/malformed events should not be retried infinitely. The comment explains the rationale well.
219-237: LGTM!Defensive parsing with proper handling of nil event, empty data, and JSON unmarshal errors. The nil check at line 222 provides defense-in-depth even though Execute already validates the event.
239-284: LGTM!Clean builder pattern implementation with fluent API. Build() delegates to NewExecutor for consistent validation.
internal/executor/resource_executor.go (8)
26-52: LGTM!Constructor and ExecuteAll are well-implemented. The defensive nil check for
execCtx.Resourcesat lines 37-39 addresses the past review comment.
54-143: LGTM!Comprehensive resource execution with proper error categorization (retryable vs fatal), dry-run support, and execution context updates.
145-191: LGTM!Robust manifest building with proper type handling for YAML maps, deep copy to prevent mutation, template rendering, and validation of required K8s fields.
193-263: LGTM!Discovery implementation with template rendering, dual lookup strategies (byName/bySelectors), and deterministic selection via generation annotation sorting.
265-356: LGTM!CRUD operations are well-implemented.
updateResourcecorrectly preservesresourceVersionandUIDfor optimistic concurrency.waitForDeletionproperly handles context cancellation and uses NotFound as success signal. The hardcoded poll interval at line 319 is noted as a post-MVP improvement item.
391-425: LGTM!
deepCopyMapnow logs warnings on fallback to shallow copy, addressing the past review comment. The logging makes debugging easier when JSON serialization fails.
427-471: LGTM!Recursive template rendering for manifest keys and values handles all common Go/YAML types correctly.
473-516: LGTM!Helper functions for generation annotation extraction and resource map building are clean and handle nil cases appropriately.
internal/criteria/cel_evaluator.go (6)
54-73: LGTM!Constructor properly initializes with logger and handles nil context defensively. Error wrapping uses domain-specific error types.
75-91: Good thread-safety improvement.Snapshotting
ctx.Data()at line 84 before iteration prevents potential concurrent modification issues. This is a solid defensive pattern.
119-200: Well-designed error handling strategy.The three-tier error handling (parse → fail fast, program → fail fast, eval → capture) is correctly implemented. The documentation clearly explains when errors are returned vs captured, which helps callers understand the expected behavior.
202-259: LGTM!Generic
EvaluateAsfunction with proper type mismatch error handling. Type-specific convenience methods provide a clean API surface.
286-322: LGTM!
ConditionToCELproperly handles all operators including the nuancedexistscheck (usinghas()for nested paths vs null check for top-level). Uses domain-specific error types for unsupported operators.
324-387: LGTM!
formatCELValuehandles all common Go types including reflection-based handling for typed slices.ConditionsToCELproperly wraps per-condition errors.internal/executor/types.go (7)
17-45: LGTM!Well-defined phase and status constants with clear documentation.
47-95: LGTM!ExecutorConfig and ExecutionResult are well-structured with clear field documentation distinguishing runtime errors from business outcomes.
97-173: LGTM!Result types (PreconditionResult, ResourceResult, PostActionResult) capture comprehensive outcome data including API responses, CEL results, and operation details.
175-212: LGTM!ExecutionContext and EvaluationRecord provide proper runtime state tracking with evaluation audit trail.
250-263: LGTM!Clean constructor that properly initializes all maps and sets default status.
265-308: LGTM!Evaluation tracking methods provide good audit capabilities for debugging condition evaluations.
328-368: LGTM!ExecutorError with proper Error/Unwrap implementation for error chain support. PreconditionsOutcome clearly separates business outcomes from execution errors.
xueli181114
force-pushed
the
executor-new
branch
from
770d25a to
67155db
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
internal/config_loader/types.go (1)
49-58: Parameter validation is missing (as noted in prior review).The
Parameterstruct supportsSourceand new fields (Type,Description,Required,Default), but unlikePayload, it lacks aValidate()method to enforce constraints. This was flagged in a previous review.internal/criteria/types.go (1)
126-147: Potential deadlock in Merge still present (as noted in prior review).The
Mergemethod acquiresc.mu.Lock()thenother.mu.RLock(). If two goroutines callctx1.Merge(ctx2)andctx2.Merge(ctx1)simultaneously, a deadlock can occur due to inconsistent lock ordering.internal/executor/post_action_executor.go (1)
20-25: Consider adding nil validation for apiClient.The constructor doesn't validate that
apiClientis non-nil, which could lead to nil pointer dereferences in methods likeexecuteAPICall.test/integration/executor/executor_k8s_integration_test.go (1)
958-963: Potential data race when modifying clusterResponse.The
clusterResponsefield is read inside the HTTP handler under mutex (line 92), but it's being modified here in the test without acquiring the lock.Add a thread-safe setter method:
func (m *k8sTestAPIServer) SetClusterResponse(response map[string]interface{}) { m.mu.Lock() defer m.mu.Unlock() m.clusterResponse = response }Then use it in tests:
mockAPI.SetClusterResponse(map[string]interface{}{ "metadata": map[string]interface{}{"name": "test-cluster"}, // ... })internal/executor/precondition_executor.go (1)
86-94: Indentation is still misaligned.The
ExecutionErrorstruct literal andreturnstatement are not properly indented within theif err != nilblock. This was flagged in a previous review.if err != nil { result.Status = StatusFailed result.Error = err // Set ExecutionError for API call failure - execCtx.Adapter.ExecutionError = &ExecutionError{ - Phase: string(PhasePreconditions), - Step: precond.Name, - Message: err.Error(), - } - - return result, NewExecutorError(PhasePreconditions, precond.Name, "API call failed", err) + execCtx.Adapter.ExecutionError = &ExecutionError{ + Phase: string(PhasePreconditions), + Step: precond.Name, + Message: err.Error(), + } + + return result, NewExecutorError(PhasePreconditions, precond.Name, "API call failed", err) }test/integration/executor/executor_integration_test.go (1)
680-683: Directos.Unsetenvmay cause test pollution in parallel runs.Using
os.Unsetenvdirectly modifies global process state without automatic cleanup, unliket.Setenv. This could affect parallel test runs.Consider registering cleanup to restore the environment variable:
// Unset the env var after executor creation - if err := os.Unsetenv("HYPERFLEET_API_BASE_URL"); err != nil { - t.Fatalf("Failed to unset env var: %v", err) - } + originalValue := os.Getenv("HYPERFLEET_API_BASE_URL") + os.Unsetenv("HYPERFLEET_API_BASE_URL") + t.Cleanup(func() { + os.Setenv("HYPERFLEET_API_BASE_URL", originalValue) + })internal/executor/types.go (1)
314-323:SetSkippedoverwritesreasonwithmessagewhen both are provided.The logic at lines 320-322 sets
SkipReason = reasonthen immediately overwrites it withmessageif non-empty, discarding thereasonentirely.Consider either using message as fallback, or concatenating both:
func (ec *ExecutionContext) SetSkipped(reason, message string) { ec.Adapter.ExecutionStatus = string(StatusSuccess) ec.Adapter.ResourcesSkipped = true - ec.Adapter.SkipReason = reason - if message != "" { - ec.Adapter.SkipReason = message // Use message if provided for more detail - } + if message != "" { + ec.Adapter.SkipReason = message + } else { + ec.Adapter.SkipReason = reason + } }
🧹 Nitpick comments (19)
pkg/errors/network_error_test.go (1)
16-26: Test mock implements deprecated interface method.The
mockNetErrortype includesTemporary() boolwhich is deprecated since Go 1.18. While necessary for interface compliance withnet.Error, consider adding a comment noting the deprecation.type mockNetError struct { timeout bool temporary bool msg string } func (e *mockNetError) Error() string { return e.msg } func (e *mockNetError) Timeout() bool { return e.timeout } +// Temporary is deprecated but required by net.Error interface func (e *mockNetError) Temporary() bool { return e.temporary }internal/config_loader/types.go (1)
205-209: Consider validatingLogAction.Levelvalues.The comment documents valid levels (
debug,info,warning,error), but there's no validation. Invalid levels in configuration would be silently accepted. Consider adding validation in the schema validator or aValidate()method.// ValidLogLevels defines allowed log levels var ValidLogLevels = map[string]bool{ "debug": true, "info": true, "warning": true, "error": true, "": true, // empty defaults to info } func (l *LogAction) Validate() error { if l.Level != "" && !ValidLogLevels[l.Level] { return fmt.Errorf("invalid log level %q (allowed: debug, info, warning, error)", l.Level) } return nil }pkg/logger/logger.go (1)
170-185:WithEventIDsilently returns unchanged logger for non-internal implementations.For non-internal logger implementations (like test loggers),
WithEventIDreturns the original logger without adding the event ID. This means event IDs won't appear in logs when using custom logger implementations. Consider documenting this behavior or logging a warning.// For other logger implementations (like test loggers), return as-is // They should extract event ID from context if needed + // Note: Event ID will not be added to log output for non-internal logger types return loginternal/executor/post_action_executor_test.go (1)
419-421: Mock response is set but never used in some test cases.In test cases like "nil post config" and "empty post actions",
tt.mockResponseis nil and the mock client setup (lines 419-421) is essentially a no-op. This is fine but could be simplified by only setting mock responses when needed.internal/executor/README.md (1)
25-35: Consider adding a language identifier to satisfy linter, or suppress the warning.The static analysis tool flags the ASCII diagram code block for missing language. You can use
textorplaintextas the language identifier to silence this warning while keeping the formatting.-``` +```text ┌──────────────────────────────────────────────────────────────────────┐ │ Event Processing Pipeline │test/integration/executor/main_test.go (2)
101-102: Function name doesn't follow Go naming conventions.The function
setupsharedK8sEnvtestEnvshould besetupSharedK8sEnvtestEnvwith camelCase for "shared".-// setupsharedK8sEnvtestEnv creates the shared envtest environment for executor tests -func setupsharedK8sEnvtestEnv() (*K8sTestEnv, error) { +// setupSharedK8sEnvtestEnv creates the shared envtest environment for executor tests +func setupSharedK8sEnvtestEnv() (*K8sTestEnv, error) {Also update the call site at line 76:
- env, err := setupsharedK8sEnvtestEnv() + env, err := setupSharedK8sEnvtestEnv()
181-203:waitForAPIServerReadycreates new resources on each iteration.The function creates a new logger and attempts to create a new client on every loop iteration. While this works, it's slightly inefficient. Consider creating the logger once outside the loop.
func waitForAPIServerReady(config *rest.Config, timeout time.Duration) error { deadline := time.Now().Add(timeout) ctx := context.Background() + log := logger.NewLogger(ctx) for time.Now().Before(deadline) { // Try to create a client - log := logger.NewLogger(ctx) client, err := k8s_client.NewClientFromConfig(ctx, config, log)internal/executor/param_extractor.go (2)
60-63: Implicit fallback to event data may cause confusing errors.When a source string doesn't match any known prefix (
env.,event.,secret.,configmap.), it falls back to treating the entire source as an event data path. This could lead to confusing error messages if a user typos a prefix (e.g.,environ.VARwould try to findenviron.VARin event data).Consider adding a warning log or explicitly documenting this behavior.
default: - // Try to extract from event data directly + // Unknown prefix - try to extract from event data directly + // This allows shorthand like "cluster_id" instead of "event.cluster_id" return extractFromEvent(source, eventData)
141-149: Event time format may not be ideal for downstream consumers.
execCtx.Event.Time().String()uses Go's default time format which may not be suitable for API payloads or templates. Consider using RFC3339 format for better interoperability.if execCtx.Event != nil { execCtx.Params["eventMetadata"] = map[string]interface{}{ "id": execCtx.Event.ID(), "type": execCtx.Event.Type(), "source": execCtx.Event.Source(), - "time": execCtx.Event.Time().String(), + "time": execCtx.Event.Time().Format(time.RFC3339), } }This would require adding
"time"to the imports.internal/executor/executor_test.go (3)
23-37: Consider consolidating test logger implementations.Both
executor_test.goandpost_action_executor_test.godefine similar test logger structs (mockLoggerandtestLogger). Consider extracting a shared test logger to a test utilities package to reduce duplication.
400-403: Inconsistent indentation in assertion block.The
assert.Errorandreturnstatements have inconsistent indentation compared to other test functions in the file.if tt.expectError { - assert.Error(t, err) - return - } + assert.Error(t, err) + return + }
456-459: Same indentation issue as above.if tt.expectError { - assert.Error(t, err) - return - } + assert.Error(t, err) + return + }internal/k8s_client/client.go (1)
194-216: Minor inconsistency in error types for ListResources.Lines 197 and 201 return
apperrors.KubernetesError(a*ServiceError) for selector parsing failures, while line 208 returns&apperrors.K8sOperationErrorfor the actual list operation failure. Consider usingK8sOperationErrorconsistently for all errors in this method for uniform error handling by callers.if labelSelector != "" { selector, err := metav1.ParseToLabelSelector(labelSelector) if err != nil { - return nil, apperrors.KubernetesError("invalid label selector %s: %v", labelSelector, err) + return nil, &apperrors.K8sOperationError{ + Operation: "list", + Kind: gvk.Kind, + Namespace: namespace, + Message: fmt.Sprintf("invalid label selector %s: %v", labelSelector, err), + Err: err, + } } parsedLabelSelector, err := metav1.LabelSelectorAsSelector(selector) if err != nil { - return nil, apperrors.KubernetesError("failed to convert label selector: %v", err) + return nil, &apperrors.K8sOperationError{ + Operation: "list", + Kind: gvk.Kind, + Namespace: namespace, + Message: fmt.Sprintf("failed to convert label selector: %v", err), + Err: err, + } }internal/executor/executor.go (2)
91-111: Consider extracting precondition outcome handling to a helper.The precondition outcome handling has three branches (error, not matched, success) with context mutations. While functional, extracting this to a method like
handlePreconditionOutcomewould improve readability and reduce complexity inExecute.
222-238: Inconsistent nil handling in parseEventData.Lines 223-225 return an empty map for
nilevent, butExecutealready guards against nil events at line 44. This nil check is defensive but unreachable in normal flow. Consider documenting this as defensive coding or removing the redundancy.internal/hyperfleet_api/mock.go (1)
69-117: RequestOption parameters are ignored.The
Get,Post,Put,Patch, andDeletemethods acceptopts ...RequestOptionbut don't apply them to the request. This may cause tests to miss configuration issues. Consider applying options:func (m *MockClient) Get(ctx context.Context, url string, opts ...RequestOption) (*Response, error) { req := &Request{Method: "GET", URL: url} + for _, opt := range opts { + opt(req) + } m.Requests = append(m.Requests, req)internal/k8s_client/data_extractor.go (1)
21-33: Path parsing allows ambiguous formats when namespace/name contain dots.The path format
namespace.name.keyassumes namespace and name don't contain dots. If either contains a dot (e.g.,my.namespace.secret-name.key), parsing will be incorrect. Consider documenting this limitation or using a different delimiter:// ParseResourcePath parses a path in the format: namespace.name.key // Note: Namespace and resource name must not contain dots. // The key portion may contain dots (e.g., "default.mysecret.config.json")internal/executor/utils_test.go (1)
503-551: Add test for invalid template syntax.The template rendering tests cover success cases and missing variables, but don't test invalid template syntax (e.g.,
"{{ .name"). Consider adding a test case for malformed templates.test/integration/testutil/mock_api_server.go (1)
144-147:Close()doesn't reset state, potentially leaking test state between tests.If a test reuses the mock server instance after calling
Close()on a previous instance (or shares state), the recorded requests and responses persist. Consider callingReset()inClose()for cleaner teardown.// Close stops the mock server func (m *MockAPIServer) Close() { + m.Reset() m.server.Close() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (48)
README.md(1 hunks)configs/adapter-config-template.yaml(1 hunks)go.mod(1 hunks)internal/config_loader/accessors.go(1 hunks)internal/config_loader/types.go(3 hunks)internal/config_loader/validator_schema.go(2 hunks)internal/config_loader/validator_test.go(1 hunks)internal/criteria/README.md(7 hunks)internal/criteria/cel_evaluator.go(8 hunks)internal/criteria/cel_evaluator_test.go(12 hunks)internal/criteria/evaluator.go(6 hunks)internal/criteria/evaluator_scenarios_test.go(7 hunks)internal/criteria/evaluator_test.go(13 hunks)internal/criteria/evaluator_version_test.go(1 hunks)internal/criteria/types.go(2 hunks)internal/executor/README.md(1 hunks)internal/executor/executor.go(1 hunks)internal/executor/executor_test.go(1 hunks)internal/executor/param_extractor.go(1 hunks)internal/executor/post_action_executor.go(1 hunks)internal/executor/post_action_executor_test.go(1 hunks)internal/executor/precondition_executor.go(1 hunks)internal/executor/resource_executor.go(1 hunks)internal/executor/resource_executor_test.go(1 hunks)internal/executor/types.go(1 hunks)internal/executor/utils.go(1 hunks)internal/executor/utils_test.go(1 hunks)internal/hyperfleet_api/client_test.go(17 hunks)internal/hyperfleet_api/mock.go(1 hunks)internal/k8s_client/README.md(2 hunks)internal/k8s_client/client.go(11 hunks)internal/k8s_client/data_extractor.go(1 hunks)internal/k8s_client/interface.go(1 hunks)internal/k8s_client/mock.go(1 hunks)pkg/errors/cel_error.go(1 hunks)pkg/errors/k8s_error.go(1 hunks)pkg/errors/k8s_error_test.go(1 hunks)pkg/errors/network_error.go(1 hunks)pkg/errors/network_error_test.go(1 hunks)pkg/logger/logger.go(3 hunks)pkg/logger/logger_test.go(2 hunks)test/integration/config-loader/config_criteria_integration_test.go(8 hunks)test/integration/executor/executor_integration_test.go(1 hunks)test/integration/executor/executor_k8s_integration_test.go(1 hunks)test/integration/executor/main_test.go(1 hunks)test/integration/executor/setup_test.go(1 hunks)test/integration/executor/testdata/test-adapter-config.yaml(1 hunks)test/integration/testutil/mock_api_server.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- internal/config_loader/accessors.go
- README.md
- test/integration/config-loader/config_criteria_integration_test.go
- internal/config_loader/validator_test.go
- internal/hyperfleet_api/client_test.go
- internal/executor/utils.go
- configs/adapter-config-template.yaml
- internal/criteria/README.md
- go.mod
- pkg/errors/cel_error.go
- pkg/logger/logger_test.go
- test/integration/executor/testdata/test-adapter-config.yaml
- internal/k8s_client/README.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-04T14:06:51.656Z
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 13
File: internal/executor/post_action_executor.go:198-205
Timestamp: 2025-12-04T14:06:51.656Z
Learning: Logger refactor is tracked in HYPERFLEET-304 for the hyperfleet-adapter repository, which will address how CEL evaluation failures and similar errors are handled and logged.
Applied to files:
internal/executor/post_action_executor.go
🧬 Code graph analysis (19)
internal/config_loader/validator_schema.go (1)
internal/config_loader/constants.go (3)
FieldSpec(9-9)FieldResources(20-20)FieldName(31-31)
internal/k8s_client/interface.go (2)
internal/k8s_client/discovery.go (1)
Discovery(14-29)internal/k8s_client/client.go (1)
Client(20-23)
internal/executor/executor_test.go (6)
internal/executor/types.go (6)
ExecutorConfig(48-58)NewExecutionContext(248-260)StatusSuccess(42-42)ExecutionStatus(38-38)NewExecutorError(345-352)ExecutionContext(173-190)internal/config_loader/types.go (7)
AdapterConfig(6-11)Metadata(14-18)Parameter(51-58)Precondition(98-105)Resource(171-178)PostAction(199-203)LogAction(206-209)internal/executor/executor.go (1)
NewExecutor(16-36)internal/criteria/evaluator.go (1)
EvaluationResult(14-25)internal/criteria/types.go (3)
Operator(10-10)OperatorEquals(14-14)OperatorGreaterThan(24-24)internal/config_loader/constants.go (1)
FieldValue(72-72)
test/integration/executor/main_test.go (3)
test/integration/executor/setup_test.go (1)
K8sTestEnv(16-22)test/integration/testutil/container.go (2)
ContainerConfig(20-51)StartSharedContainer(403-495)internal/k8s_client/client.go (2)
NewClientFromConfig(105-115)Client(20-23)
internal/criteria/evaluator_scenarios_test.go (1)
internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
internal/executor/utils_test.go (8)
internal/hyperfleet_api/types.go (1)
Response(159-172)internal/executor/utils.go (3)
ValidateAPIResponse(200-234)ToConditionDefs(21-31)ExecuteLogAction(36-67)pkg/errors/api_error.go (2)
IsAPIError(144-150)NewAPIError(123-134)internal/config_loader/types.go (2)
Condition(131-135)LogAction(206-209)internal/criteria/evaluator.go (1)
ConditionDef(298-302)internal/criteria/types.go (7)
Operator(10-10)OperatorEquals(14-14)OperatorGreaterThan(24-24)OperatorNotEquals(16-16)OperatorLessThan(26-26)OperatorContains(22-22)OperatorIn(18-18)internal/executor/types.go (4)
ExecutionError(238-245)AdapterMetadata(222-235)ExecutionStatus(38-38)ExecutionContext(173-190)internal/executor/resource_executor.go (2)
BuildResourcesMap(488-496)GetResourceAsMap(478-483)
internal/hyperfleet_api/mock.go (1)
internal/hyperfleet_api/types.go (3)
Response(159-172)Request(73-88)RequestOption(91-91)
internal/k8s_client/data_extractor.go (2)
pkg/errors/k8s_error.go (3)
NewK8sInvalidPathError(96-102)NewK8sResourceDataError(128-136)NewK8sResourceKeyNotFoundError(74-81)internal/k8s_client/client.go (1)
Client(20-23)
internal/executor/param_extractor.go (3)
internal/config_loader/types.go (3)
AdapterConfig(6-11)Parameter(51-58)Metadata(14-18)internal/executor/types.go (3)
ExecutionContext(173-190)NewExecutorError(345-352)PhaseParamExtraction(22-22)internal/k8s_client/interface.go (1)
K8sClient(13-47)
test/integration/executor/setup_test.go (2)
internal/k8s_client/client.go (1)
Client(20-23)pkg/logger/logger.go (1)
Logger(11-19)
internal/criteria/evaluator_test.go (1)
internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
internal/k8s_client/mock.go (2)
internal/k8s_client/discovery.go (1)
Discovery(14-29)internal/k8s_client/interface.go (1)
K8sClient(13-47)
pkg/errors/k8s_error.go (1)
pkg/errors/network_error.go (1)
IsNetworkError(24-75)
internal/criteria/cel_evaluator.go (3)
internal/criteria/types.go (1)
EvaluationContext(64-72)pkg/logger/logger.go (1)
Logger(11-19)pkg/errors/cel_error.go (8)
NewCELEnvError(127-132)NewCELParseError(47-54)NewCELProgramError(57-64)NewCELEvalError(67-77)NewCELTypeMismatchError(218-224)NewCELUnsupportedOperatorError(174-180)NewCELUnsupportedTypeError(183-189)NewCELConditionConversionError(192-198)
internal/executor/resource_executor_test.go (1)
pkg/logger/logger.go (1)
NewLogger(32-39)
pkg/errors/network_error_test.go (2)
pkg/errors/network_error.go (1)
IsNetworkError(24-75)pkg/errors/error.go (1)
New(107-123)
internal/criteria/cel_evaluator_test.go (3)
internal/criteria/cel_evaluator.go (1)
NewCELEvaluator(55-73)internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
internal/criteria/evaluator_version_test.go (2)
internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
internal/k8s_client/client.go (2)
pkg/errors/error.go (1)
KubernetesError(203-205)pkg/errors/k8s_error.go (1)
K8sOperationError(16-29)
🪛 markdownlint-cli2 (0.18.1)
internal/executor/README.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (80)
pkg/errors/network_error.go (1)
24-75: Well-structured network error detection with comprehensive coverage.The implementation properly uses
errors.Asfor unwrapping, leverages the stablek8s.io/apimachinery/pkg/util/netutilities, and covers additional syscall error codes. The recursive handling ofnet.OpErroris correct.One minor observation: the
net.Errorinterface check (lines 68-72) only returnstruefor timeout errors. Non-timeoutnet.Errorimplementations that aren't covered by prior checks will returnfalse. This appears intentional based on the documented behavior, but worth noting for future reference.pkg/errors/network_error_test.go (1)
27-334: Comprehensive test coverage for network error detection.The tests cover a wide range of scenarios including direct syscall errors, wrapped errors,
net.OpError,url.Error, timeouts, EOF handling, and real-world simulations. The explicit documentation of expected behavior (e.g., wrapped EOF not detected by utilnet) is helpful.internal/config_loader/types.go (1)
84-95: LGTM - Payload validation correctly enforces mutual exclusivity.The
Validate()method properly ensures that exactly one ofBuildorBuildRefis set, with clear error messages.internal/config_loader/validator_schema.go (2)
13-17: CEL-compatible resource name validation is well-designed.The regex
^[a-z][a-zA-Z0-9_]*$correctly enforces:
- Must start with lowercase letter
- Allows snake_case (
my_resource) and camelCase (myResource)- Disallows kebab-case (hyphens) which would conflict with CEL's minus operator
The comment clearly documents the rationale.
151-172: Resource validation improvements look good.The validation correctly:
- Checks for empty names first
- Validates name format against the CEL-compatible regex
- Detects duplicate resource names with clear error messages
The
seenmap is properly scoped and used.internal/criteria/types.go (2)
62-88: Thread-safe context implementation with version tracking.The
EvaluationContextproperly usessync.RWMutexfor concurrent access. The version tracking with change detection (viareflect.DeepEqual) is a good optimization to avoid unnecessary CEL environment recreation.
172-184: Data() returns shallow copy - verify callers don't mutate values.The
Data()method returns a shallow copy of the map. The map structure itself is independent, but the values remain shared references to the original objects. This is safe for read-only access but poses a risk if any caller mutates nested structures. Verify that the CEL evaluator and all other callers treat the returned map values as read-only. If mutation is possible, consider either: documenting this constraint explicitly, or implementing a deep copy strategy for value types that require isolation.internal/criteria/evaluator_scenarios_test.go (1)
45-45: Constructor updates are consistent with new API signature.All
NewEvaluatorcalls correctly passnilas the second argument (logger) to match the updated signatureNewEvaluator(ctx *EvaluationContext, log logger.Logger).internal/criteria/evaluator_test.go (2)
11-15: Test correctly updated to useData()method.The test now uses
ctx.Data()method instead of direct field access, aligning with the encapsulation changes toEvaluationContext.
727-731: Nil context and logger test is correct.
TestNewEvaluatorWithNilContextproperly verifies thatNewEvaluator(nil, nil)creates a valid evaluator with an initialized context.pkg/errors/k8s_error_test.go (3)
16-19: Helper function is clean and reusable.The
newStatusErrorhelper simplifies test setup for Kubernetes API errors.
256-328: Excellent retry behavior matrix documentation.The
TestIsRetryableDiscoveryError_RetryBehaviorMatrixtest serves dual purposes: testing and documentation. TheexpectedActionfield clearly explains the rationale for each retry decision, which is valuable for operators and developers.
339-423: K8sOperationError tests are comprehensive.The tests cover:
- Error message formatting with/without namespace
- Underlying error unwrapping via
errors.IsIsK8sOperationErrorhelper for both direct and wrapped errorspkg/logger/logger.go (1)
99-116: Good addition ofcopyExtrato prevent shared state bugs.The deep copy mechanism in
copyExtracorrectly prevents mutations from propagating between logger instances when usingV()orWithEventID(). This is a solid fix for potential shared-state issues.internal/executor/post_action_executor_test.go (2)
620-627: Good coverage of template error handling.The "URL template error" test case correctly verifies that missing template variables result in an error. This is important for catching configuration issues early.
17-28: ThetestLoggerimplementation is correct. TheWarningfandErrorfmethods (along withInfof) are standard methods in logger interfaces, following common Go logging patterns used by libraries like logrus and zap. These methods are not dead code—they are part of the expected logger interface contract alongside the string-basedWarningandErrormethods.Likely an incorrect or invalid review comment.
internal/criteria/cel_evaluator_test.go (3)
16-16: Constructor signature updated to accept logger parameter.The test correctly passes
nilfor the logger parameter in unit tests. This aligns with the new signatureNewCELEvaluator(ctx *EvaluationContext, log logger.Logger)shown in the relevant code snippets.
101-117: Clear distinction between parse errors and evaluation errors.The updated test logic correctly handles the two error paths:
- Parse errors are returned as
err(line 104-107)- Evaluation errors are captured in
result.HasError()(line 109)This is a good pattern for allowing callers to distinguish between configuration issues (parse errors) and runtime data issues (evaluation errors).
235-241: Good addition of explicit parse error test.This test case validates that malformed CEL expressions return an actual error rather than being silently captured, ensuring configuration bugs surface clearly.
internal/executor/README.md (1)
1-19: Clear separation of execution status vs business outcomes.The documentation clearly explains the important distinction between process execution status (
success/failed) and business outcomes (resources executed/skipped). This is crucial for operators and developers to understand the system behavior.test/integration/executor/main_test.go (1)
165-168: Default namespace creation correctly handles already-exists error.Good defensive coding - the check for
apierrors.IsAlreadyExists(err)prevents test failures if the namespace already exists from a previous test run or envtest initialization.internal/executor/param_extractor.go (1)
75-100: Robust path traversal with good type handling.The
extractFromEventfunction correctly handles bothmap[string]interface{}andmap[interface{}]interface{}types, which is important since YAML unmarshaling can produce either type. The error messages are clear and include the full path context.internal/executor/executor_test.go (2)
470-586: Comprehensive precondition sequential execution tests.These tests thoroughly validate the stop-on-first-failure behavior for preconditions, correctly distinguishing between execution success and business outcomes (resources skipped). The test cases cover first/middle/last failure positions.
755-829: Good validation of SkipReason capture.These tests verify that when preconditions aren't met, the execution is still marked as
StatusSuccesswithResourcesSkipped=trueand appropriate skip reasons. This aligns with the documented distinction between process execution status and business outcomes.internal/executor/resource_executor_test.go (3)
1-10: LGTM! Comprehensive test coverage for deepCopyMap.The test file provides excellent coverage for the
deepCopyMapfunction, including edge cases like non-JSON-serializable types, nil logger handling, and realistic Kubernetes manifest structures.
131-155: Good documentation of shallow copy danger.This test effectively documents the dangerous behavior when
deepCopyMapfalls back to shallow copy due to non-serializable types. The assertion message on line 153-154 clearly explains the expected (dangerous) behavior.Consider adding a note in the production
deepCopyMapfunction's documentation referencing this behavior to warn callers about the implications of the fallback.
13-39: Verify mockLogger is defined in the package.The test uses
&mockLogger{}but this type could not be verified in the provided context. EnsuremockLoggeris properly defined in the test file or imported from another package before running tests.internal/k8s_client/interface.go (2)
10-47: Well-designed interface with good documentation.The
K8sClientinterface provides a clean abstraction for Kubernetes operations with clear documentation for each method. The separation of CRUD operations, discovery, and data extraction is logical.
49-51: Good practice: Compile-time interface assertion.The
var _ K8sClient = (*Client)(nil)pattern ensures at compile time thatClientimplements theK8sClientinterface, catching implementation drift early.internal/criteria/evaluator.go (4)
40-49: Thread-safe CEL evaluator caching with version tracking.Good implementation of lazy CEL evaluator creation with version tracking. The mutex ensures thread safety, and recreating the evaluator when the context version changes prevents stale environment issues.
320-340: Clean operator dispatch pattern with negate wrapper.The
operatorFuncsmap combined with thenegatewrapper function provides an elegant and maintainable approach for operator evaluation. This reduces code duplication and makes adding new operators straightforward.
238-246: Good use of generics for CEL evaluator wrapper.The
withCELEvaluatorgeneric function elegantly reduces boilerplate acrossEvaluateCEL,EvaluateCELBool, andEvaluateCELStringmethods.
507-520: Reflection-based toFloat64 handles broader numeric types.Using reflection to handle all float, int, and uint kinds is more robust than a type switch with specific types. This correctly handles cases where the concrete type might vary (e.g.,
intvsint64).internal/k8s_client/client.go (2)
117-142: Consistent error handling with proper unwrapping.Good pattern: returning
AlreadyExistserrors unwrapped allows callers to check withapierrors.IsAlreadyExists(err), while other errors are wrapped with rich context viaK8sOperationError.
268-295: Good idempotent delete behavior.Returning
nilforNotFounderrors during delete is the correct idempotent behavior - the desired state (resource doesn't exist) is achieved.internal/executor/post_action_executor.go (2)
148-169: CEL expression error handling now propagates errors correctly.Good improvement: CEL evaluation errors are now returned as errors (line 158) and logged appropriately. The
result.HasError()check on line 160 handles CEL evaluation errors (like missing keys) gracefully by logging them at debug level, which is appropriate for expected conditions like missing status fields.
70-109: Solid payload building logic with proper error propagation.The
buildPostPayloadsfunction correctly handles both inlineBuildandBuildRefContentsources, with appropriate error wrapping and JSON marshaling for template rendering.internal/executor/precondition_executor.go (2)
27-62: Well-structured precondition execution flow.The
ExecuteAllmethod has clear separation between execution errors (API failures) and business outcomes (precondition not satisfied). The early return pattern with accumulated results is appropriate for short-circuit evaluation.
195-220: Robust field capture with nested map traversal.
captureFieldFromDatacorrectly handles bothmap[string]interface{}andmap[interface{}]interface{}(common in YAML parsing), with clear error messages indicating the exact path where traversal failed.test/integration/executor/setup_test.go (3)
83-87: Good: Uses Kubernetes API errors package for type-safe error checking.This addresses the previous review comment - using
apierrors.IsAlreadyExists(err)instead of string matching is robust and properly handles error unwrapping.
47-70: Well-structured test namespace management.The
CreateTestNamespacemethod correctly sets test-specific labels for identification and tolerates already-existing namespaces, which is useful for test reruns.
24-38: Verify shared test environment variables are defined.
setupErrandsharedK8sEnvare referenced but not defined in this file. They should be defined in a companion test setup file (likelymain_test.go).test/integration/executor/executor_k8s_integration_test.go (4)
112-116: Good: Thread-safe getter with defensive copy.
GetStatusResponsescorrectly locks the mutex and returns a copy of the slice, preventing race conditions when reading status responses from tests.
297-419: Comprehensive end-to-end test for resource creation.This test thoroughly validates the full execution flow: preconditions, resource creation (ConfigMap and Secret), and post-action status reporting. The assertions verify both executor results and actual K8s state.
602-684: Good test for recreateOnChange behavior.The test correctly verifies that
RecreateOnChange: trueresults in a new resource with a different UID, confirming the delete+create behavior.
813-944: Clear documentation of test limitations.The TODO comment on lines 816-818 clearly documents that discovery-based update logic is not yet implemented, and the test is scoped accordingly. This is helpful for future maintainers.
internal/executor/executor.go (4)
16-36: LGTM on constructor validation.The
NewExecutorfunction properly validates all required configuration fields and returns structured errors with phase context. The naming follows the establishedNewXxxconvention.
40-74: LGTM on setup phase.Good boundary handling: parsing event data early decouples CloudEvent specifics from downstream processing. The nil event check and context initialization are appropriately defensive.
240-279: LGTM on builder pattern.The fluent builder API is clean and idiomatic. Each
With*method properly returns the builder for chaining.
202-217: Verify that param extraction failures are properly logged before ACKing.The handler silently ACKs events on param extraction failures (line 209) to prevent retries, but malformed events should be logged or exported as metrics to prevent silent data loss. Confirm that:
- Param extraction failures are logged with sufficient detail (event ID, error reason)
- Monitoring/alerting is in place for param extraction failures
- This behavior is intentional and documented for downstream systems
Search the codebase for:
- Logging calls around
PhaseParamExtractionor in param extraction code paths- Metrics/monitoring exported for extraction failures
- Documentation of this ACK-on-failure behavior
internal/criteria/evaluator_version_test.go (3)
100-138: Comprehensive version tracking assertions.The
TestVersionIncrementstest thoroughly covers version behavior: new keys increment, same values don't, value changes do, and merge behavior. This is excellent coverage for the versioning semantics.
10-42: LGTM on context version tracking test.Good test verifying the core use case: adding variables after evaluation should trigger CEL environment recreation. The inline comment about the fix (line 29) provides helpful context.
140-179: Clarify the testing approach for internal evaluator state.The test accesses
evaluator.getCELEvaluator()(lines 154, 164, 174), an unexported method, which is accessible because tests are in the same package. In Go, this is standard practice for white-box testing of internal behavior (like optimization verification). However, consider whether this test coupling is necessary—if the evaluator's recreation behavior is important to verify, it may warrant an exported method or a different testing strategy that validates behavior through the public API instead.internal/hyperfleet_api/mock.go (1)
137-138: LGTM on interface compliance assertion.The compile-time check
var _ Client = (*MockClient)(nil)ensures the mock stays in sync with the interface.internal/k8s_client/data_extractor.go (3)
53-85: LGTM on Secret extraction with base64 decoding.The implementation correctly handles the Kubernetes Secret data format where values are base64-encoded. Error handling covers all edge cases: invalid path, missing resource, missing key, non-string value, and decode failure.
87-113: LGTM on ConfigMap extraction.The ConfigMap extraction correctly handles plain string values without base64 decoding, matching Kubernetes ConfigMap semantics.
35-51: LGTM on GetResourceData helper.Clean separation of resource fetching and data field access. The error wrapping provides good context for debugging.
internal/executor/utils_test.go (6)
18-30: LGTM on success response validation test.Clean test verifying that a 200 OK response with no error returns no error.
87-202: Excellent coverage of non-success status codes.The table-driven test comprehensively covers client errors (4xx) and server errors (5xx), including verification of response body inclusion in the error.
351-417: LGTM on APIError helper method tests.Good coverage of all semantic status code helpers (
IsServerError,IsClientError,IsNotFound, etc.). This ensures the error type API remains consistent.
434-501: LGTM on ToConditionDefs tests.Comprehensive coverage including empty conditions, single condition, multiple conditions with different operators, and all operator types. The camelCase operator naming is tested explicitly.
936-1024: LGTM on BuildResourcesMap tests.Good coverage including nil resources, empty map, single/multiple resources, and nil resource entries being filtered out.
771-782: Verify mockLogger implementation exists.The test uses
&mockLogger{}(line 773) but this type isn't defined in the provided code snippet. Ensure it's defined elsewhere in this file or imported from a test helper package.test/integration/testutil/mock_api_server.go (1)
20-37: LGTM! Clear struct design with appropriate TODO documentation.The struct design is clean with proper mutex for thread safety, and the TODO comment clearly documents the temporary nature of this mock server.
internal/executor/resource_executor.go (4)
34-50: LGTM! Defensive nil check forexecCtx.Resourcesadded.The nil check at line 35-37 properly initializes the Resources map if nil, addressing the potential nil pointer dereference identified in previous reviews.
377-406: LGTM!deepCopyMapnow logs warnings on fallback to shallow copy.The implementation correctly logs warnings when JSON marshaling/unmarshaling fails, addressing the concern from previous reviews about silent fallback behavior.
238-252: Deterministic discovery sorting by generation annotation - good design.The sorting logic ensures deterministic behavior when multiple resources match the label selector by sorting by generation annotation (descending) with fallback to metadata.name. This prevents flaky behavior in discovery.
308-337:waitForDeletionrelies solely on context for timeout — verify callers set appropriate deadlines.The function polls indefinitely until the context is cancelled or the resource is deleted. This is correct behavior, but callers of
recreateResource(and transitivelywaitForDeletion) must ensure contexts have appropriate deadlines to prevent infinite waiting.internal/k8s_client/mock.go (1)
121-122: LGTM! Interface compliance assertion is a good practice.The compile-time interface assertion ensures
MockK8sClientimplementsK8sClientcompletely.pkg/errors/k8s_error.go (2)
160-192: LGTM! Well-structured retryable error detection with clear documentation.The
IsRetryableDiscoveryErrorfunction correctly categorizes:
- Retryable: timeouts, server errors (5xx), rate limiting, network errors
- Non-retryable: auth failures (401/403), bad requests (400), validation errors
- Default: non-retryable for unknown errors (fail-fast approach)
This aligns with the documented behavior and leverages
IsNetworkErrorfromnetwork_error.go.
14-54: LGTM! K8sOperationError provides rich context for debugging K8s failures.The struct captures all relevant context (operation, resource, kind, namespace, message) and properly implements
Unwrap()for error chain inspection witherrors.Is/As.internal/criteria/cel_evaluator.go (3)
119-200: LGTM! Clear error handling strategy with safe evaluation semantics.The
EvaluateSafemethod correctly implements the documented strategy:
- Parse errors: returned as errors (fail-fast for config bugs)
- Program creation errors: returned as errors (fail-fast for invalid expressions)
- Evaluation errors: captured in
CELResult.Error(safe for missing data)This design allows callers to treat missing fields gracefully (e.g., as "not matched") while still failing fast on configuration errors.
202-224: LGTM! GenericEvaluateAs[T]provides type-safe CEL evaluation.The generic function properly handles:
- Propagating
EvaluateSafeerrors- Checking
result.Errorfor captured evaluation errors- Type assertion with descriptive
CELTypeMismatchErroron failure
83-91: Thread-safe data snapshot for CEL environment building.Using
ctx.Data()to get a snapshot ensures thread safety when building CEL options, which aligns with theEvaluationContextdesign that uses mutex-protected data access.internal/executor/types.go (2)
247-260: LGTM!NewExecutionContextproperly initializes all maps.The constructor initializes
Params,Resources, andEvaluationsto empty collections, preventing nil map panics during execution.
325-352: LGTM!ExecutorErrorprovides structured error context with proper unwrapping.The error type captures phase, step, message, and underlying error, with
Unwrap()support forerrors.Is/Ascompatibility.test/integration/executor/executor_integration_test.go (4)
77-203: LGTM! Comprehensive full-flow success test with detailed assertions.The test covers:
- Parameter extraction verification
- Precondition matching and field capture
- Post-action execution and API call verification
- Status payload structure validation with health conditions
1107-1271: LGTM! Thorough test for CEL access toadapter.executionError.The test validates that CEL expressions can access structured error information (
phase,step,message) when execution fails, enabling rich error reporting in post-actions.
38-75: LGTM! Embedded YAML config improves test maintainability.Using
//go:embedfor test configuration separates test data from test logic, making the config easier to maintain and read.
975-1006: Verify theLoggerinterface definition to confirm which methods are required.The review comment claims
logCaptureLoggermay not fully implement theLoggerinterface because it includesWarningfandErrorfmethods that are not present in the interface definition. However, this cannot be verified without access to the actualLoggerinterface inpkg/logger/logger.go. Please inspect the full interface definition to determine whether thefvariant methods are part of the required interface contract.
xueli181114
force-pushed
the
executor-new
branch
from
67155db to
4470c92
Compare
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)
internal/k8s_client/client.go (1)
356-359: Potential error masking in PatchResource.If
Patchsucceeds butGetResourcefails, the caller receives an error despite the patch being applied. Consider documenting this behavior or handling the GetResource error separately.c.log.Infof("Successfully patched resource: %s/%s", gvk.Kind, name) - // Get the updated resource to return + // Get the updated resource to return the server-generated state. + // Note: If this fails, the patch was still applied successfully. return c.GetResource(ctx, gvk, namespace, name)
♻️ Duplicate comments (9)
internal/hyperfleet_api/mock.go (1)
41-57: Avoid sharing a single Response instance across all methods
NewMockClientassigns the samedefaultResponsepointer to all ofDoResponse,GetResponse,PostResponse, etc. Mutating one (e.g.,mock.GetResponse.StatusCode = 404) will unintentionally affect the others, which can confuse tests.Prefer independent responses per method, e.g.:
func NewMockClient() *MockClient { - defaultResponse := &Response{ - StatusCode: 200, - Status: "200 OK", - } return &MockClient{ BaseURLValue: "http://mock-api.example.com", - DoResponse: defaultResponse, - GetResponse: defaultResponse, - PostResponse: defaultResponse, - PutResponse: defaultResponse, - PatchResponse: defaultResponse, - DeleteResponse: defaultResponse, + DoResponse: &Response{StatusCode: 200, Status: "200 OK"}, + GetResponse: &Response{StatusCode: 200, Status: "200 OK"}, + PostResponse: &Response{StatusCode: 200, Status: "200 OK"}, + PutResponse: &Response{StatusCode: 200, Status: "200 OK"}, + PatchResponse: &Response{StatusCode: 200, Status: "200 OK"}, + DeleteResponse: &Response{StatusCode: 200, Status: "200 OK"}, Requests: make([]*Request, 0), } }internal/executor/post_action_executor.go (1)
20-25: Consider adding nil validation for apiClient parameter.The constructor doesn't validate that
apiClientis non-nil. If a nil client is passed, it will cause a panic whenexecuteAPICallattempts to use it.Add validation:
func NewPostActionExecutor(apiClient hyperfleet_api.Client) *PostActionExecutor { + if apiClient == nil { + panic("apiClient is required") + } return &PostActionExecutor{ apiClient: apiClient, } }internal/executor/precondition_executor.go (2)
104-112: Fix misaligned indentation in JSON parse error handling.Same indentation issue as above - the struct literal and return statement need proper indentation.
Apply this diff:
// Set ExecutionError for parse failure - execCtx.Adapter.ExecutionError = &ExecutionError{ - Phase: string(PhasePreconditions), - Step: precond.Name, - Message: err.Error(), - } - - return result, NewExecutorError(PhasePreconditions, precond.Name, "failed to parse API response", err) + execCtx.Adapter.ExecutionError = &ExecutionError{ + Phase: string(PhasePreconditions), + Step: precond.Name, + Message: err.Error(), + } + + return result, NewExecutorError(PhasePreconditions, precond.Name, "failed to parse API response", err) }
86-94: Fix misaligned indentation in error handling block.The
ExecutionErrorstruct literal and thereturnstatement are not properly indented within theif err != nilblock.Apply this diff to fix the indentation:
if err != nil { result.Status = StatusFailed result.Error = err // Set ExecutionError for API call failure - execCtx.Adapter.ExecutionError = &ExecutionError{ - Phase: string(PhasePreconditions), - Step: precond.Name, - Message: err.Error(), - } - - return result, NewExecutorError(PhasePreconditions, precond.Name, "API call failed", err) + execCtx.Adapter.ExecutionError = &ExecutionError{ + Phase: string(PhasePreconditions), + Step: precond.Name, + Message: err.Error(), + } + + return result, NewExecutorError(PhasePreconditions, precond.Name, "API call failed", err) }test/integration/executor/executor_k8s_integration_test.go (2)
63-68: Body reading may truncate large payloads.The single
Readcall with a fixed buffer may not read the entire request body if data exceeds 1MB or arrives in chunks. Useio.ReadAllfor reliable body reading.+import "io" + var bodyStr string if r.Body != nil { - buf := make([]byte, 1024*1024) - n, _ := r.Body.Read(buf) - bodyStr = string(buf[:n]) + bodyBytes, _ := io.ReadAll(r.Body) + bodyStr = string(bodyBytes) }
958-963: Data race when modifying clusterResponse.The
clusterResponsefield is read inside the HTTP handler under mutex (line 92), but modified here without acquiring the lock. This can cause data races and flaky tests.Add a thread-safe setter:
+func (m *k8sTestAPIServer) SetClusterResponse(response map[string]interface{}) { + m.mu.Lock() + defer m.mu.Unlock() + m.clusterResponse = response +} // In the test: - mockAPI.clusterResponse = map[string]interface{}{ + mockAPI.SetClusterResponse(map[string]interface{}{ "metadata": map[string]interface{}{"name": "test-cluster"}, "spec": map[string]interface{}{"region": "us-east-1"}, "status": map[string]interface{}{"phase": "Terminating"}, - } + })test/integration/testutil/mock_api_server.go (1)
73-79: Body reading may truncate large payloads.Same issue as flagged before - single
Readcall may not capture the entire body.+import "io" + // Read body var bodyStr string if r.Body != nil { - buf := make([]byte, 1024*1024) - n, _ := r.Body.Read(buf) - bodyStr = string(buf[:n]) + bodyBytes, _ := io.ReadAll(r.Body) + bodyStr = string(bodyBytes) }internal/executor/types.go (1)
307-323:SetSkippedstill discards thereasonwhenmessageis provided
SetSkippedfirst setsSkipReason = reasonand then unconditionally overwrites it withmessagewhenmessageis non-empty, so callers passing both lose the original reason code entirely. If you want a code + human-friendly detail, consider either a separateSkipMessagefield or concatenating both (e.g."reason: message").test/integration/executor/executor_integration_test.go (1)
658-721: Directos.Unsetenvrisks cross-test pollutionThis test explicitly unsets
HYPERFLEET_API_BASE_URLafter executor creation. That mutates global process state and could impact other tests if they ever run in parallel or rely on that env var. Usingt.Setenv("HYPERFLEET_API_BASE_URL", "")for this test case (or restoring viat.Cleanup) would avoid the leak while preserving the behavior under test.
🧹 Nitpick comments (12)
internal/hyperfleet_api/mock.go (1)
69-77: Apply RequestOptions in MockClient verb helpers
Get/Post/Put/Patch/Deletetakeopts ...RequestOptionbut never apply them to the constructedRequest. That means things likeWithHeader, custom timeouts, or backoff overrides won’t be reflected inMockClient.Requests, diverging from realClientbehavior and weakening tests that assert on those fields.Consider applying options before recording/returning:
func (m *MockClient) Get(ctx context.Context, url string, opts ...RequestOption) (*Response, error) { - req := &Request{Method: "GET", URL: url} + req := &Request{Method: "GET", URL: url} + for _, opt := range opts { + opt(req) + } m.Requests = append(m.Requests, req) ... }(and similarly for
Post,Put,Patch, andDelete).Also applies to: 80-87, 90-97, 100-107, 109-117
pkg/errors/k8s_error.go (1)
104-136: Consider wiringFieldinto K8sResourceDataError’s constructor/message
K8sResourceDataErrorhas aFieldmember (e.g.,"data"or a specific key), but:
NewK8sResourceDataErrordoesn’t accept afieldargument or populate it.Error()doesn’t includeFieldin the formatted message.Callers currently have to remember to set
Fieldmanually (if at all), and that context is missing from the error string.Consider either:
- Extending the constructor to take
field stringand set it, and/or- Including
Fieldin theError()output when non-empty.This will make debugging data-path issues much easier.
internal/hyperfleet_api/client_test.go (2)
16-18: Testify integration and base-URL error test look good (env cleanup slightly redundant)Using
require.NoError/require.NotNilinTestNewClientand asserting the “missing base URL” error viarequire.Error/assert.Containsis a nice improvement.In
TestNewClientMissingBaseURL, the combination ofLookupEnv+t.Setenv+os.Unsetenv+t.Cleanupcorrectly restores state, butt.Setenvalone (with the original value captured implicitly) could likely replace the manualCleanup. Not critical, just a simplification opportunity.Also applies to: 23-24, 27-48
145-145: Consistent use of require for client construction improves test clarityAcross the HTTP verb, retry, timeout, and APIError integration tests, switching to
require.NoErroronNewClient(...)(and similar setup steps) ensures early failure and avoids nil‑client follow-on errors. The patterns look consistent and correct throughout.Also applies to: 178-178, 210-210, 249-249, 284-284, 315-315, 345-345, 360-360, 463-463, 484-484, 505-505, 635-635
internal/executor/resource_executor_test.go (1)
57-64: Consider adding type assertion safety comments.These type assertions will panic if the structure doesn't match expectations. While acceptable in tests with controlled input, a brief comment explaining this is intentional test setup behavior would improve clarity.
internal/k8s_client/client.go (1)
206-216: Consider resource vs kind for ListResources error.Both
ResourceandKindfields are set togvk.Kind. While not incorrect,Resourcetypically refers to an instance name. For list operations without a specific resource name, this is acceptable but could use a comment.return nil, &apperrors.K8sOperationError{ Operation: "list", - Resource: gvk.Kind, + Resource: "", // List operation has no specific resource name Kind: gvk.Kind, Namespace: namespace, Message: err.Error(), Err: err, }internal/criteria/types.go (1)
187-196: Shallow copy is documented but consider deep copy for nested structures.The shallow copy prevents modification of the map itself, but if values contain mutable types (nested maps, slices), external code could still mutate them. If this context may hold nested mutable data, consider a deep copy or document this limitation more explicitly for callers.
internal/executor/precondition_executor.go (1)
20-25: Consider adding nil validation for apiClient parameter.Similar to
PostActionExecutor, this constructor doesn't validate thatapiClientis non-nil, which could cause nil pointer dereferences later.func NewPreconditionExecutor(apiClient hyperfleet_api.Client) *PreconditionExecutor { + if apiClient == nil { + panic("apiClient is required") + } return &PreconditionExecutor{ apiClient: apiClient, } }internal/k8s_client/data_extractor.go (1)
21-33: Consider documenting path format edge cases.The path parsing allows dots in the key portion (line 31), which is useful for nested keys. However, this means namespace and resource name cannot contain dots. Consider adding validation or documenting this constraint.
// ParseResourcePath parses a path in the format: namespace.name.key +// Note: Namespace and resource name must not contain dots. +// The key portion can contain dots (e.g., "default.my-configmap.app.config.key"). func ParseResourcePath(path, resourceType string) (*ResourcePath, error) {internal/executor/resource_executor.go (1)
308-337: Consider making poll interval configurable or adding a max wait time.The hardcoded 100ms poll interval is reasonable for tests, but in production scenarios with slow K8s API or finalizers, this could lead to excessive polling or context timeout without clear feedback.
Consider adding a warning log after extended wait times:
+const pollInterval = 100 * time.Millisecond +const warnAfterWait = 5 * time.Second + startTime := time.Now() + warnLogged := false for { select { case <-ctx.Done(): // ... case <-ticker.C: + if !warnLogged && time.Since(startTime) > warnAfterWait { + log.Warning(fmt.Sprintf("Still waiting for deletion of %s/%s after %v", gvk.Kind, name, time.Since(startTime))) + warnLogged = true + } _, err := re.k8sClient.GetResource(ctx, gvk, namespace, name)internal/criteria/cel_evaluator.go (1)
202-259: Type-safeEvaluateAshelpers are clear, but strictly enforce concrete Go types
EvaluateAs[T]+ the wrappers (EvaluateBool,EvaluateString, etc.) make call sites much cleaner and centralize error typing, but they rely onresult.Valuebeing a concrete Go type exactly matchingT. If in future you introduce custom CEL type adapters or non-standard return types, this may surface moreCELTypeMismatchErrors than expected. For now, the behavior is consistent and explicit.test/integration/executor/executor_integration_test.go (1)
625-656: Consider adding concrete assertions to the context cancellation test
TestExecutor_ContextCancellationcurrently just logs the result with a cancelled context. Adding at least one assertion (e.g., non-success status or a specific phase) would prevent regressions in how cancellation is honored.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (48)
README.md(1 hunks)configs/adapter-config-template.yaml(1 hunks)go.mod(3 hunks)internal/config_loader/accessors.go(1 hunks)internal/config_loader/types.go(3 hunks)internal/config_loader/validator_schema.go(2 hunks)internal/config_loader/validator_test.go(1 hunks)internal/criteria/README.md(7 hunks)internal/criteria/cel_evaluator.go(8 hunks)internal/criteria/cel_evaluator_test.go(12 hunks)internal/criteria/evaluator.go(6 hunks)internal/criteria/evaluator_scenarios_test.go(7 hunks)internal/criteria/evaluator_test.go(14 hunks)internal/criteria/evaluator_version_test.go(1 hunks)internal/criteria/types.go(2 hunks)internal/executor/README.md(1 hunks)internal/executor/executor.go(1 hunks)internal/executor/executor_test.go(1 hunks)internal/executor/param_extractor.go(1 hunks)internal/executor/post_action_executor.go(1 hunks)internal/executor/post_action_executor_test.go(1 hunks)internal/executor/precondition_executor.go(1 hunks)internal/executor/resource_executor.go(1 hunks)internal/executor/resource_executor_test.go(1 hunks)internal/executor/types.go(1 hunks)internal/executor/utils.go(1 hunks)internal/executor/utils_test.go(1 hunks)internal/hyperfleet_api/client_test.go(17 hunks)internal/hyperfleet_api/mock.go(1 hunks)internal/k8s_client/README.md(2 hunks)internal/k8s_client/client.go(11 hunks)internal/k8s_client/data_extractor.go(1 hunks)internal/k8s_client/interface.go(1 hunks)internal/k8s_client/mock.go(1 hunks)pkg/errors/cel_error.go(1 hunks)pkg/errors/k8s_error.go(1 hunks)pkg/errors/k8s_error_test.go(1 hunks)pkg/errors/network_error.go(1 hunks)pkg/errors/network_error_test.go(1 hunks)pkg/logger/logger.go(3 hunks)pkg/logger/logger_test.go(2 hunks)test/integration/config-loader/config_criteria_integration_test.go(8 hunks)test/integration/executor/executor_integration_test.go(1 hunks)test/integration/executor/executor_k8s_integration_test.go(1 hunks)test/integration/executor/main_test.go(1 hunks)test/integration/executor/setup_test.go(1 hunks)test/integration/executor/testdata/test-adapter-config.yaml(1 hunks)test/integration/testutil/mock_api_server.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/executor/executor_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- go.mod
- internal/config_loader/validator_test.go
- internal/criteria/README.md
- internal/config_loader/validator_schema.go
- README.md
- internal/criteria/evaluator_scenarios_test.go
- pkg/errors/network_error.go
- configs/adapter-config-template.yaml
- internal/executor/utils.go
- internal/executor/executor.go
- internal/k8s_client/mock.go
- pkg/errors/cel_error.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-04T14:06:51.656Z
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 13
File: internal/executor/post_action_executor.go:198-205
Timestamp: 2025-12-04T14:06:51.656Z
Learning: Logger refactor is tracked in HYPERFLEET-304 for the hyperfleet-adapter repository, which will address how CEL evaluation failures and similar errors are handled and logged.
Applied to files:
internal/executor/post_action_executor.go
🧬 Code graph analysis (21)
internal/hyperfleet_api/client_test.go (2)
internal/hyperfleet_api/client.go (2)
NewClient(117-145)WithBaseURL(101-105)internal/hyperfleet_api/types.go (1)
WithHeader(106-113)
internal/executor/post_action_executor_test.go (5)
internal/criteria/evaluator.go (1)
NewEvaluator(52-60)internal/config_loader/types.go (3)
PostAction(199-203)APICall(108-116)Header(119-122)internal/hyperfleet_api/mock.go (1)
NewMockClient(43-58)internal/executor/post_action_executor.go (1)
NewPostActionExecutor(21-25)internal/executor/utils.go (1)
ExecuteAPICall(73-199)
internal/executor/resource_executor_test.go (1)
pkg/logger/logger.go (1)
NewLogger(32-39)
internal/executor/post_action_executor.go (6)
internal/config_loader/types.go (4)
PostConfig(193-196)Payload(67-78)PostAction(199-203)APICall(108-116)internal/executor/types.go (1)
ExecutionContext(173-190)pkg/logger/logger.go (1)
Logger(11-19)internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (2)
NewEvaluator(52-60)Evaluator(40-49)internal/executor/utils.go (3)
ExecuteLogAction(36-67)ExecuteAPICall(73-199)ValidateAPIResponse(204-238)
test/integration/config-loader/config_criteria_integration_test.go (1)
internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
pkg/errors/k8s_error_test.go (1)
pkg/errors/k8s_error.go (3)
IsRetryableDiscoveryError(160-192)K8sOperationError(16-29)IsK8sOperationError(48-54)
internal/criteria/evaluator_test.go (2)
internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
pkg/errors/network_error_test.go (2)
pkg/errors/network_error.go (1)
IsNetworkError(24-75)pkg/errors/error.go (1)
New(107-123)
test/integration/executor/main_test.go (3)
test/integration/executor/setup_test.go (1)
K8sTestEnv(16-22)pkg/logger/logger.go (1)
NewLogger(32-39)internal/k8s_client/client.go (2)
NewClientFromConfig(105-115)Client(20-23)
internal/hyperfleet_api/mock.go (2)
internal/hyperfleet_api/types.go (3)
Response(159-172)Request(73-88)RequestOption(91-91)internal/k8s_client/client.go (1)
Client(20-23)
internal/criteria/cel_evaluator_test.go (3)
internal/criteria/cel_evaluator.go (1)
NewCELEvaluator(55-73)internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
test/integration/executor/setup_test.go (2)
internal/k8s_client/client.go (1)
Client(20-23)pkg/logger/logger.go (1)
Logger(11-19)
internal/executor/types.go (9)
internal/config_loader/types.go (2)
AdapterConfig(6-11)Resource(171-178)internal/k8s_client/client.go (1)
Client(20-23)internal/k8s_client/interface.go (1)
K8sClient(13-47)pkg/logger/logger.go (1)
Logger(11-19)internal/executor/precondition_executor.go (1)
PreconditionExecutor(16-18)internal/executor/resource_executor.go (1)
ResourceExecutor(21-23)internal/executor/post_action_executor.go (1)
PostActionExecutor(16-18)internal/criteria/evaluator.go (1)
EvaluationResult(14-25)internal/criteria/cel_evaluator.go (1)
CELResult(26-42)
internal/executor/utils_test.go (8)
internal/hyperfleet_api/types.go (1)
Response(159-172)internal/executor/utils.go (3)
ValidateAPIResponse(204-238)ToConditionDefs(21-31)ExecuteLogAction(36-67)pkg/errors/api_error.go (2)
IsAPIError(144-150)NewAPIError(123-134)internal/config_loader/types.go (2)
Condition(131-135)LogAction(206-209)internal/criteria/evaluator.go (1)
ConditionDef(298-302)internal/criteria/types.go (7)
Operator(10-10)OperatorEquals(14-14)OperatorGreaterThan(24-24)OperatorNotEquals(16-16)OperatorLessThan(26-26)OperatorContains(22-22)OperatorIn(18-18)internal/executor/types.go (3)
ExecutionError(238-245)AdapterMetadata(222-235)ExecutionStatus(38-38)internal/executor/resource_executor.go (2)
BuildResourcesMap(492-500)GetResourceAsMap(482-487)
internal/k8s_client/data_extractor.go (2)
pkg/errors/k8s_error.go (3)
NewK8sInvalidPathError(96-102)NewK8sResourceDataError(128-136)NewK8sResourceKeyNotFoundError(74-81)internal/k8s_client/client.go (1)
Client(20-23)
test/integration/executor/executor_k8s_integration_test.go (3)
internal/config_loader/types.go (7)
AdapterConfig(6-11)Resource(171-178)DiscoveryConfig(181-185)PostConfig(193-196)Payload(67-78)PostAction(199-203)SelectorConfig(188-190)internal/hyperfleet_api/client.go (2)
WithTimeout(66-70)WithRetryAttempts(73-77)internal/executor/types.go (5)
StatusSuccess(42-42)OperationCreate(143-143)OperationUpdate(145-145)OperationRecreate(147-147)PhasePostActions(28-28)
internal/executor/param_extractor.go (3)
internal/config_loader/types.go (3)
AdapterConfig(6-11)Parameter(51-58)Metadata(14-18)internal/executor/types.go (3)
ExecutionContext(173-190)NewExecutorError(345-352)PhaseParamExtraction(22-22)internal/k8s_client/interface.go (1)
K8sClient(13-47)
pkg/errors/k8s_error.go (1)
pkg/errors/network_error.go (1)
IsNetworkError(24-75)
internal/k8s_client/client.go (2)
pkg/errors/error.go (1)
KubernetesError(203-205)pkg/errors/k8s_error.go (1)
K8sOperationError(16-29)
test/integration/executor/executor_integration_test.go (4)
test/integration/testutil/mock_api_server.go (2)
NewMockAPIServer(48-142)MockRequest(14-18)internal/executor/types.go (7)
StatusSuccess(42-42)ExecutionContext(173-190)ExecutionStatus(38-38)StatusFailed(44-44)ExecutionResult(69-94)PhaseParamExtraction(22-22)PhasePostActions(28-28)internal/criteria/cel_evaluator.go (1)
CELResult(26-42)pkg/logger/logger.go (1)
Logger(11-19)
internal/criteria/evaluator_version_test.go (2)
internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
🪛 markdownlint-cli2 (0.18.1)
internal/executor/README.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (75)
internal/k8s_client/README.md (3)
17-17: Add executor package reference with helpful pointer to high-level operations.The change clearly guides users to the appropriate abstraction layer. This is helpful separation of concerns.
319-319: Best practice update is well-placed and clear.Adding executor to best practices reinforces the recommended usage pattern for users implementing resource management workflows.
323-324: Related packages section updated appropriately.The reorganization clarifies the intended usage:
executorfor high-level orchestration andconfig_loaderfor adapter configuration parsing. Clear and helpful for navigation.Please verify that the executor package's actual behavior (based on the PR's implementation) aligns with the documented descriptions of "template rendering," "resource discovery," and "lifecycle management." Since this README will guide users to the executor, it's important to ensure the descriptions match the implementation accurately.
internal/config_loader/accessors.go (1)
30-37: Comment now matches implementation of post payload variablesThe docstring correctly reflects that
GetDefinedVariablesincludes post payload names fromSpec.Post.Payloads. No functional issues.pkg/errors/network_error_test.go (1)
1-334: Comprehensive IsNetworkError test matrix looks solidThe table-driven scenarios (syscall, wrapped, net.OpError/url.Error, timeouts, EOFs, non-network, and “real world” cases) align well with
IsNetworkError’s behavior and should guard against regressions.internal/config_loader/types.go (1)
49-58: New config types (Payload, logging) and docs look consistent
Parameterdocs now accurately describe extraction viaSource.PayloadplusValidatecleanly enforce the “exactly one of Build/BuildRef” rule with appropriate YAML tags.- Adding
Log *LogActiontoPrecondition/PostActionand definingLogActionis straightforward and backwards‑compatible.Also applies to: 60-95, 97-105, 192-209
pkg/errors/k8s_error.go (1)
14-55: Structured K8s operation errors and retry classifier look well-designed
K8sOperationError(withUnwrap) andIsK8sOperationErrorprovide good context + error-chain support, andIsRetryableDiscoveryErrorcleanly separates transient API/server/network issues from fatal ones, with a conservative default of non‑retryable for unknowns. This should integrate nicely with the k8s client and executor flows.Also applies to: 142-192
pkg/errors/k8s_error_test.go (1)
1-423: Thorough coverage of K8s discovery and operation error behaviorThe tests exercise all key branches of
IsRetryableDiscoveryError(including wrapped errors and NotFound semantics) and validateK8sOperationError’s formatting and unwrapping. This gives strong confidence in the new error types and retry classification.test/integration/config-loader/config_criteria_integration_test.go (1)
63-63: Evaluator constructor updates align with new signatureAll
criteria.NewEvaluatorcall sites in these integration tests now pass an explicitnillogger, matching the updatedNewEvaluator(ctx, log)signature while preserving existing behavior. No issues spotted.Also applies to: 150-150, 184-184, 206-206, 228-228, 315-315, 377-377, 407-407
internal/hyperfleet_api/client_test.go (1)
530-532: APIError assertion format strings are now correctly parameterizedThe
assert.Containscalls inTestAPIErrorandTestAPIErrorInRetryExhaustednow passerrStr/bodyStr/ResponseBodyString()as arguments to match the%splaceholders in the messages, fixing the earlier formatting issue and yielding useful failure output.Also applies to: 547-547, 656-656
pkg/logger/logger_test.go (1)
167-173: LGTM! Test updates align with the API change.The test updates correctly reflect the transition from OpIDKey to EvtIDKey, with proper assertions for the new context key and expected string value "evt_id".
Also applies to: 290-292
pkg/logger/logger.go (2)
99-109: LGTM! Deep copy prevents shared state bugs.The
copyExtrahelper correctly creates a deep copy of the extra map, and its use inV()at line 115 prevents shared state mutations when chaining logger calls.Also applies to: 115-115
170-185: LGTM! WithEventID handles multiple logger types correctly.The function properly type-asserts internal loggers to add event ID to context, while gracefully returning non-internal loggers unchanged. This allows flexibility with test loggers.
internal/criteria/evaluator_test.go (1)
113-155: LGTM! Valuable concurrency test for deadlock prevention.The test validates that concurrent cross-merges don't deadlock by exercising both
ctx1.Merge(ctx2)andctx2.Merge(ctx1)simultaneously. The test correctly verifies that both contexts receive merged data after 100 iterations in each goroutine.test/integration/executor/testdata/test-adapter-config.yaml (1)
1-89: LGTM! Well-structured test configuration.The test adapter config properly exercises the executor pipeline with parameter extraction, preconditions with API calls and capture, empty resources for dry-run mode, and post-actions with CEL expressions for status reporting. This provides good end-to-end test coverage.
internal/executor/post_action_executor_test.go (2)
17-28: LGTM! Proper test logger implementation.The testLogger correctly implements all methods of the logger.Logger interface with no-op implementations suitable for testing.
30-664: LGTM! Comprehensive test coverage.The test suite provides excellent coverage of post-action executor functionality including:
- Payload building with various types and templates
- Map payload handling with value definitions
- Value processing with CEL expressions
- API call execution across all HTTP methods
- Error handling and edge cases
internal/executor/param_extractor.go (2)
15-41: LGTM! Correct parameter extraction with proper error handling.The function correctly handles required vs. non-required parameters, applies defaults appropriately, and returns detailed errors when required parameters fail extraction.
76-100: LGTM! Robust path traversal with detailed errors.The function handles both
map[string]interface{}andmap[interface{}]interface{}types, providing detailed error messages with the full path context when fields are not found or parent types are incorrect.test/integration/executor/main_test.go (2)
35-99: LGTM! Proper TestMain setup with graceful error handling.The TestMain function correctly handles prerequisite checks (short tests, env vars, container runtime), sets up shared test infrastructure, and ensures proper cleanup after all tests complete.
131-137: LGTM! Insecure TLS config is appropriate for test environment.The insecure TLS configuration with bearer token authentication is intentional and suitable for the envtest environment, which uses self-signed certificates for testing purposes.
internal/criteria/cel_evaluator_test.go (4)
16-18: LGTM on constructor update.The test correctly uses the new two-parameter constructor
NewCELEvaluator(ctx, nil)withnillogger, aligning with the updated API signature.
101-117: Appropriate error handling for parse vs evaluation errors.The test logic correctly distinguishes between parse errors (returned as
err) and evaluation errors (captured inresult.HasError()). This aligns well with the new EvaluateSafe semantics.
170-178: Good test coverage for safe evaluation semantics.The test correctly verifies that evaluation errors (missing fields) are captured in the result rather than returned as errors, with appropriate assertions on
ErrorReasoncontaining "no such key".
235-241: Important test for parse error behavior.This test ensures parse errors are surfaced as actual errors (not captured in result), which is the correct behavior for distinguishing bugs (invalid syntax) from runtime conditions (missing fields).
internal/executor/resource_executor_test.go (5)
12-38: Comprehensive test for basic type deep copying.The test properly verifies value preservation, mutation isolation, and absence of warnings. Good coverage of primitive types.
90-112: Good edge case coverage for channel handling.Tests verify that
copystructurehandles channels properly without warnings, which is important for documenting the library's behavior with non-copyable types.
173-193: Verify nil logger doesn't panic.Good defensive test ensuring
deepCopyMaphandles nil logger gracefully. This is important for robustness.
240-262: Realistic integration test.Using
logger.NewLogger(context.Background())provides good coverage for the actual usage pattern in production code.
13-13: Verify mockLogger is defined.The test uses
&mockLogger{}but I was unable to access the repository to confirm the type definition. Manual verification is required to check whethermockLoggeris defined in the test file itself, imported from another package, or potentially missing.internal/k8s_client/interface.go (3)
10-47: Well-designed interface for Kubernetes abstraction.The interface provides a clean abstraction for testing without real clusters. Good documentation on each method, clear separation between CRUD operations, discovery, and data extraction.
49-50: Good use of compile-time interface assertion.This ensures
ClientimplementsK8sClientat compile time, catching implementation drift early.
36-36: Verify Discovery type is defined.The
DiscoverResourcesmethod references aDiscoverytype parameter. Ensure theDiscoverytype is defined or properly imported in theinternal/k8s_clientpackage.internal/criteria/evaluator.go (5)
66-83: Thread-safe CEL evaluator caching with version tracking.Good implementation of lazy initialization with version-based invalidation. The mutex properly guards the cached evaluator and version.
238-246: Clean generic helper for CEL evaluator access.The
withCELEvaluatorhelper reduces repetition and provides a consistent pattern for accessing the cached evaluator.
320-340: Good refactor using operator dispatch table.The
operatorFuncsmap andnegatewrapper provide a cleaner, more extensible approach than a switch statement. Easy to add new operators.
508-520: Reflection-based numeric conversion handles more types.Using
reflect.Kindswitch handles all integer and float variants uniformly, which is more robust than explicit type switches. This properly handles type aliases built on numeric types.
70-70: Verify EvaluationContext.Version() is thread-safe.
e.context.Version()is called before acquiring the lock for the CEL evaluator check. IfVersion()involves reads of mutable state that could race withSet()operations, this could be problematic. Unable to verify without access to theEvaluationContext.Version()implementation and the full context around this code section.internal/k8s_client/client.go (3)
126-138: Consistent error handling for CreateResource.AlreadyExists errors are passed through for caller checking, while other errors are wrapped with structured
K8sOperationError. Good pattern.
156-170: Appropriate error wrapping for GetResource.NotFound errors preserved for caller checking with
apierrors.IsNotFound(), other errors wrapped with operation context.
268-295: Good idempotent delete handling.Treating NotFound as success (logging "already deleted") is the correct approach for idempotent delete operations.
internal/criteria/types.go (3)
62-72: Well-designed concurrency-safe context with version tracking.The struct design with private
data,version, andmufields properly encapsulates state and enables thread-safe operations. The version tracking mechanism enables efficient CEL evaluator cache invalidation.
93-104: LGTM - Set method with change detection optimization.The use of
reflect.DeepEqualto avoid unnecessary version increments is a good optimization that prevents CEL evaluator recreation when values haven't actually changed.
130-159: Deadlock-safe Merge implementation.The snapshot approach correctly addresses the bidirectional merge deadlock risk by copying
other's data while holding only its read lock, then releasing it before acquiring the write lock onself. This is the recommended pattern.internal/executor/post_action_executor.go (3)
27-68: LGTM - Well-structured sequential execution with proper error handling.The two-phase approach (build payloads, then execute actions) with early exit on failure and proper
ExecutionErrorpropagation is clean and follows the documented behavior.
148-204: LGTM - Comprehensive value processing with proper error handling.The recursive handling of maps, lists, expressions, and value definitions is well-implemented. CEL evaluation errors are properly propagated, and the debug-level logging for
HasError()cases (like missing keys during evaluation) is appropriate. Based on learnings, this aligns with the logger refactor tracked in HYPERFLEET-304.
232-258: LGTM - Robust API call execution with proper error handling.Good defensive programming by capturing response details even when errors occur. The validation and structured error creation provide useful context for debugging failures.
test/integration/executor/setup_test.go (3)
47-81: LGTM - Test namespace management with proper error handling.The namespace creation correctly ignores
AlreadyExistserrors using the type-safeapierrors.IsAlreadyExists, and cleanup appropriately logs warnings rather than failing tests on cleanup errors. The test labels enable easy identification and manual cleanup if needed.
83-87: LGTM - Type-safe error checking.Using
apierrors.IsAlreadyExistsinstead of string matching is the correct approach, as it properly unwraps error chains and checks the status reason.
29-37: Verify thatsharedK8sEnvandsetupErrare defined in a TestMain or init.The function references
setupErrandsharedK8sEnvwhich appear to be package-level variables. Ensure these are properly initialized in aTestMainfunction (standard Go testing pattern) or similar initialization mechanism.internal/executor/precondition_executor.go (2)
195-220: LGTM - Robust field capture with dot notation.The function correctly handles both
map[string]interface{}andmap[interface{}]interface{}(common with YAML unmarshaling), and provides helpful error messages that include the path context.
27-62: LGTM - Clean separation of execution errors from business outcomes.The distinction between execution errors (API failures) and business outcomes (precondition not met) with a
NotMetReasonfield provides clear semantics for callers.internal/criteria/evaluator_version_test.go (3)
10-42: LGTM - Comprehensive version tracking test.This test validates the critical behavior that CEL evaluators are recreated when context variables change after initial evaluation. The test correctly verifies that new variables are accessible in subsequent evaluations.
100-138: LGTM - Thorough version increment behavior tests.Excellent coverage testing both increment cases (new key, value change) and no-increment cases (same value). This validates the optimization that prevents unnecessary CEL evaluator recreation.
140-179: Unable to verify method existence - cannot access repository.The test's approach to verify caching behavior using pointer identity (
assert.Same/assert.NotSame) is sound, but verification that thegetCELEvaluator()method exists on theEvaluatortype with the expected signature could not be completed.test/integration/executor/executor_k8s_integration_test.go (1)
297-419: LGTM! Comprehensive integration test coverage.The
TestExecutor_K8s_CreateResourcestest provides thorough end-to-end validation including:
- Resource creation verification via K8s API
- ConfigMap data and label assertions
- Post-action status payload verification with CEL expression evaluation
The test structure with proper setup/cleanup and detailed logging will help with debugging.
internal/executor/utils_test.go (4)
18-30: LGTM! Well-structured API response validation tests.The tests thoroughly cover:
- Nil error with success/nil responses
- APIError preservation and wrapping behavior
- Status code handling across 4xx and 5xx ranges
434-501: LGTM! Comprehensive condition definition conversion tests.Tests cover empty conditions, single conditions, multiple conditions with various operator types, ensuring proper mapping from config_loader types to criteria types.
936-1024: LGTM! Resource map building tests cover essential scenarios.Good coverage of nil resources, empty maps, single/multiple resources, and nil entries within the map. This ensures CEL evaluation receives properly structured data.
771-781: Verify mockLogger is defined and properly implements the logger.Logger interface.The test uses
&mockLogger{}but its definition is not visible in the provided snippet. Ensure the mock is properly defined in this or another test file and implements all required methods of thelogger.Loggerinterface.internal/k8s_client/data_extractor.go (2)
53-85: LGTM! Secret extraction with proper base64 decoding.The implementation correctly:
- Parses the resource path
- Fetches secret data via unstructured API
- Validates key existence and type
- Decodes base64-encoded secret values
Error handling is comprehensive with specific error types for each failure mode.
87-113: LGTM! ConfigMap extraction implementation.Clean implementation that correctly handles ConfigMap data as plain strings (no decoding needed, unlike Secrets).
test/integration/testutil/mock_api_server.go (2)
168-173: LGTM! Thread-safe setter for cluster response.This is the correct pattern - acquiring mutex before modifying shared state. The
k8sTestAPIServerin the integration test file should follow this same pattern.
203-225: LGTM! Reset method provides clean test isolation.The
Resetmethod properly reinitializes all mutable state under the mutex, which is important for test isolation when running multiple tests against the same server instance.internal/executor/resource_executor.go (4)
34-50: LGTM! Defensive initialization for Resources map.The nil check at line 35-37 addresses the previous review concern about potential nil pointer dereference. This ensures the map is always initialized before use.
78-93: Good error classification for discovery operations.The distinction between retryable errors (transient/network) and fatal errors (auth/permission/validation) ensures appropriate behavior:
- Transient errors log and continue (fallback to create)
- Fatal errors fail fast with clear error context
372-410: LGTM! Robust deep copy with fallback and logging.The implementation using
copystructurehandles non-JSON-serializable types correctly. The fallback to shallow copy with warning logging (addressing past review) ensures visibility when the copy might not be fully isolated.
241-251: Good deterministic selection when multiple resources match.Sorting by generation annotation (descending) with secondary sort by name ensures consistent behavior regardless of the order K8s API returns results. This is important for avoiding flaky behavior in production.
internal/criteria/cel_evaluator.go (2)
54-73: EvaluateSafe env construction and logging look solidUsing
buildCELOptions(ctx)plusctx.Data()snapshots, typed apperrors for env/parse/program failures, and logging eval errors vialog.V(2).Infofgives a clear separation between config bugs and runtime data issues. This is a nice, testable error model.Also applies to: 80-91, 133-200
286-387: CEL conversion helpers now return precise error types – good for debugging
ConditionToCEL,formatCELValue, andConditionsToCELnow funnel unsupported operators/types/index failures intoNewCELUnsupportedOperatorError,NewCELUnsupportedTypeError, andNewCELConditionConversionError. That gives much better diagnostics to callers and aligns well with the executor’s structured error handling. No issues spotted.internal/executor/types.go (2)
68-116: ExecutionResult / ExecutionContext / AdapterMetadata design is coherentThe split between
ExecutionResult(external outcome),ExecutionContext(internal state & evaluations), andAdapterMetadata/ExecutionError(CEL-visible, serializable error surface) is clean and matches the four-phase executor model. The fields used in tests (status, phase, params, per-phase results, adapter metadata) are all present and named consistently.Also applies to: 172-190, 221-245
325-352:ExecutorErrorshape and formatting are appropriateWrapping phase/step/message plus an underlying
errorand implementingError()/Unwrap()the standard way will make this easy to log and to use witherrors.Is/As. This matches how it’s consumed in the tests viaExecutionError.test/integration/executor/executor_integration_test.go (3)
38-75: Embedded adapter config +createTestConfigkeep tests focusedUsing
//go:embedand a singlecreateTestConfighelper makes the integration tests much easier to read and maintain. KeepingapiBaseURLas a no-op parameter for compatibility is fine as long as callers know all runtime wiring comes fromHYPERFLEET_API_BASE_URL.
831-973: Log-capture helper cleanly verifies config-driven logging
TestExecutor_LogActionpluslogCaptureLoggergive good coverage for precondition/post-actionLogActions and ensure the config-sourced messages actually reach the logger. The helper correctly implementslogger.Loggerand serializes messages in a way that’s easy to assert on.Also applies to: 975-1005
1007-1105: Integration tests thoroughly exercise executor error surfacesThe post-action API failure,
adapter.executionErrorCEL access, and payload-build-failure tests collectively validate the structured error model (status/phase,ExecutionErrorfields, and blocking semantics for post actions). Assertions on phase, step, HTTP status, and payload content look precise and aligned with the new executor types.Also applies to: 1107-1271, 1273-1402
| ``` | ||
| ┌──────────────────────────────────────────────────────────────────────┐ | ||
| │ Event Processing Pipeline │ | ||
| ├──────────────────────────────────────────────────────────────────────┤ | ||
| │ │ | ||
| │ CloudEvent ──► Phase 1 ──► Phase 2 ──► Phase 3 ──► Phase 4 ──► Done │ | ||
| │ Extract Precond. Resources Post-Act. │ | ||
| │ Params Eval. Create Execute │ | ||
| │ │ | ||
| └──────────────────────────────────────────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The ASCII diagram should specify a language (e.g., text) to satisfy markdown linting and improve rendering consistency.
Apply this diff:
-```
+```text
┌──────────────────────────────────────────────────────────────────────┐
│ Event Processing Pipeline │🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
internal/executor/README.md around lines 25 to 35: the fenced ASCII diagram
lacks a language specifier which markdown linters require; update the opening
fence to include a language (e.g., change "```" to "```text") so the block
starts with "```text" and retain the diagram content exactly as-is.
|
/test presubmits-images |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-amarin, xueli181114 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 |
|
/unassign @tirthct |
openshift-merge-bot
bot
merged commit 987aa58
into
openshift-hyperfleet:main
3 participants
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.