HYPERFLEET-293 | feat: Update the code to use latest broker lib #17
Conversation
WalkthroughThis PR bumps the adapter version to 0.1.0, adds a Cobra-based CLI (root, serve, version), and reworks startup to load config → create HyperFleet API client → create Kubernetes client → build executor → subscribe to broker (env-driven) with graceful shutdown. It removes the internal broker_consumer implementation and related integration tests. Helm/chart tweaks include PDB pre-validation and deployment template simplifications. Multiple internal packages (executor, criteria, config_loader, k8s_client, utils, logger) receive logging, CEL/evaluation/context, template helper, and minor API/signature changes. go.mod updates add Cobra/pflag and adjust several dependencies. Sequence Diagram(s)sequenceDiagram
participant CLI as cobra rootCmd
participant Config as Config Loader
participant API as HyperFleet API Client
participant K8s as Kubernetes Client
participant Builder as Executor Builder
participant Broker as Broker Subscriber
participant Handler as Executor Handler
participant OS as OS Signals
CLI->>Config: load config (file/env)
Config-->>CLI: config
CLI->>API: create API client
API-->>CLI: client ready
CLI->>K8s: create Kubernetes client (KUBECONFIG fallback)
K8s-->>CLI: client ready
CLI->>Builder: build executor(API, K8s, config)
Builder-->>CLI: executor
CLI->>Broker: create subscriber (env: BROKER_SUBSCRIPTION_ID / BROKER_TOPIC)
Broker-->>CLI: subscriber ready
CLI->>Handler: handler := executor.Handler()
CLI->>Broker: subscribe(handler) (non-blocking, errors -> fatal channel)
Broker->>Handler: deliver CloudEvent
Handler->>Handler: execute phases (params → preconds → resources → post actions)
Handler-->>Broker: ACK (always ACK on failure to avoid retries)
OS->>CLI: SIGTERM/SIGINT
CLI->>Broker: close subscriber (30s timeout)
Broker-->>CLI: closed
CLI->>CLI: shutdown complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
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: 2
🧹 Nitpick comments (11)
test/integration/executor/main_test.go (1)
101-103: Helper rename is fine; consider simplifying the function name (nit)The helper rename is consistent and behavior is unchanged. If you touch this again, consider a slightly shorter/clearer name like
setupExecutorEnvtestorsetupSharedK8sEnvtestto avoid the repeatedEnvtestEnvsuffix, but this is purely cosmetic.internal/k8s_client/mock.go (1)
57-58: Pluralizing Kind improves fidelity, but resource name still differs from real k8s errorsUsing
gvk.Kind + "s"is a reasonable step toward matching real NotFound errors (plural resource instead of Kind) and should work fine with the updated tests. Just note that real Kubernetes resource names are typically lowercased (e.g.pods) and some Kinds have irregular plurals (e.g.Ingress→ingresses), so any future code that inspectsgr.Resourceorerr.Error()may still not exactly match the live API server.If you ever need closer fidelity, consider either lowercasing and/or allowing the resource name to be provided explicitly (e.g. via a
schema.GroupResourceparameter) rather than deriving it from Kind.internal/executor/utils.go (1)
249-329: Consider documenting available template functions.The template function library provides useful helpers for time formatting, string manipulation, and type conversions. However, the type conversion functions silently ignore errors and return zero values, which may mask issues.
Consider:
- Adding documentation (inline or separate) listing available template functions for users
- For production robustness, you might want conversion functions to return errors or provide separate "safe" variants
Since these are internal utilities and have reasonable fallback behavior, this is not critical.
test/integration/config-loader/config_criteria_integration_test.go (1)
18-23: TestMain environment setup is functional but lacks cleanup.Using
os.SetenvinTestMainis appropriate since it runs once before all tests. However, this doesn't restore the original environment variable value, which could affect other test packages if run in the same process.Consider adding cleanup for test isolation:
func TestMain(m *testing.M) { // Set required environment variables for tests + originalValue := os.Getenv("HYPERFLEET_API_BASE_URL") os.Setenv("HYPERFLEET_API_BASE_URL", "http://test-api.example.com") - os.Exit(m.Run()) + code := m.Run() + // Restore original value + if originalValue == "" { + os.Unsetenv("HYPERFLEET_API_BASE_URL") + } else { + os.Setenv("HYPERFLEET_API_BASE_URL", originalValue) + } + os.Exit(code) }internal/executor/precondition_executor.go (1)
91-98: Inconsistent indentation in error handling blocks.The
ExecutionErrorassignment blocks have inconsistent indentation compared to the surrounding code, which affects readability.// 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) }Apply similar fix to lines 109-115.
Also applies to: 109-116
cmd/adapter/main.go (3)
221-232: Error monitoring goroutine may leak after shutdown.The goroutine monitoring
subscriber.Errors()will block on the channel read after the subscriber is closed if the errors channel isn't closed by the subscriber. Additionally, when context is cancelled, this goroutine continues running until the errors channel is closed.Consider adding context awareness to the error monitoring goroutine:
go func() { - for subErr := range subscriber.Errors() { - log.Error(fmt.Sprintf("Subscription error: %v", subErr)) - // For critical errors, signal shutdown - select { - case fatalErrCh <- subErr: - // Signal sent, trigger shutdown - default: - // Channel already has an error, don't block + for { + select { + case <-ctx.Done(): + return + case subErr, ok := <-subscriber.Errors(): + if !ok { + return // Channel closed + } + log.Error(fmt.Sprintf("Subscription error: %v", subErr)) + // For critical errors, signal shutdown + select { + case fatalErrCh <- subErr: + // Signal sent, trigger shutdown + default: + // Channel already has an error, don't block + } } } }()
245-251: Consider adding timeout to subscriber.Close().If the broker subscriber's
Close()method hangs (e.g., network issues), the shutdown process will block indefinitely. Consider adding a timeout.// Close subscriber gracefully log.Info("Closing broker subscriber...") - if err := subscriber.Close(); err != nil { - log.Error(fmt.Sprintf("Error closing subscriber: %v", err)) - } else { - log.Info("Subscriber closed successfully") - } + closeCtx, closeCancel := context.WithTimeout(context.Background(), 10*time.Second) + defer closeCancel() + + closeDone := make(chan error, 1) + go func() { + closeDone <- subscriber.Close() + }() + + select { + case err := <-closeDone: + if err != nil { + log.Error(fmt.Sprintf("Error closing subscriber: %v", err)) + } else { + log.Info("Subscriber closed successfully") + } + case <-closeCtx.Done(): + log.Warning("Subscriber close timed out after 10s") + }
254-256: Hardcoded grace period.The 1-second grace period is hardcoded. Consider making this configurable or at least documenting why 1 second is sufficient.
+ const shutdownGracePeriod = time.Second + // Give a small grace period for in-flight messages to complete log.Info("Initiating graceful shutdown...") - time.Sleep(time.Second) + time.Sleep(shutdownGracePeriod) log.Info("Adapter shutdown complete")internal/criteria/evaluator.go (2)
286-306: Consider distinguishing "invalid definition" from "evaluation failure".Currently, both invalid input (missing/empty expression) and evaluation failure return
(nil, bool)with different boolean values:
- Missing expression →
(nil, false)- Evaluation failure →
(nil, true)or(default, true)This is correct per the doc, but the semantic of the boolean (
true= "was handled") may be subtle for callers. Consider adding a brief inline comment clarifying this distinction for maintainability.func (e *Evaluator) EvaluateExpressionDef(def map[string]any) (any, bool) { expr, ok := def[ExpressionKey].(string) if !ok || expr == "" { + // Invalid definition - expression key missing or empty return nil, false }
308-316: Inconsistent API style: standalone function vs. method.
GetValueDefis a standalone function whileEvaluateExpressionDefis a method onEvaluator. SinceGetValueDefdoesn't need the evaluator's context, this is intentional. Consider a brief comment explaining why it's a package-level function for clarity.+// GetValueDef extracts a value from a value definition map. +// This is a standalone function (not a method) because it doesn't require +// the Evaluator's context - it simply extracts the "value" key from the map. +// The map should contain: // - "value": the value to return (required)configs/adapter-config-template.yaml (1)
176-189: Consider extracting repeated CEL expressions.The expression
resources.?clusterNamespace.?status.?phase.orValue("")is repeated across multiple conditions. While YAML doesn't support DRY patterns natively, this could be addressed with:
- A capture variable in the post payload context
- Documentation noting the intentional repetition
This is minor given it's a template file.
📜 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 (41)
Dockerfile(1 hunks)Makefile(2 hunks)charts/templates/deployment.yaml(0 hunks)charts/templates/pdb.yaml(1 hunks)charts/values.yaml(1 hunks)cmd/adapter/main.go(3 hunks)configs/adapter-config-template.yaml(7 hunks)configs/adapter.yaml(2 hunks)go.mod(4 hunks)internal/broker_consumer/README.md(0 hunks)internal/broker_consumer/consumer.go(0 hunks)internal/config_loader/README.md(1 hunks)internal/config_loader/loader_test.go(22 hunks)internal/config_loader/validator.go(1 hunks)internal/config_loader/validator_schema.go(2 hunks)internal/config_loader/validator_test.go(31 hunks)internal/criteria/evaluator.go(2 hunks)internal/executor/executor.go(6 hunks)internal/executor/executor_test.go(1 hunks)internal/executor/param_extractor.go(1 hunks)internal/executor/post_action_executor.go(5 hunks)internal/executor/post_action_executor_test.go(1 hunks)internal/executor/precondition_executor.go(7 hunks)internal/executor/resource_executor.go(5 hunks)internal/executor/types.go(1 hunks)internal/executor/utils.go(4 hunks)internal/k8s_client/client.go(4 hunks)internal/k8s_client/mock.go(1 hunks)pkg/logger/logger.go(2 hunks)test/integration/broker_consumer/README.md(0 hunks)test/integration/broker_consumer/adapter_integration_test.go(0 hunks)test/integration/broker_consumer/setup_test.go(0 hunks)test/integration/broker_consumer/testutil_container.go(0 hunks)test/integration/broker_consumer/testutil_publisher.go(0 hunks)test/integration/config-loader/config_criteria_integration_test.go(7 hunks)test/integration/config-loader/loader_template_test.go(5 hunks)test/integration/config-loader/testdata/adapter-config-template.yaml(1 hunks)test/integration/config-loader/testdata/adapter_config_valid.yaml(1 hunks)test/integration/executor/executor_integration_test.go(1 hunks)test/integration/executor/main_test.go(2 hunks)test/testdata/adapter_config_valid.yaml(1 hunks)
💤 Files with no reviewable changes (8)
- test/integration/broker_consumer/adapter_integration_test.go
- test/integration/broker_consumer/testutil_publisher.go
- test/integration/broker_consumer/README.md
- test/integration/broker_consumer/testutil_container.go
- test/integration/broker_consumer/setup_test.go
- internal/broker_consumer/README.md
- internal/broker_consumer/consumer.go
- charts/templates/deployment.yaml
🧰 Additional context used
🧬 Code graph analysis (11)
internal/config_loader/validator.go (1)
internal/config_loader/constants.go (1)
FieldAdapter(15-15)
test/integration/config-loader/loader_template_test.go (1)
internal/criteria/types.go (1)
Operator(10-10)
test/integration/executor/main_test.go (1)
test/integration/executor/setup_test.go (1)
K8sTestEnv(16-22)
internal/executor/post_action_executor_test.go (1)
pkg/logger/logger.go (1)
Flush(158-160)
internal/k8s_client/client.go (2)
pkg/logger/logger.go (1)
Logger(11-20)pkg/errors/error.go (1)
KubernetesError(203-205)
internal/executor/precondition_executor.go (5)
internal/executor/types.go (2)
PreconditionsOutcome(380-390)PhasePreconditions(24-24)internal/config_loader/types.go (1)
APICall(108-116)internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
NewEvaluator(52-60)internal/config_loader/constants.go (1)
FieldValue(72-72)
internal/config_loader/loader_test.go (2)
internal/config_loader/types.go (4)
AdapterConfig(6-11)AdapterConfigSpec(21-29)Resource(171-178)DiscoveryConfig(181-185)internal/k8s_client/discovery.go (2)
Discovery(14-29)DiscoveryConfig(32-42)
test/integration/executor/executor_integration_test.go (1)
pkg/logger/logger.go (1)
Flush(158-160)
test/integration/config-loader/config_criteria_integration_test.go (2)
internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
internal/executor/post_action_executor.go (4)
internal/executor/types.go (4)
ExecutionError(238-245)PhasePostActions(28-28)PostActionResult(153-170)NewExecutorError(370-377)internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
GetValueDef(313-316)internal/config_loader/types.go (1)
APICall(108-116)
internal/criteria/evaluator.go (1)
internal/criteria/cel_evaluator.go (1)
CELEvaluator(17-21)
🔇 Additional comments (60)
test/integration/executor/main_test.go (1)
75-82: Updated TestMain setup call looks goodThe call to
setupSharedK8sEnvtestEnv()is consistent with the helper rename and keeps all setup/teardown logic centralized inTestMain. No functional issues here.Dockerfile (1)
51-51: LGTM!The version label update to 0.1.0 aligns with the broader version bump across the PR.
charts/values.yaml (1)
76-76: LGTM!The indentation adjustment improves YAML formatting consistency without affecting behavior.
charts/templates/pdb.yaml (1)
2-7: LGTM!The validation logic correctly enforces Kubernetes PodDisruptionBudget requirements by ensuring that
minAvailableandmaxUnavailableare mutually exclusive and that at least one is specified when the PDB is enabled.Makefile (2)
5-5: LGTM!Version update to 0.1.0 is consistent with the PR-wide version bump.
204-266: LGTM!The new
test-helm-chartstarget provides comprehensive validation coverage for Helm charts, testing multiple configuration scenarios including the newly added PDB validation. This complements the chart validation improvements in the PR.internal/executor/types.go (1)
325-348: LGTM!The
GetCELVariables()method provides a clean abstraction for building CEL evaluation context. The implementation correctly:
- Creates a fresh map to avoid shared state
- Copies all parameters
- Includes adapter metadata
- Safely converts resources with nil checks
This centralizes context building logic that was previously duplicated.
pkg/logger/logger.go (1)
19-19: LGTM!The
Flush()method addition to the Logger interface provides explicit control over log flushing. The implementation correctly delegates toglog.Flush(), and test mocks have been updated accordingly (per AI summary).Note: This is a breaking change for any external Logger interface implementations.
Also applies to: 147-150
internal/executor/param_extractor.go (2)
21-21: LGTM!The enhanced error message now includes the parameter name and source, making debugging parameter extraction failures much easier.
30-32: Verify impact of treating empty string as missing value.The default handling now treats empty strings (
"") the same asnil, applying the default value for both cases. While this is often intuitive behavior, verify that:
- No existing configurations rely on distinguishing between
""andnil- Empty string sources (e.g., environment variables set to
"") should trigger defaultsThis change makes default behavior more user-friendly but could be a subtle breaking change.
internal/executor/utils.go (1)
138-141: Verify that POST bodies don't contain sensitive data before logging.Logging the full request body on failure is helpful for debugging, but could expose sensitive information (credentials, PII, authentication tokens, etc.) in logs. Before merging:
- Audit POST bodies in this adapter to confirm they don't contain secrets or sensitive user data
- Review log retention policies to ensure error logs with body content are handled appropriately
- Consider implementing field-level redaction for sensitive data if needed
go.mod (1)
12-16: LGTM on new direct dependencies.The addition of
cobraandpflagfor CLI support, along with the reclassification ofpubsub,uuid, andgrpcto indirect dependencies, aligns with the PR objective of using the latest broker library.internal/k8s_client/client.go (2)
20-22: LGTM!Good use of a constant for the standard
KUBECONFIGenvironment variable name, enabling consistent usage across the codebase.
64-84: LGTM!The priority-based kubeconfig resolution is well-implemented with clear logging for each path. The fallback chain (explicit path →
KUBECONFIGenv → in-cluster) follows standard Kubernetes tooling conventions.test/integration/config-loader/testdata/adapter_config_valid.yaml (1)
14-14: LGTM!Version bump to
0.1.0aligns with the broader release changes across the codebase.test/integration/config-loader/testdata/adapter-config-template.yaml (1)
47-47: LGTM!Version bump to
0.1.0maintains consistency with other configuration files in the PR.internal/executor/post_action_executor_test.go (1)
29-29: LGTM!Adding the
Flush()method to the test logger correctly satisfies the updatedlogger.Loggerinterface. A no-op implementation is appropriate for test mocks.internal/config_loader/README.md (1)
51-51: LGTM! Documentation updated to reflect version bump.The version reference has been correctly updated to match the adapter version upgrade to 0.1.0 throughout the codebase.
internal/executor/executor_test.go (1)
38-38: LGTM! Mock logger updated to satisfy interface.The no-op
Flush()method correctly implements the extended logger interface for testing purposes without affecting test behavior.test/testdata/adapter_config_valid.yaml (2)
14-14: Version bump applied correctly.The adapter version has been updated to 0.1.0, consistent with the PR objectives.
182-189: Verify authentication mechanism for API calls.The
hyperfleetApiTokenparameter andAuthorizationheader have been removed from the test configuration. Please confirm:
- Is authentication now handled at a different layer (e.g., service mesh, API gateway)?
- Are there alternative authentication mechanisms in place?
- Should integration tests be updated to reflect the new authentication pattern?
internal/config_loader/validator_test.go (1)
29-29: LGTM! Test data consistently updated with baseUrl field.All test YAML snippets have been updated to include the
baseUrlfield underhyperfleetApi, ensuring comprehensive test coverage for the updated API configuration schema.Also applies to: 61-61, 89-89, 116-116, 143-143, 170-170, 198-198, 226-226, 252-252, 304-304, 332-332, 359-359, 391-391, 455-455, 480-480, 506-506, 556-556, 586-586, 614-614, 642-642, 669-669, 699-699, 724-724, 870-870, 915-915, 1011-1011, 1035-1035, 1060-1060, 1082-1082, 1105-1105, 1127-1127
internal/config_loader/validator.go (1)
352-356: LGTM! CEL environment enhanced for adapter metadata access.The addition of the
adapterCEL variable enables expressions to safely access adapter execution metadata (e.g.,adapter.executionStatus,adapter.errorReason). This follows the established pattern used for theresourcesvariable and supports richer status evaluation in post-action payloads.configs/adapter.yaml (2)
20-20: Version bump applied correctly.The adapter version has been updated to 0.1.0 throughout the configuration.
38-43: Good change: API version now optional with sensible default.Making
hyperfleetApiVersionoptional (with default "v1") improves usability while maintaining backward compatibility. Deployments can omit this parameter and rely on the default, reducing configuration boilerplate.test/integration/executor/executor_integration_test.go (1)
1006-1006: LGTM! Integration test logger updated for interface compatibility.The no-op
Flush()method correctly implements the extended logger interface without affecting integration test behavior.internal/config_loader/validator_schema.go (1)
199-199: Good enhancement: Flexible namespace discovery.Allowing empty or
"*"for the namespace in resource discovery improves flexibility for cluster-scoped resources and multi-namespace scenarios. This is a sensible relaxation of the previous requirement.test/integration/config-loader/loader_template_test.go (2)
41-42: LGTM! Proper test environment setup usingt.Setenv.Using
t.Setenvis the correct approach for test isolation - it automatically restores the original environment variable value after the test completes.Also applies to: 127-128
58-58: Test expectations updated to match new config structure.The updated assertions align with the adapter version bump (0.1.0) and configuration schema changes. Ensure the corresponding template file (
configs/adapter-config-template.yaml) has been updated with the expected values including the adapter version and any other configuration changes referenced in the affected test assertions.internal/config_loader/loader_test.go (2)
28-30: Test fixtures updated consistently for adapter version 0.1.0 and baseUrl field.The test YAML fixtures are consistently updated to include the new
baseUrlfield underhyperfleetApiand the version bump to0.1.0. This aligns with the schema changes in the PR.Also applies to: 71-71
1152-1167: Relaxed namespace requirement for manifest.ref discovery.The test case now validates that an empty namespace (meaning "all namespaces") is valid for manifest.ref scenarios. This is a reasonable change that allows cluster-scoped or multi-namespace resource discovery.
test/integration/config-loader/config_criteria_integration_test.go (1)
47-48: Test data updated to reflect template condition changes.The test data now uses
"NotReady"instead of"Ready"to match the updated template conditions. The test case at lines 204-221 explicitly tests that"Ready"phase should fail the precondition (since the template expects"NotReady"), which is good negative testing.Also applies to: 67-67, 154-154, 185-185, 204-221
internal/executor/post_action_executor.go (4)
35-47: Enhanced logging for post payload building phase.The added logging provides good visibility into payload building progress and failures. Using
log.V(1).Infoffor per-payload success messages is appropriate for verbose output.
79-81: CEL context now uses centralized GetCELVariables() method.This change properly consolidates CEL variable setup through
execCtx.GetCELVariables(), ensuring consistent variable availability across different executors.
52-53: Consistent indexed logging for post-action execution.The indexed logging format
[PostAction %d/%d]with clear SUCCESS/FAILED markers provides excellent observability for debugging execution flows.Also applies to: 58-58, 70-70, 210-210, 214-214
157-158: Verify error handling in EvaluateExpressionDef.The
EvaluateExpressionDefreturns(result, ok)whereokindicates if the map contained an expression definition. However, if CEL evaluation itself fails, how is that error surfaced? Examine the implementation to determine whether evaluation errors are returned, suppressed, or handled internally.internal/executor/precondition_executor.go (3)
32-33: Comprehensive indexed logging for precondition execution.The logging format
[Precondition %d/%d]with clear MET/NOT MET/EXECUTION ERROR states provides excellent visibility into precondition evaluation flow.Also applies to: 39-39, 49-49, 57-58
136-139: CEL context properly includes all variables via GetCELVariables().The comment correctly notes that resources will be empty during preconditions since they haven't been created yet. This is a good clarifying comment.
158-165: Detailed per-condition result logging.Logging each condition result with field, operator, expected, and actual values is very helpful for debugging. Using
log.V(1)for successful conditions and regularlog.Infoffor failures is a good pattern - failures are always visible while successes require verbose mode.internal/executor/resource_executor.go (3)
40-41: Consistent indexed logging for resource processing.The logging pattern
[Resource %d/%d]matches the other executors, providing consistent observability across all execution phases.Also applies to: 46-46, 49-50
97-101: Clear discovery result logging.Explicitly logging whether an existing resource was found or will be created improves debuggability and makes the execution flow easier to follow.
201-202: Helpful clarifying comment for namespace semantics.The comment explains that empty namespace (normalized from
"*"at config load time) means "all namespaces", which aids maintainability.cmd/adapter/main.go (3)
40-98: Well-structured Cobra CLI implementation.The CLI structure with
serveandversionsubcommands follows standard patterns. The help text is clear and informative.
180-192: Required environment variables validated with clear error messages.Good practice to validate required environment variables early with descriptive error messages that include the variable name.
261-291: API client creation with proper option handling.The
createAPIClientfunction correctly validates and applies all configuration options including timeout, retry attempts, and backoff strategy.internal/executor/executor.go (5)
75-80: LGTM - Structured event receipt logging.Clear phase delineation with visual separators aids debugging and observability.
99-120: LGTM - Clear precondition phase logging.The logging distinguishes between evaluation failure (error), business outcome (not met), and success with counts.
140-143: LGTM - Resource result logging provides useful diagnostics.Per-resource details including operation type aid troubleshooting.
154-158: LGTM - Null-safe post action count calculation.Correctly handles the case where
Spec.Postis nil.
230-246: Verify ACK-on-failure strategy aligns with operational requirements.The change to always ACK (return nil) even on failure prevents infinite retry loops for non-recoverable errors. This is a sound pattern for 4xx errors and validation failures. However, ensure this behavior is intentional for all failure modes:
- Transient failures (network timeouts, 5xx) typically benefit from retries
- The current implementation treats all failures uniformly
If differentiated handling is needed later, consider distinguishing recoverable vs. non-recoverable errors.
internal/criteria/evaluator.go (3)
255-269: LGTM - Clarified documentation.The comments now explicitly state the return behavior on failure, helping callers understand they can ignore the error for safe defaults.
271-276: LGTM - Centralizing definition keys as constants.Good practice to avoid magic strings scattered across the codebase.
387-391: Good fix - error propagation in negate wrapper.The previous implementation would have silently negated the result even when the underlying function returned an error, leading to incorrect behavior. This fix correctly propagates errors.
configs/adapter-config-template.yaml (7)
28-33: LGTM - Helpful documentation for CEL optional chaining.Good to document the pattern upfront since it's used extensively in the payload definitions.
53-53: Version bump to 0.1.0.Appropriate for the scope of breaking changes in this PR.
92-95: Breaking change: clusterId source changed fromevent.cluster_idtoevent.id.Ensure all CloudEvent producers emit the cluster identifier in the
idfield. This is a contract change that affects integration.
109-110: API path structure change - verify backend compatibility.The API path now includes
/api/hyperfleet/prefix. Ensure the HyperFleet API backend serves endpoints at this path structure.
123-126: Simplified precondition: single equals check.Changed from multi-condition validation to a single
equals "NotReady"check. This is cleaner but ensure the business logic correctly requires only this condition.
140-141: Good practice: lowercase transformation for K8s resource names.Using
{{ .clusterId | lower }}ensures compliance with RFC 1123 DNS label requirements for Kubernetes resource names.
242-243: API endpoint uses correct naming convention — The plural form/statusesaligns with REST API best practices for collection resources. Verify that the backend API supports POST requests to/clusters/{id}/statusesand confirm whether the old/statusendpoint remains available for backward compatibility.
xueli181114
force-pushed
the
HYPERFLEET-293
branch
from
2705f9e to
e973d08
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
internal/config_loader/validator_schema.go (1)
126-133: Consider validation timing for environment variables.The new validation checks required environment variables at config load time. As noted in past review comments, this could cause issues when:
- Configs are validated in CI/CD before env vars are available
- Developers validate configs locally without production env vars
- Validation happens during container image builds
Based on past review comments, this remains a concern.
#!/bin/bash # Check where config validation occurs and in what context echo "=== Find config Load/Parse calls ===" rg -n "config_loader\.Load|config_loader\.Parse" --type go -C 3 echo "=== Check for validation-only mode flags ===" rg -n "validation.*(only|mode)" --type go echo "=== Check main entry points and initialization ===" rg -n "func main\(\)|func.*Init" --type go -A 10 | rg -A 10 "config_loader"internal/executor/executor.go (1)
92-94: Potential sensitive data exposure in logs (previously flagged).This concern was raised in a previous review. Logging parameter values at V(1) could expose secrets, tokens, or PII if such values flow through the params map.
🧹 Nitpick comments (8)
charts/templates/pdb.yaml (1)
15-20: Consider simplifying redundant conditions.Since lines 2-4 now fail when both values are set, the
(not .Values.podDisruptionBudget.maxUnavailable)and(not .Values.podDisruptionBudget.minAvailable)guards are redundant. You could simplify to:- {{- if and .Values.podDisruptionBudget.minAvailable (not .Values.podDisruptionBudget.maxUnavailable) }} + {{- if .Values.podDisruptionBudget.minAvailable }} minAvailable: {{ .Values.podDisruptionBudget.minAvailable }} {{- end }} - {{- if and .Values.podDisruptionBudget.maxUnavailable (not .Values.podDisruptionBudget.minAvailable) }} + {{- if .Values.podDisruptionBudget.maxUnavailable }} maxUnavailable: {{ .Values.podDisruptionBudget.maxUnavailable }} {{- end }}However, keeping the extra guards is harmless and acts as defense-in-depth.
Makefile (1)
204-265: Well-structured Helm chart test target.The test cases provide good coverage of various configurations. Consider adding a negative test case for the new PDB mutual exclusion validation:
@echo "📋 Testing PDB mutual exclusion validation..." @if helm template test-release charts/ \ --set podDisruptionBudget.enabled=true \ --set podDisruptionBudget.minAvailable=1 \ --set podDisruptionBudget.maxUnavailable=1 2>&1 | grep -q "cannot set both"; then \ echo "✅ PDB mutual exclusion validation OK"; \ else \ echo "❌ PDB mutual exclusion validation failed"; \ exit 1; \ fiThis would verify the new fail condition in
pdb.yamlis working correctly.internal/executor/utils.go (3)
252-257:dateanddateFormatare identical.Both functions have the same signature and implementation. Consider removing one to reduce API surface confusion, or document why both exist (e.g., compatibility with different templating conventions).
281-328: Type conversion functions silently swallow parse errors.When
strconv.Atoi,strconv.ParseInt, orstrconv.ParseFloatfail, these functions return0without any indication of failure. This can mask template bugs where invalid data is passed.Consider either:
- Logging a warning on parse failure
- Returning an error (if the template engine supports it)
- Documenting this behavior clearly
Example with warning:
"int": func(v interface{}) int { switch val := v.(type) { // ... other cases ... case string: - i, _ := strconv.Atoi(val) + i, err := strconv.Atoi(val) + if err != nil { + // Silent fallback to 0 - consider logging in debug mode + } return i default: return 0 } },
261-263:titlefunction hardcodes English locale.Using
language.Englishmay produce unexpected results for non-English text. Consider making this configurable or documenting the limitation.internal/executor/precondition_executor.go (2)
91-98: Inconsistent indentation in error handling block.The code block has mixed indentation - the
execCtx.Adapter.ExecutionErrorblock and thereturnstatement appear to use different indentation levels, which may cause readability issues or indicate copy-paste errors.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) + Phase: string(PhasePreconditions), + Step: precond.Name, + Message: err.Error(), + } + + return result, NewExecutorError(PhasePreconditions, precond.Name, "API call failed", err) }
109-115: Same indentation inconsistency in JSON parse error handling.This block has the same indentation issue as the previous error handling block.
// 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) }cmd/adapter/main.go (1)
217-243: Consider logging non-fatal subscription errors without triggering shutdown.The current implementation sends any subscription error to
fatalErrCh, triggering shutdown. This means even transient or recoverable errors will terminate the adapter.Consider categorizing errors: log non-fatal errors and only signal shutdown for truly fatal errors (e.g., connection lost, authentication failure).
// Monitor subscription errors channel in a separate goroutine go func() { for subErr := range subscriber.Errors() { log.Error(fmt.Sprintf("Subscription error: %v", subErr)) - // For critical errors, signal shutdown - select { - case fatalErrCh <- subErr: - // Signal sent, trigger shutdown - default: - // Channel already has an error, don't block + // TODO: Categorize errors - only signal shutdown for fatal errors + // For now, all errors trigger shutdown to surface issues early + if isFatalSubscriptionError(subErr) { + select { + case fatalErrCh <- subErr: + // Signal sent, trigger shutdown + default: + // Channel already has an error, don't block + } } } }()
📜 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 (41)
Dockerfile(1 hunks)Makefile(2 hunks)charts/templates/deployment.yaml(0 hunks)charts/templates/pdb.yaml(1 hunks)charts/values.yaml(1 hunks)cmd/adapter/main.go(3 hunks)configs/adapter-config-template.yaml(7 hunks)configs/adapter.yaml(2 hunks)go.mod(4 hunks)internal/broker_consumer/README.md(0 hunks)internal/broker_consumer/consumer.go(0 hunks)internal/config_loader/README.md(1 hunks)internal/config_loader/loader_test.go(22 hunks)internal/config_loader/validator.go(1 hunks)internal/config_loader/validator_schema.go(2 hunks)internal/config_loader/validator_test.go(31 hunks)internal/criteria/evaluator.go(2 hunks)internal/executor/executor.go(6 hunks)internal/executor/executor_test.go(1 hunks)internal/executor/param_extractor.go(1 hunks)internal/executor/post_action_executor.go(5 hunks)internal/executor/post_action_executor_test.go(1 hunks)internal/executor/precondition_executor.go(7 hunks)internal/executor/resource_executor.go(5 hunks)internal/executor/types.go(1 hunks)internal/executor/utils.go(4 hunks)internal/k8s_client/client.go(4 hunks)internal/k8s_client/mock.go(1 hunks)pkg/logger/logger.go(2 hunks)test/integration/broker_consumer/README.md(0 hunks)test/integration/broker_consumer/adapter_integration_test.go(0 hunks)test/integration/broker_consumer/setup_test.go(0 hunks)test/integration/broker_consumer/testutil_container.go(0 hunks)test/integration/broker_consumer/testutil_publisher.go(0 hunks)test/integration/config-loader/config_criteria_integration_test.go(7 hunks)test/integration/config-loader/loader_template_test.go(5 hunks)test/integration/config-loader/testdata/adapter-config-template.yaml(1 hunks)test/integration/config-loader/testdata/adapter_config_valid.yaml(1 hunks)test/integration/executor/executor_integration_test.go(1 hunks)test/integration/executor/main_test.go(2 hunks)test/testdata/adapter_config_valid.yaml(1 hunks)
💤 Files with no reviewable changes (8)
- internal/broker_consumer/README.md
- test/integration/broker_consumer/adapter_integration_test.go
- charts/templates/deployment.yaml
- test/integration/broker_consumer/README.md
- test/integration/broker_consumer/testutil_container.go
- test/integration/broker_consumer/testutil_publisher.go
- test/integration/broker_consumer/setup_test.go
- internal/broker_consumer/consumer.go
🚧 Files skipped from review as they are similar to previous changes (13)
- configs/adapter.yaml
- internal/k8s_client/mock.go
- Dockerfile
- internal/config_loader/README.md
- charts/values.yaml
- internal/executor/post_action_executor_test.go
- test/integration/config-loader/testdata/adapter-config-template.yaml
- internal/config_loader/validator.go
- test/integration/executor/executor_integration_test.go
- internal/config_loader/loader_test.go
- test/integration/executor/main_test.go
- internal/executor/executor_test.go
- internal/executor/types.go
🧰 Additional context used
🧬 Code graph analysis (7)
test/integration/config-loader/loader_template_test.go (1)
internal/criteria/types.go (1)
Operator(10-10)
internal/executor/post_action_executor.go (3)
internal/executor/types.go (1)
PostActionResult(153-170)internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
GetValueDef(313-316)
internal/k8s_client/client.go (1)
pkg/errors/error.go (1)
KubernetesError(203-205)
internal/criteria/evaluator.go (1)
internal/criteria/cel_evaluator.go (1)
CELEvaluator(17-21)
internal/executor/resource_executor.go (4)
internal/k8s_client/discovery.go (1)
Discovery(14-29)pkg/errors/k8s_error.go (1)
IsRetryableDiscoveryError(160-192)internal/executor/types.go (5)
StatusFailed(44-44)PhaseResources(26-26)OperationRecreate(147-147)OperationUpdate(145-145)OperationCreate(143-143)internal/config_loader/types.go (1)
Resource(171-178)
internal/executor/precondition_executor.go (4)
internal/executor/types.go (2)
PreconditionsOutcome(380-390)PhasePreconditions(24-24)internal/config_loader/types.go (1)
APICall(108-116)internal/criteria/types.go (2)
NewEvaluationContext(75-80)Operator(10-10)internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
cmd/adapter/main.go (5)
pkg/logger/logger.go (1)
Flush(158-160)internal/k8s_client/client.go (2)
NewClient(60-109)ClientConfig(30-38)internal/hyperfleet_api/client.go (2)
NewClient(117-145)WithTimeout(66-70)internal/hyperfleet_api/types.go (1)
ClientConfig(38-54)internal/executor/executor.go (1)
NewBuilder(277-281)
🔇 Additional comments (58)
go.mod (2)
30-30: Dependency reclassification correctly reflects broker_consumer removal.Moving
cloud.google.com/go/pubsub/v2,github.com/google/uuid, andgoogle.golang.org/grpcto indirect aligns with the removal of the internal broker_consumer. These are now only transitively required through thehyperfleet-brokerdependency, which is the intended design after the refactoring.Also applies to: 66-66, 137-137
12-13: Cobra and PFlag additions appropriately support CLI refactoring.The direct dependencies on Cobra v1.10.2 and PFlag v1.0.10 align with the PR's refactoring to a Cobra CLI entrypoint. Both versions are stable and compatible with Go 1.25.0.
charts/templates/pdb.yaml (1)
2-4: LGTM! Good defensive validation.Adding fail-fast validation when both
minAvailableandmaxUnavailableare set aligns with Kubernetes PDB semantics, which allows only one of these fields.test/testdata/adapter_config_valid.yaml (2)
14-14: Version bump acknowledged.The version update to
0.1.0aligns with the broader release changes across the repository.
182-189: Verify removal of Authorization header is intentional.The
reportClusterStatusAPI call no longer includes anAuthorizationheader with Bearer token. Ensure the HyperFleet API endpoint at/clusters/{clusterId}/statusno longer requires authentication, or that authentication is handled differently (e.g., mTLS, service mesh).Makefile (1)
5-5: Version bump acknowledged.Consistent with the broader
0.1.0release across the repository.pkg/logger/logger.go (2)
19-19: Good addition to the Logger interface.Adding
Flush()enables callers to ensure pending log entries are written before process termination or critical operations.
147-160: LGTM on Flush implementations.Both the method and package-level function correctly delegate to
glog.Flush(). Having both allows flexibility—use the method when you have aLoggerinstance, or the package function for standalone calls.internal/executor/utils.go (1)
338-338: Good integration of templateFuncs.Registering
templateFuncswith the template before parsing ensures all helper functions are available during template execution.test/integration/config-loader/loader_template_test.go (3)
41-42: LGTM: Proper test environment setup.Using
t.Setenvensures the environment variable is properly scoped to the test and cleaned up automatically after execution.Also applies to: 127-128
58-58: LGTM: Version bump aligned with PR objectives.The adapter version update from "0.0.1" to "0.1.0" is consistent with the PR objectives.
67-96: All test expectations at lines 67, 72, 90, and 96 match the adapter config template exactly:
- Template contains exactly 3 parameters (line 76-96):
hyperfleetApiBaseUrl,hyperfleetApiVersion, andclusterIdclusterIdsource is"event.id"(line 92)clusterNamecapture field is"name"(line 117)- First precondition operator is
"equals"(line 124)The test file correctly validates the shipped template configuration.
test/integration/config-loader/config_criteria_integration_test.go (2)
18-23: LGTM: Proper integration test setup.Using
TestMainfor package-level environment variable setup is the appropriate approach for integration tests that all requireHYPERFLEET_API_BASE_URL.
47-48: LGTM: Test data aligned with template condition changes.The tests have been correctly updated to reflect the template's new precondition that checks for
"NotReady"phase instead of"Ready". The test coverage is comprehensive:
- Positive case: clusterPhase = "NotReady" should pass (lines 47-48, 67, 154)
- Negative cases: "Terminating" and "Ready" should fail (lines 185, 204-221)
The added comments clarify the relationship between test data and template expectations, which improves maintainability.
Also applies to: 67-67, 154-154, 185-185, 204-221, 230-230
internal/k8s_client/client.go (3)
6-6: LGTM: Well-defined constant for environment variable.Exposing
EnvKubeConfigas a public constant is good practice, allowing other packages to reference the same environment variable name consistently.Also applies to: 20-21
32-59: LGTM: Comprehensive documentation improvements.The updated documentation clearly explains the three-tier priority system for Kubernetes configuration resolution, with practical examples for both development and production scenarios.
64-84: LGTM: Clean priority-based configuration resolution.The refactored implementation clearly follows the documented priority order:
- Explicit
KubeConfigPathfrom configKUBECONFIGenvironment variable- In-cluster configuration
The code is more readable with explicit path resolution and appropriate logging at each step.
internal/criteria/evaluator.go (3)
255-264: LGTM: Clarified error behavior documentation.The updated documentation clearly states the return values on failure, helping callers understand the safe-default pattern for CEL evaluation.
272-316: LGTM: Well-designed expression definition helpers.The new constants and helper methods provide a clean API for evaluating CEL expressions defined in map structures with optional defaults. The implementation:
- Uses constants to prevent typos
- Handles both expression evaluation and direct value extraction
- Properly handles default values on evaluation failure
384-391: LGTM: Proper error propagation in negate wrapper.The updated
negatefunction now correctly propagates errors instead of silently converting them tofalse. This is an important correctness improvement that prevents masking evaluation errors.test/integration/config-loader/testdata/adapter_config_valid.yaml (1)
14-14: LGTM: Test data version aligned with PR objectives.The adapter version update from "0.0.1" to "0.1.0" keeps test data consistent with the version bump across the codebase.
internal/config_loader/validator_test.go (1)
29-29: LGTM: Consistent test data updates.All test cases have been updated to include
baseUrl: "https://test.example.com"in thehyperfleetApiconfiguration section. This ensures tests reflect the expected configuration structure.Also applies to: 61-61, 89-89, 116-116, 143-143, 170-170, 198-198, 226-226, 252-252, 304-304, 332-332, 359-359, 391-391, 455-455, 480-480, 506-506, 556-556, 586-586, 614-614, 642-642, 669-669, 699-699, 724-724, 870-870, 915-915, 1011-1011, 1035-1035, 1060-1060, 1082-1082, 1105-1105, 1127-1127
internal/config_loader/validator_schema.go (1)
199-199: LGTM: Flexible namespace handling for resource discovery.Allowing empty or
"*"for the namespace field is appropriate for all-namespace resource discovery, particularly useful for cluster-scoped resources or when discovering resources across multiple namespaces.internal/executor/param_extractor.go (2)
19-22: LGTM: More informative error messages.The enhanced error message now includes both the parameter name and source, making it much easier to troubleshoot extraction failures.
30-33: Verify that treating empty strings as missing values aligns with your intended behavior for empty environment variables.The change now applies defaults when parameter values are
nilor"". While test cases and config files show no intentional use of empty strings as valid parameter values, ensure this behavior is desired—particularly for environment variables that may be set but empty. The validator already treats empty env values similarly (checkingenvValue == ""for missing required params), so this change is consistent with existing validation logic.internal/executor/post_action_executor.go (5)
35-47: LGTM: Enhanced payload logging with per-payload success indication.The indexed logging provides good visibility into payload building progress. The loop at lines 45-47 iterates after successful build, which is appropriate.
52-70: LGTM: Consistent indexed logging for post-action execution.The
[PostAction i/n]format provides clear progress tracking and aligns with the logging pattern used in preconditions and resources phases.
79-81: LGTM: CEL context setup via GetCELVariables.Using
execCtx.GetCELVariables()ensures consistent CEL variable assembly across all executor phases, aligning with the types.go implementation.
157-168: LGTM: Expression and value definition handling.The updated flow correctly delegates to
EvaluateExpressionDeffor expressions andGetValueDeffor simple values, with proper template rendering for string values.
210-214: LGTM: API call logging.Logging the method/URL before the call and HTTP status after success provides good observability without exposing sensitive response data.
internal/executor/precondition_executor.go (4)
32-58: LGTM: Consistent indexed logging for precondition evaluation.The
[Precondition i/n]format with MET/NOT MET/EXECUTION ERROR outcomes provides clear visibility into precondition evaluation flow.
136-139: LGTM: CEL context setup via GetCELVariables.Using
execCtx.GetCELVariables()ensures consistent CEL variable assembly. The comment correctly notes that resources will be empty during preconditions.
158-166: LGTM: Detailed per-condition result logging.Logging individual condition results with field, operator, expected, and actual values provides excellent debugging visibility at the appropriate verbosity level.
175-186: LGTM: CEL expression evaluation logging.The CEL expression logging at V(1) for the expression itself and at Info level for the result provides appropriate visibility without being overly verbose.
internal/executor/resource_executor.go (6)
40-51: LGTM: Consistent indexed logging for resource processing.The
[Resource i/n]format with operation details (Kind, namespace/name, operation) provides excellent visibility into resource processing progress.
64-79: LGTM: Manifest building and info logging.The verbose log at V(1) for build start and info-level log for manifest details (Group/Kind, name, namespace) provide appropriate visibility.
84-101: LGTM: Discovery result logging with clear state indication.The explicit logging of whether an existing resource was found or will be created provides clear visibility into the discovery outcome.
107-121: LGTM: Operation determination logging.Logging the chosen operation (RECREATE/UPDATE/CREATE) with context (e.g.,
recreateOnChange=true) provides good traceability for debugging resource lifecycle decisions.
139-142: LGTM: Resource storage confirmation.The V(1) log confirming resource storage in context is helpful for debugging without adding noise at default verbosity.
201-206: LGTM: Clarifying comment for namespace handling.The comment explaining that empty namespace means all namespaces (normalized from "*" at config load time) is helpful documentation.
internal/executor/executor.go (6)
75-80: LGTM: Clear event receipt logging.The event metadata logging (ID, Type, Source, Time) with visual separators provides good visibility into incoming events without exposing payload data.
99-121: LGTM: Preconditions phase logging with count and outcome.The logging of configured precondition count and clear MET/NOT MET outcomes provides good phase visibility.
126-149: LGTM: Resources phase logging with detailed outcomes.Per-resource logging with Kind, namespace/name, and operation provides excellent traceability. Skip reasons are clearly logged.
154-177: LGTM: Post actions phase logging with HTTP status details.The conditional logging of HTTP status when
APICallMadeis true provides appropriate detail without redundant information.
185-198: LGTM: Clear execution completion logging.The final summary with SUCCESS/FAILED status, skip reasons, and failed phase details provides a clear execution outcome.
230-247: Verify ACK-on-failure strategy aligns with operational requirements.The updated strategy ACKs all messages (including failures) to prevent retry loops. This is appropriate for non-recoverable errors (4xx, validation failures) but means transient failures (network timeouts, 5xx) won't be retried automatically.
Ensure this aligns with the operational model. If some errors are recoverable, consider distinguishing between retryable and non-retryable errors, or implementing a dead-letter queue (DLQ) strategy as discussed in the previous review comment.
cmd/adapter/main.go (5)
40-97: LGTM: Clean Cobra CLI structure.The root command with serve and version subcommands provides a well-organized CLI. The glog flag integration via pflag and completion command disabling are appropriate.
100-156: LGTM: Well-structured serve initialization.The initialization flow (config → API client → K8s client → executor) is clean and well-logged. The builder pattern for executor creation is appropriate.
180-214: LGTM: Environment-based broker configuration.Reading subscription ID and topic from environment variables with clear error messages is appropriate. The non-blocking subscription pattern is correct.
245-265: LGTM: Graceful shutdown with timeout addresses previous review feedback.The 30-second timeout for subscriber close with goroutine-based timeout handling is a proper implementation of graceful shutdown, addressing the concern raised in a previous review.
272-301: LGTM: API client creation with proper option handling.The createAPIClient function correctly parses timeout, validates backoff strategy, and applies options. The error messages are clear and actionable.
configs/adapter-config-template.yaml (7)
28-33: Clear CEL optional chaining documentation.The new section provides helpful guidance with concrete examples for safe field access patterns. This should improve configuration clarity and reduce runtime errors from missing fields.
109-110: Verify API path changes are compatible with HyperFleet server.The precondition and postAction endpoints now include
/api/hyperfleet/prefix and change status endpoint from/statusto/statuses. This is a breaking change that requires server-side support.Confirm:
- HyperFleet API server has been updated to support the
/api/hyperfleet/prefix- The
/statusesendpoint exists and accepts the new payload structurehyperfleetApiBaseUrlenvironment variable does not already include/api(to avoid double-prefixing)Also applies to: 242-243
117-117: Verify field mappings against actual HyperFleet API response schema.The capture mappings reference:
field: "name"(previouslymetadata.name)field: "generation"(new, for generationId)Confirm these fields exist at the specified paths in the HyperFleet API response. If the response structure differs (e.g.,
metadata.nameinstead of top-levelname), the capture will fail silently and subsequent template expressions using{{ .clusterName }}will be empty.Also applies to: 120-121
91-95: Verify CloudEvent structure for clusterId extraction.The
clusterIdparameter now sources fromevent.idinstead of previous source. Confirm:
- CloudEvents published to the broker contain the cluster ID in the
idfield- The
idfield format and content match expectations for cluster identification- No other events could accidentally match this param extraction
172-213: Verify CEL framework supports optional chaining and context variable injection.The conditions now rely on:
- CEL optional chaining syntax (
?.orValue()) which must be supported by the CEL evaluator- Context variables like
adapter.executionStatus,adapter.errorReason,adapter.errorMessagethat must be injected at runtime- Resource variables like
resources.clusterNamespace.status.phasethat must be available in CEL contextConfirm:
- The CEL evaluator supports the
?.orValue()operator- Adapter execution context is properly injected with
adapter.*variables before condition evaluation- Resource status fields are accessible in the CEL context post-creation
- Long multi-line expressions (e.g., lines 182-184) are parsed and evaluated correctly
140-158: Well-structured resource definition with good label-based discovery.The resource definition includes:
- RFC 1123 compliant naming with
| lowerfilter (line 141)- Proper label strategy for multi-adapter resource isolation
- Clear discovery selectors for resource lookup
- Good clarification on namespace field semantics for cluster-scoped resources (lines 150-153)
The annotation referencing
{{ .generationId }}aligns well with the precondition capture.
215-231: Well-designed payload structure with proper type handling.The post-processing payload improvements include:
- Correct use of CEL expressions for numeric fields (line 217) to preserve type semantics
- ISO 8601 timestamp format with timezone (line 221)
- Clear separation between CEL expressions (computed fields) and Go templates (static values)
- Additional context in
datablock aids observability and debugging
xueli181114
force-pushed
the
HYPERFLEET-293
branch
from
e973d08 to
ea8a534
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/executor/utils.go (1)
138-141: Note: Security concern regarding POST body logging is tracked.As per the retrieved learnings, this logging concern is being addressed in HYPERFLEET-304. The current implementation logs the full request body on POST failures, which may expose sensitive data.
Based on learnings, logger refactor is tracked in HYPERFLEET-304.
🧹 Nitpick comments (5)
internal/config_loader/validator.go (1)
321-355: Align CEL env comments with newadapterrootThe new
adaptermap root in the CEL env looks correct and matches the config template’sadapter.*usage. For clarity, consider updating the capacity comment (now +3: optional types,resources,adapter) and/or markingaddedRoots[FieldAdapter] = trueso future readers seeadaptertreated like other roots.charts/templates/pdb.yaml (1)
2-6: PDB validation treats0as “unset”; confirm that’s intendedThe new checks correctly enforce that exactly one of
minAvailable/maxUnavailableis set and truthy. Note that in Helm,0(int) is falsy, sominAvailable: 0ormaxUnavailable: 0will hit the “neither set”fail. If0should be allowed, consider switching to a presence check (e.g., viahasKeyon.Values.podDisruptionBudget).Makefile (1)
5-5: Helm chart test target is useful; consider integrating into CI aggregatorsThe VERSION bump to
0.1.0and thetest-helm-chartsworkflow look good and give solid manual coverage of common Helm configurations. If you want chart validation to be enforced in CI, consider callingtest-helm-chartsfromtest-allorverify(or a dedicated CI job) so it’s not forgotten locally.Also applies to: 204-265
cmd/adapter/main.go (1)
41-96: Serve flow and shutdown look solid; consider surfacing fatal subscription errors via exit codeThe Cobra CLI wiring,
runServeflow (config → API client → k8s client → executor → broker), signal handling, and 30s timeout aroundsubscriber.Close()all look well thought out.One thing to consider: when
subscriber.Errors()sends a value, you log it as a “Fatal subscription error” and cancel the context, butrunServestill returnsnil, somainexits with status 0. If these broker failures should cause the container to be treated as failed (for restarts/alerting), it would be better to return that error (or wrap it) fromrunServesorootCmd.Execute()surfaces a non‑nil error and the process exits non‑zero.Also applies to: 98-268
internal/executor/utils.go (1)
281-328: Type conversion functions silently discard errors.The type conversion template helpers (
int,int64,float64) usestrconvfunctions but discard errors, returning zero values on parse failures. This silent failure mode may make debugging template issues difficult.Consider whether these helpers should:
- Return a sentinel value or error indicator on parse failure
- Log warnings when conversions fail
- Document this zero-default behavior explicitly
For example, at lines 290 and 305-306:
i, _ := strconv.Atoi(val) // Error discardedIf the current behavior is intentional (fail-safe defaults for templates), consider adding inline comments explaining this design choice.
📜 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 (41)
Dockerfile(1 hunks)Makefile(2 hunks)charts/templates/deployment.yaml(0 hunks)charts/templates/pdb.yaml(1 hunks)charts/values.yaml(1 hunks)cmd/adapter/main.go(3 hunks)configs/adapter-config-template.yaml(7 hunks)configs/adapter.yaml(2 hunks)go.mod(4 hunks)internal/broker_consumer/README.md(0 hunks)internal/broker_consumer/consumer.go(0 hunks)internal/config_loader/README.md(1 hunks)internal/config_loader/loader_test.go(22 hunks)internal/config_loader/validator.go(1 hunks)internal/config_loader/validator_schema.go(2 hunks)internal/config_loader/validator_test.go(31 hunks)internal/criteria/evaluator.go(2 hunks)internal/executor/executor.go(6 hunks)internal/executor/executor_test.go(1 hunks)internal/executor/param_extractor.go(1 hunks)internal/executor/post_action_executor.go(5 hunks)internal/executor/post_action_executor_test.go(1 hunks)internal/executor/precondition_executor.go(7 hunks)internal/executor/resource_executor.go(5 hunks)internal/executor/types.go(1 hunks)internal/executor/utils.go(4 hunks)internal/k8s_client/client.go(4 hunks)internal/k8s_client/mock.go(1 hunks)pkg/logger/logger.go(2 hunks)test/integration/broker_consumer/README.md(0 hunks)test/integration/broker_consumer/adapter_integration_test.go(0 hunks)test/integration/broker_consumer/setup_test.go(0 hunks)test/integration/broker_consumer/testutil_container.go(0 hunks)test/integration/broker_consumer/testutil_publisher.go(0 hunks)test/integration/config-loader/config_criteria_integration_test.go(7 hunks)test/integration/config-loader/loader_template_test.go(5 hunks)test/integration/config-loader/testdata/adapter-config-template.yaml(1 hunks)test/integration/config-loader/testdata/adapter_config_valid.yaml(1 hunks)test/integration/executor/executor_integration_test.go(1 hunks)test/integration/executor/main_test.go(2 hunks)test/testdata/adapter_config_valid.yaml(1 hunks)
💤 Files with no reviewable changes (8)
- test/integration/broker_consumer/testutil_container.go
- test/integration/broker_consumer/adapter_integration_test.go
- test/integration/broker_consumer/setup_test.go
- test/integration/broker_consumer/testutil_publisher.go
- internal/broker_consumer/consumer.go
- test/integration/broker_consumer/README.md
- internal/broker_consumer/README.md
- charts/templates/deployment.yaml
🚧 Files skipped from review as they are similar to previous changes (18)
- internal/executor/types.go
- internal/config_loader/validator_schema.go
- test/testdata/adapter_config_valid.yaml
- internal/executor/param_extractor.go
- test/integration/config-loader/testdata/adapter_config_valid.yaml
- internal/config_loader/README.md
- internal/executor/resource_executor.go
- internal/k8s_client/mock.go
- internal/config_loader/validator_test.go
- Dockerfile
- test/integration/executor/executor_integration_test.go
- test/integration/config-loader/loader_template_test.go
- configs/adapter.yaml
- test/integration/config-loader/testdata/adapter-config-template.yaml
- charts/values.yaml
- internal/executor/executor.go
- pkg/logger/logger.go
- internal/executor/post_action_executor_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-11T02:06:33.975Z
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 17
File: internal/config_loader/validator_schema.go:126-133
Timestamp: 2025-12-11T02:06:33.975Z
Learning: In the hyperfleet-adapter repository (Go), environment variable validation for required parameters (those with `env.*` source, `Required=true`, and no default) occurs at config load time in `internal/config_loader/validator_schema.go`. This is intentional design - environment variables must be set before deployment, and the validation ensures fail-fast behavior with clear error messages.
Applied to files:
internal/config_loader/validator.gointernal/executor/post_action_executor.gocmd/adapter/main.gotest/integration/config-loader/config_criteria_integration_test.gointernal/config_loader/loader_test.go
📚 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/utils.go
🧬 Code graph analysis (10)
internal/config_loader/validator.go (1)
internal/config_loader/constants.go (1)
FieldAdapter(15-15)
test/integration/executor/main_test.go (1)
test/integration/executor/setup_test.go (1)
K8sTestEnv(16-22)
internal/executor/post_action_executor.go (4)
internal/executor/types.go (3)
PhasePostActions(28-28)PostActionResult(153-170)NewExecutorError(370-377)internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
GetValueDef(313-316)internal/config_loader/types.go (1)
APICall(108-116)
internal/k8s_client/client.go (2)
pkg/logger/logger.go (1)
Logger(11-20)pkg/errors/error.go (1)
KubernetesError(203-205)
internal/executor/precondition_executor.go (5)
internal/executor/types.go (2)
PreconditionsOutcome(380-390)PhasePreconditions(24-24)internal/config_loader/types.go (1)
APICall(108-116)internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
NewEvaluator(52-60)internal/config_loader/constants.go (1)
FieldValue(72-72)
internal/executor/executor_test.go (1)
pkg/logger/logger.go (1)
Flush(158-160)
test/integration/config-loader/config_criteria_integration_test.go (2)
internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
internal/criteria/evaluator.go (1)
internal/criteria/cel_evaluator.go (1)
CELEvaluator(17-21)
internal/config_loader/loader_test.go (2)
internal/config_loader/types.go (4)
AdapterConfig(6-11)AdapterConfigSpec(21-29)Resource(171-178)DiscoveryConfig(181-185)internal/k8s_client/discovery.go (2)
Discovery(14-29)DiscoveryConfig(32-42)
internal/executor/utils.go (2)
pkg/errors/error.go (1)
New(107-123)internal/config_loader/loader.go (1)
Parse(114-130)
🔇 Additional comments (19)
internal/executor/executor_test.go (1)
38-38: LGTM! Flush() method correctly added to satisfy the Logger interface.The no-op implementation is appropriate for a test mock and consistent with the other mock methods.
test/integration/executor/main_test.go (1)
76-76: LGTM! Proper Go naming convention applied.The function name has been corrected from
setupsharedK8sEnvtestEnvtosetupSharedK8sEnvtestEnv, properly capitalizing "Shared" to follow Go camelCase conventions. The function correctly remains unexported since it's a test helper used only within this package.Also applies to: 101-102
internal/config_loader/loader_test.go (2)
28-30: LGTM: Version bump and baseUrl additions are consistent.The version update to
"0.1.0"and newbaseUrlfield align with the PR's goal of updating to the latest broker lib version requirements.Also applies to: 71-71
1152-1167: LGTM: Namespace validation relaxation aligns with discovery semantics.The test case correctly reflects that an empty namespace now implies "all namespaces" for discovery, matching the
DiscoveryConfiginterface whereGetNamespace()returning empty means cluster-scoped or all namespaces.test/integration/config-loader/config_criteria_integration_test.go (3)
18-23: LGTM: TestMain correctly sets required environment variable for integration tests.Setting
HYPERFLEET_API_BASE_URLbefore running tests ensures the adapter config template loads successfully. This aligns with the learned behavior that environment variable validation occurs at config load time for fail-fast behavior. Based on learnings.
47-48: LGTM: Test data updated to match template precondition expectations.The shift to
NotReadyphase reflects the adapter config template's precondition requirements. The comment clarifies the coupling between test data and template configuration.Also applies to: 67-67
204-222: LGTM: Good negative test coverage for phase mismatch.This test case properly validates that preconditions fail when
clusterPhaseis"Ready"but the template expects"NotReady". Combined with the"Terminating"test case above, this provides solid coverage for phase validation.go.mod (1)
12-16: Dependency wiring matches new CLI/broker usageAdding
cobra/pflagand promotingx/textto a direct dep, plus moving pubsub/uuid/grpc to indirect and addingmousetrap, all look consistent with the new CLI and broker integration.Please ensure you’ve run
go mod tidyand your usual test suite on this branch so the module graph and sums stay clean after these changes.Also applies to: 30-31, 66-72, 129-137
internal/k8s_client/client.go (1)
6-7: KUBECONFIG precedence and logging look goodThe updated
NewClientflow (explicitKubeConfigPath→KUBECONFIG→ in-cluster) plus logging of the chosen source is a solid improvement for both dev and production; no further changes needed from my side.Also applies to: 20-22, 32-38, 42-59, 64-84
cmd/adapter/main.go (1)
270-300:createAPIClientcentralizes HyperFleet API options cleanlyThe helper cleanly parses the timeout via
ParseTimeout, guardsRetryBackoffwith an explicit allowlist, and wires all options throughhyperfleet_api.NewClient. This keeps API client construction logic in one place and improves validation; no changes requested.configs/adapter-config-template.yaml (1)
28-33: Adapter config template aligns well with new CEL context and status semanticsThe updates—version
0.1.0,/api/hyperfleet/.../statusespaths,clusterIdfromevent.id,generationIdcapture, namespace naming viaclusterId | lower, and the CEL-based conditions/data usingresources.?...andadapter.?...—all look consistent with the new CEL environment and executor behavior. I don’t see issues with the template structure.Also applies to: 53-54, 92-96, 109-122, 135-159, 167-232, 241-247
internal/executor/post_action_executor.go (1)
35-48: Post-action payload building and logging integrate cleanly with the new CEL pipelineUsing
execCtx.GetCELVariables()+criteria.NewEvaluator, theEvaluateExpressionDef/GetValueDefhandling inprocessValue, and the richer per-payload / per-post-action logging all look correct and aligned with the updated config schema (expression/value maps, adapter/resources CEL usage). Error propagation viaExecutionErrorandNewExecutorError(PhasePostActions, ...)is also consistent. No changes requested here.Also applies to: 52-71, 79-113, 131-194, 197-215, 221-245
internal/executor/utils.go (1)
249-329: Template function map is well-structured.The addition of
templateFuncsprovides a comprehensive set of utilities for template rendering, including time formatting, string manipulation, and type conversions. The use ofgolang.org/x/text/casesfor proper title casing is appropriate.internal/executor/precondition_executor.go (3)
32-59: Enhanced precondition logging improves observability.The addition of indexed logging (
[Precondition %d/%d]) with explicit status markers (MET ✓, NOT MET, EXECUTION ERROR) significantly improves debugging and monitoring capabilities. This follows a consistent pattern across the execution flow.
136-139: Correct usage of CEL variables from execution context.The change to use
execCtx.GetCELVariables()instead of directParamsaccess aligns with the broader CEL evaluation infrastructure introduced in this PR. The comment correctly notes that resources will be empty during preconditions.
159-165: Detailed condition logging aids troubleshooting.Per-condition result logging with actual vs. expected values provides valuable debugging information. The use of log level V(1) for successful conditions and Info level for failures strikes a good balance between verbosity and usefulness.
internal/criteria/evaluator.go (3)
384-391: Good fix: Error propagation in negation logic.The updated
negatefunction now correctly propagates errors from the wrapped evaluation function instead of discarding them. This prevents silent failures in negated conditions (e.g.,NotEquals,NotIn).Before: Errors were lost, returning
(false, nil)regardless of the underlying error.
After: Errors are properly returned, only the boolean result is negated on success.
278-316: Expression and value definition helpers are well-designed.The new
EvaluateExpressionDefandGetValueDefmethods provide a clean API for handling expression definitions with optional defaults. The logic correctly:
- Validates expression presence
- Evaluates CEL expressions
- Falls back to defaults on failure
- Returns appropriate success indicators
The constants (
ExpressionKey,DefaultKey,ValueKey) ensure consistency across the codebase.
347-367: New condition definition types support structured CEL workflows.The addition of
ConditionDefandConditionDefJSONwith theToConditionDefconversion helper provides a typed representation for conditions, supporting the CEL-based evaluation infrastructure. This aligns well with the executor changes in this PR.
xueli181114
force-pushed
the
HYPERFLEET-293
branch
from
ea8a534 to
b04dac6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/executor/post_action_executor.go (1)
157-168: CEL evaluation errors are silently suppressed inEvaluateExpressionDef.When
EvaluateExpressionDefcatches a CEL evaluation error (line 297 ininternal/criteria/evaluator.go), it returnsnil, trueif no default is specified. This causesprocessValue(line 158) to return thenilresult without distinguishing between a successful evaluation that yieldedniland a failed evaluation. CEL errors are caught but their details are lost, making it impossible for callers to detect evaluation failures. Consider returning an error value alongside the result, or logging CEL failures so debugging issues becomes easier.
♻️ Duplicate comments (2)
internal/executor/utils.go (1)
138-141: Security concern: Request body may contain sensitive data.This concern was previously raised. Logging the full request body on failure could expose secrets, tokens, or PII. Based on learnings, this is tracked for the logger refactor in HYPERFLEET-304.
internal/executor/executor.go (1)
92-94: Potential sensitive data exposure in logs - tracked for logger refactor.This concern was previously raised. Logging parameter values could expose secrets if they flow through the params map. Based on learnings, this is tracked for the logger refactor in HYPERFLEET-304.
🧹 Nitpick comments (2)
internal/criteria/evaluator.go (1)
255-257: Expression/value definition helpers look solid; consider documentingboolmeaningThe new constants plus:
EvaluateExpressionDef(def map[string]any) (any, bool)andGetValueDef(def map[string]any) (any, bool)provide a clear way to handle
{expression, default}vs{value}maps, and the CEL evaluation path (including default fallback andHasError()check) is sensible.One minor suggestion: the doc comments don’t currently describe the second return value; clarifying that the
boolindicates “definition present/recognized” (vs no usableexpression/value) would make call‑site intent clearer.Also applies to: 263-265, 271-276, 286-306, 308-316
internal/executor/precondition_executor.go (1)
91-97: Fix inconsistent indentation in error handling block.The indentation of
execCtx.Adapter.ExecutionErrorassignment (lines 91-95) appears inconsistent with the surrounding code. This may cause confusion during code review and maintenance.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) + Phase: string(PhasePreconditions), + Step: precond.Name, + Message: err.Error(), + } + + return result, NewExecutorError(PhasePreconditions, precond.Name, "API call failed", 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 (41)
Dockerfile(1 hunks)Makefile(3 hunks)charts/templates/deployment.yaml(1 hunks)charts/templates/pdb.yaml(1 hunks)charts/values.yaml(1 hunks)cmd/adapter/main.go(3 hunks)configs/adapter-config-template.yaml(7 hunks)configs/adapter.yaml(2 hunks)go.mod(4 hunks)internal/broker_consumer/README.md(0 hunks)internal/broker_consumer/consumer.go(0 hunks)internal/config_loader/README.md(1 hunks)internal/config_loader/loader_test.go(22 hunks)internal/config_loader/validator.go(1 hunks)internal/config_loader/validator_schema.go(2 hunks)internal/config_loader/validator_test.go(31 hunks)internal/criteria/evaluator.go(2 hunks)internal/executor/executor.go(6 hunks)internal/executor/executor_test.go(1 hunks)internal/executor/param_extractor.go(1 hunks)internal/executor/post_action_executor.go(5 hunks)internal/executor/post_action_executor_test.go(1 hunks)internal/executor/precondition_executor.go(7 hunks)internal/executor/resource_executor.go(5 hunks)internal/executor/types.go(1 hunks)internal/executor/utils.go(4 hunks)internal/k8s_client/client.go(4 hunks)internal/k8s_client/mock.go(1 hunks)pkg/logger/logger.go(2 hunks)test/integration/broker_consumer/README.md(0 hunks)test/integration/broker_consumer/adapter_integration_test.go(0 hunks)test/integration/broker_consumer/setup_test.go(0 hunks)test/integration/broker_consumer/testutil_container.go(0 hunks)test/integration/broker_consumer/testutil_publisher.go(0 hunks)test/integration/config-loader/config_criteria_integration_test.go(7 hunks)test/integration/config-loader/loader_template_test.go(5 hunks)test/integration/config-loader/testdata/adapter-config-template.yaml(1 hunks)test/integration/config-loader/testdata/adapter_config_valid.yaml(1 hunks)test/integration/executor/executor_integration_test.go(1 hunks)test/integration/executor/main_test.go(2 hunks)test/testdata/adapter_config_valid.yaml(1 hunks)
💤 Files with no reviewable changes (7)
- test/integration/broker_consumer/setup_test.go
- test/integration/broker_consumer/testutil_publisher.go
- test/integration/broker_consumer/adapter_integration_test.go
- internal/broker_consumer/README.md
- test/integration/broker_consumer/testutil_container.go
- test/integration/broker_consumer/README.md
- internal/broker_consumer/consumer.go
🚧 Files skipped from review as they are similar to previous changes (19)
- test/integration/config-loader/testdata/adapter_config_valid.yaml
- configs/adapter.yaml
- test/integration/executor/executor_integration_test.go
- internal/k8s_client/mock.go
- internal/executor/executor_test.go
- internal/executor/types.go
- charts/templates/pdb.yaml
- test/integration/executor/main_test.go
- internal/config_loader/validator.go
- internal/config_loader/validator_test.go
- test/integration/config-loader/loader_template_test.go
- internal/executor/param_extractor.go
- internal/config_loader/loader_test.go
- charts/values.yaml
- test/integration/config-loader/testdata/adapter-config-template.yaml
- go.mod
- Makefile
- Dockerfile
- internal/executor/post_action_executor_test.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 17
File: internal/config_loader/validator_schema.go:126-133
Timestamp: 2025-12-11T02:06:33.975Z
Learning: In the hyperfleet-adapter repository (Go), environment variable validation for required parameters (those with `env.*` source, `Required=true`, and no default) occurs at config load time in `internal/config_loader/validator_schema.go`. This is intentional design - environment variables must be set before deployment, and the validation ensures fail-fast behavior with clear error messages.
📚 Learning: 2025-12-11T02:06:33.975Z
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 17
File: internal/config_loader/validator_schema.go:126-133
Timestamp: 2025-12-11T02:06:33.975Z
Learning: In the hyperfleet-adapter repository (Go), environment variable validation for required parameters (those with `env.*` source, `Required=true`, and no default) occurs at config load time in `internal/config_loader/validator_schema.go`. This is intentional design - environment variables must be set before deployment, and the validation ensures fail-fast behavior with clear error messages.
Applied to files:
test/integration/config-loader/config_criteria_integration_test.gotest/testdata/adapter_config_valid.yamlinternal/executor/post_action_executor.gocharts/templates/deployment.yamlcmd/adapter/main.gointernal/config_loader/README.md
📚 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/utils.gointernal/executor/executor.go
📚 Learning: 2025-12-11T02:06:26.232Z
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 17
File: internal/config_loader/validator_schema.go:126-133
Timestamp: 2025-12-11T02:06:26.232Z
Learning: In Go projects, keep required environment variables validated at config load time (as in internal/config_loader/validator_schema.go) by checking variables annotated as env.* with Required=true and no default, fail-fast on startup with clear error messages. This ensures deployment preconditions are enforced and misconfig is reported immediately. Add unit tests that simulate missing envs and verify early exit with informative messages.
Applied to files:
internal/config_loader/validator_schema.go
🧬 Code graph analysis (5)
test/integration/config-loader/config_criteria_integration_test.go (2)
internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
NewEvaluator(52-60)
internal/executor/post_action_executor.go (4)
internal/executor/types.go (2)
PostActionResult(153-170)NewExecutorError(370-377)internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
GetValueDef(313-316)internal/config_loader/types.go (1)
APICall(108-116)
internal/k8s_client/client.go (2)
pkg/logger/logger.go (1)
Logger(11-20)pkg/errors/error.go (1)
KubernetesError(203-205)
internal/executor/resource_executor.go (4)
internal/k8s_client/discovery.go (1)
Discovery(14-29)pkg/errors/k8s_error.go (1)
IsRetryableDiscoveryError(160-192)internal/executor/types.go (5)
NewExecutorError(370-377)PhaseResources(26-26)OperationRecreate(147-147)OperationUpdate(145-145)OperationCreate(143-143)internal/config_loader/types.go (1)
Resource(171-178)
internal/criteria/evaluator.go (1)
internal/criteria/cel_evaluator.go (1)
CELEvaluator(17-21)
🔇 Additional comments (31)
test/testdata/adapter_config_valid.yaml (2)
14-14: Version bump to 0.1.0 aligns with adapter versioning changes.The version update is consistent with the broader adapter release versioning in this PR.
47-54: Verify that broker lib handles authentication for API calls in preconditions and postActions.The removal of
hyperfleetApiTokenand Authorization headers from both the precondition GET request (line 51) and postAction POST request (line 185) indicates a shift to broker lib–managed authentication. This is consistent with the PR objective ("use latest broker lib"), but needs verification that:
- The broker lib properly authenticates the precondition API call to fetch cluster status
- The broker lib properly authenticates the postAction API call to report cluster status
- Credentials (if needed) are correctly sourced from the broker configuration or environment
Without explicit auth headers in the config, the authentication mechanism is now implicit. Confirm that both API interactions succeed with the new broker lib without requiring explicit token/credential configuration in the adapter config.
Also applies to: 181-189
internal/config_loader/README.md (1)
50-52: Adapter version in README sample updated correctlySample config now shows adapter version
0.1.0, matching the new adapter version across the repo. Looks good.pkg/logger/logger.go (1)
19-20: Instance-levelFlushmatches existing package-level behaviorAdding
Flush()to theLoggerinterface and implementing it on*loggerby delegating toglog.Flush()is consistent with the existing package-levelFlush()helper and enables call‑site flexibility. No issues spotted.Also applies to: 147-150
internal/config_loader/validator_schema.go (2)
122-133: Requiredenv.*params are now validated at config loadThe new block in
validateParamsenforces that required params sourced fromenv.*must have the corresponding environment variable set (or a default), otherwise validation fails. This cleanly encodes the “required env at deployment time” contract and fits the fail‑fast behavior you described for this repo. Based on learnings, this is the intended design.
194-207: Discovery namespace comment now matches validator behaviorThe comment that
Namespaceis optional (empty or"*"meaning all namespaces) aligns with the current validator, which only requiresbyNameorbySelectorsand does not enforceNamespace. The clarification is helpful and matches the existing logic here.test/integration/config-loader/config_criteria_integration_test.go (3)
18-23: TestMain correctly seeds required env for config validationSetting
HYPERFLEET_API_BASE_URLinTestMainensures the integration tests satisfy the new requiredenv.*param validation when loading the adapter config template. This keeps tests aligned with the fail‑fast env validation behavior. Based on learnings, this is exactly what’s expected.
47-49: Tests consistently reflectclusterPhase == "NotReady"semanticsUpdating the evaluation context and cluster details to use
"NotReady"for passing cases, and asserting failures for"Terminating"and"Ready", keeps the integration tests in sync with the new template precondition (clusterPhaseequalsNotReady). Both the condition‑by‑condition and combined/CEL paths are covered and look consistent.Also applies to: 67-68, 154-155, 183-186, 204-222
230-233: vpcId exists-check remains a good generic null-safety regressionThe
EvaluateConditionSafe("vpcId", OperatorExists, true)test (withvpcIdunset) is a nice generic guard to ensure the exists operator behaves safely when fields are missing, independent of the specific template. No changes needed.charts/templates/deployment.yaml (1)
137-151: BrokerenvKeysgating cleanly separates per-key vs full ConfigMap injectionUsing
{{- if and .Values.broker.mountAsEnv .Values.broker.envKeys }}before rangingenvKeysensures:
- Selected keys are projected as individual env vars when
envKeysis non‑empty.- The later
envFromblock handles the fallback whenmountAsEnvis true butenvKeysis unset.This yields predictable behavior for both patterns without extra conditionals.
internal/k8s_client/client.go (1)
20-22: Kubeconfig resolution priority is clear and correctly implemented
NewClientnow resolves config as:
ClientConfig.KubeConfigPathif set,- else
KUBECONFIG(viaEnvKubeConfig),- else in‑cluster
rest.InClusterConfig(),with informative logging for the chosen path. This matches the updated docs on
ClientConfig.KubeConfigPathand makes local vs in‑cluster behavior explicit without changing in‑cluster defaults.Also applies to: 31-33, 64-77
internal/criteria/evaluator.go (1)
383-392: Operator helpers now propagate errors and guard nil/invalid inputsThe changes to:
negate(returning the underlying error instead of silently invertingfalse),evaluateIn(explicit nil and invalidreflect.Valuechecks), andevaluateContains(nil guards for both operands, safer map key handling, and validity checks in slice/array traversal)significantly reduce the risk of panics and make operator failures explicit. This is a good robustness improvement for both direct condition evaluation and CEL‑backed flows.
Also applies to: 409-420, 438-505
configs/adapter-config-template.yaml (3)
28-33: CEL optional chaining usage is consistent and null-safeThe added guidance on optional chaining and the use of:
resources.?clusterNamespace.?status.?phase.orValue("")adapter.?executionStatus.orValue("")- similar patterns in
data.namespace.name/statusin the status payload makes the template much more robust to partially populated resources and adapter metadata. This lines up well with the new CEL helpers in
criteriaand should prevent “missing field” errors in post‑processing.Also applies to: 167-214, 224-232
53-53: Adapter version and HyperFleet status endpoint updates look coherent
spec.adapter.versionis bumped to"0.1.0", matching the rest of the PR.- HyperFleet API URLs now go through
{{ .hyperfleetApiBaseUrl }}/api/hyperfleet/{{ .hyperfleetApiVersion }}and the status reporter posts to.../clusters/{{ .clusterId }}/statuses.Assuming the backend expects the new
/api/hyperfleet/.../statusesshape, the config side wiring is consistent.Also applies to: 107-114, 239-244
92-96: Cluster identity, generation capture, and namespace resource wiring are aligned
clusterIdnow coming fromevent.idis reflected throughout (namespace name via{{ .clusterId | lower }}and labels).- The
clusterStatusprecondition capturesgenerationIdfromgenerationand the namespace annotationhyperfleet.io/generationuses that, preserving traceability.- Discovery for the
clusterNamespaceresource omitsnamespace, matching the comment that it’s cluster‑scoped and relying on label selectors instead.These changes are internally consistent and agree with the updated integration tests that key off
clusterPhase == "NotReady".Also applies to: 115-123, 135-149
internal/executor/utils.go (2)
248-329: Template helper functions look good overall, with a few observations.The
templateFuncsmap provides useful helpers. A few notes:
- The
dateanddateFormatfunctions are identical - consider removing one for clarity.- Type conversion functions (
int,int64,float64) silently return0on parse errors (e.g., line 290-291), which could mask issues in templates. Consider whether logging a warning or providing an error-aware variant would be beneficial.- The
defaultfunction follows Helm's argument order convention (default defaultVal val), which is good for familiarity.
338-338: LGTM - Template function registration is correctly implemented.Using
.Funcs(templateFuncs)before.Parse()ensures the functions are available during parsing. TheOption("missingkey=error")setting helps catch missing variable errors early.internal/executor/resource_executor.go (3)
40-51: LGTM - Enhanced per-resource logging improves observability.The indexed logging (
[Resource i/n]) provides clear progress tracking. The success log includes key details: name, kind, namespace, resource name, and operation.One minor note: the Unicode checkmark (✓) in logs should work fine in most modern logging systems, but verify your log aggregation tooling handles UTF-8 correctly.
97-101: LGTM - Discovery result logging aids debugging.Clear logging of whether an existing resource was found or will be created helps with troubleshooting discovery behavior.
201-203: LGTM - Good documentation of namespace semantics.The comment clarifies that empty namespace means all namespaces, which aligns with the normalization behavior in the config loader.
internal/executor/post_action_executor.go (2)
35-47: LGTM - Post payload building with success logging.The logging of payload count and per-payload success messages provides good visibility into the build phase.
79-83: LGTM - CEL evaluation context properly initialized.Using
execCtx.GetCELVariables()ensures consistent variable availability (params, adapter, resources) for CEL expressions in payload building.internal/executor/precondition_executor.go (2)
32-58: LGTM - Per-precondition logging with clear status indicators.The indexed logging (
[Precondition i/n]) with EXECUTION ERROR, NOT MET, and MET ✓ status markers provides excellent visibility into precondition evaluation flow.
136-141: LGTM - Good documentation of CEL evaluation context state.The comment clarifying that resources will be empty during preconditions (since they haven't been created yet) is helpful for understanding the evaluation context at this phase.
cmd/adapter/main.go (4)
40-59: LGTM - Cobra CLI setup is well-structured.The root command with glog flag integration via
pflag.CommandLine.AddGoFlagSet(flag.CommandLine)is correctly set up. Settinglogtostderr=trueby default is appropriate for containerized environments.
182-194: LGTM - Environment variable validation follows fail-fast pattern.The required environment variables (
BROKER_SUBSCRIPTION_ID,BROKER_TOPIC) are validated at startup with clear error messages. Based on learnings, this fail-fast approach is intentional design for the adapter.
222-234: All subscription errors trigger shutdown - verify this is intended.Currently, every error from
subscriber.Errors()triggers a shutdown signal. This may be too aggressive for transient/recoverable errors (e.g., temporary network issues).Per past discussion, distinguishing between recoverable and fatal errors (with potential DLQ support) is planned for future work.
247-267: LGTM - Graceful shutdown with timeout is well-implemented.The 30-second timeout for subscriber close allows adequate time for queue draining while preventing indefinite hangs. The goroutine + channel pattern correctly handles both successful close and timeout scenarios.
internal/executor/executor.go (3)
75-80: LGTM - Structured event header logging enhances observability.The EVENT RECEIVED header with ID, Type, Source, and Time provides excellent context for log correlation and debugging. The separator lines help visually distinguish events in log output.
228-247: ACK-all error handling strategy is documented - verify message loss is acceptable.The strategy of ACKing all messages (including failures) prevents retry loops for non-recoverable errors like 4xx responses. However, this means potentially recoverable errors (e.g., transient 5xx, network issues) won't be retried either.
Per past review discussion, distinguishing error types and potentially using a DLQ is planned for future work. Ensure this behavior is acceptable for your reliability requirements.
154-158: LGTM - Defensive nil check for post action count.The nil check on
e.config.AdapterConfig.Spec.Postbefore accessingPostActionsprevents potential nil pointer dereference.
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ciaranRoche The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 370691a
into
openshift-hyperfleet:main
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.