HYPERFLEET-387 | refactor: refactor config_loader with better validator #21
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis PR refactors the config validation pipeline by replacing the monolithic SchemaValidator with a new modular Validator that separates structural, file-reference, and semantic validation phases. It integrates go-playground validator tags for declarative structural checks, introduces ActionBase and FieldExpressionDef (affecting Precondition, PostAction, CaptureField), adds parameter type conversion utilities in the executor, updates criteria API signatures, and updates tests and integration fixtures to match the new types and flows. Sequence Diagram(s)sequenceDiagram
participant Client as Config Loader
participant Validator as Validator
participant StructVal as Struct Validator
participant FileLoader as File Loader
participant CELEnv as CEL Environment
participant Semantic as Semantic Validator
Client->>Validator: NewValidator(config, baseDir)
Validator->>StructVal: ValidateStruct(config)
alt Structural errors
StructVal-->>Client: Fail-fast error
else Structural OK
StructVal-->>Validator: OK
Validator->>FileLoader: ValidateFileReferences(baseDir)
alt File errors
FileLoader-->>Client: Return file errors
else Files OK
FileLoader-->>Validator: OK
Validator->>CELEnv: init CEL env
Validator->>Semantic: ValidateSemantic()
alt Semantic errors
Semantic-->>Client: Aggregated errors
else All valid
Semantic-->>Client: nil
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 (5)
internal/config_loader/struct_validator.go (1)
30-48: Consider logging custom validation registration errors.The custom validation registrations (lines 35-36) ignore errors using
_. While these registrations are unlikely to fail, consider logging any errors to aid debugging during development:🔎 Optional improvement to log registration errors
func getStructValidator() *validator.Validate { structValidatorOnce.Do(func() { structValidator = validator.New() // Register custom validations - _ = structValidator.RegisterValidation("resourcename", validateResourceName) - _ = structValidator.RegisterValidation("validoperator", validateOperator) + if err := structValidator.RegisterValidation("resourcename", validateResourceName); err != nil { + panic(fmt.Sprintf("failed to register resourcename validation: %v", err)) + } + if err := structValidator.RegisterValidation("validoperator", validateOperator); err != nil { + panic(fmt.Sprintf("failed to register validoperator validation: %v", err)) + } // Use yaml tag names for field names in errors structValidator.RegisterTagNameFunc(func(fld reflect.StructField) string {Note: Using
panicinsync.Onceinitialization is acceptable since these registrations should never fail in production and indicate a programming error if they do.internal/config_loader/README.md (1)
117-124: Add language specifier to fenced code block.The error message example code block at line 119 is missing a language specifier. This was flagged by markdownlint.
🔎 Proposed fix
### Error Messages Validation errors are descriptive: -``` +```text spec.params[0].name is required spec.preconditions[1].apiCall.method "INVALID" is invalid (allowed: GET, POST, PUT, PATCH, DELETE) spec.resources[0].name "my-resource": must start with lowercase letter and contain only letters, numbers, underscores (no hyphens) spec.preconditions[0].capture[0]: must have either 'field' or 'expression' set</details> </blockquote></details> <details> <summary>internal/config_loader/validator.go (1)</summary><blockquote> `606-620`: **Fragile error filtering using substring matching.** The `validateResources` helper filters errors by checking if the message contains "resources" or "Resources". This approach is brittle and may incorrectly include/exclude errors as message formats evolve. Consider tracking error paths more explicitly rather than relying on substring matching, or document this as a temporary backward-compatibility shim that should be removed once callers migrate to the new API. </blockquote></details> <details> <summary>internal/config_loader/types.go (2)</summary><blockquote> `143-149`: **LGTM!** Good extraction of common fields into a shared base type, reducing duplication between `Precondition` and `PostAction`. Minor: Consider adding `validate:"omitempty"` to `Log` field for consistency with `APICall`, though it's not strictly required for a pointer field. --- `160-169`: **LGTM!** Appropriate validation for `Method` (restricted to valid HTTP verbs) and `URL` (required). Consider adding format validation for `URL` (e.g., `validate:"required,url"`) to catch malformed URLs early, though this may be intentionally flexible to allow templated URLs. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 2acf251f6d62a3dcc6582452be5fb53a713c25a5 and c02962c68a1df52b85803bbea1ef3b27d4d1e3f5. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (21)</summary> * `configs/adapter-config-template.yaml` * `go.mod` * `internal/config_loader/README.md` * `internal/config_loader/loader.go` * `internal/config_loader/loader_test.go` * `internal/config_loader/struct_validator.go` * `internal/config_loader/types.go` * `internal/config_loader/validator.go` * `internal/config_loader/validator_schema.go` * `internal/config_loader/validator_test.go` * `internal/criteria/README.md` * `internal/executor/README.md` * `internal/executor/executor_test.go` * `internal/executor/param_extractor.go` * `internal/executor/param_extractor_test.go` * `internal/executor/post_action_executor_test.go` * `internal/executor/precondition_executor.go` * `test/integration/config-loader/testdata/adapter_config_valid.yaml` * `test/integration/executor/executor_integration_test.go` * `test/integration/executor/executor_k8s_integration_test.go` * `test/testdata/adapter_config_valid.yaml` </details> <details> <summary>💤 Files with no reviewable changes (3)</summary> * test/integration/config-loader/testdata/adapter_config_valid.yaml * test/testdata/adapter_config_valid.yaml * internal/config_loader/validator_schema.go </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (2)</summary> <details> <summary>📓 Common learnings</summary>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 withenv.*source,Required=true, and no default) occurs at config load time ininternal/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.</details> <details> <summary>📚 Learning: 2025-12-11T02:06:33.975Z</summary>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 withenv.*source,Required=true, and no default) occurs at config load time ininternal/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/struct_validator.go` - `internal/executor/param_extractor.go` - `internal/config_loader/validator.go` - `internal/config_loader/README.md` - `internal/config_loader/validator_test.go` - `internal/config_loader/loader.go` - `internal/config_loader/types.go` </details> </details><details> <summary>🧬 Code graph analysis (8)</summary> <details> <summary>internal/executor/precondition_executor.go (2)</summary><blockquote> <details> <summary>internal/criteria/types.go (2)</summary> * `Operator` (10-10) * `OperatorExists` (28-28) </details> <details> <summary>internal/config_loader/constants.go (1)</summary> * `FieldValue` (72-72) </details> </blockquote></details> <details> <summary>internal/executor/param_extractor.go (1)</summary><blockquote> <details> <summary>internal/executor/types.go (2)</summary> * `NewExecutorError` (374-381) * `PhaseParamExtraction` (21-21) </details> </blockquote></details> <details> <summary>test/integration/executor/executor_k8s_integration_test.go (1)</summary><blockquote> <details> <summary>internal/config_loader/types.go (4)</summary> * `ActionBase` (145-149) * `APICall` (161-169) * `CaptureField` (182-185) * `FieldExpressionDef` (13-18) </details> </blockquote></details> <details> <summary>internal/executor/post_action_executor_test.go (1)</summary><blockquote> <details> <summary>internal/config_loader/types.go (2)</summary> * `ActionBase` (145-149) * `LogAction` (261-264) </details> </blockquote></details> <details> <summary>internal/config_loader/validator.go (5)</summary><blockquote> <details> <summary>internal/config_loader/types.go (2)</summary> * `AdapterConfig` (69-74) * `ValidationErrors` (286-288) </details> <details> <summary>internal/config_loader/struct_validator.go (1)</summary> * `ValidateStruct` (62-82) </details> <details> <summary>internal/config_loader/loader.go (1)</summary> * `SupportedAPIVersions` (26-28) </details> <details> <summary>internal/config_loader/constants.go (13)</summary> * `FieldSpec` (9-9) * `FieldParams` (18-18) * `FieldResources` (20-20) * `FieldPost` (21-21) * `FieldPayloads` (41-41) * `FieldBuildRef` (43-43) * `FieldManifest` (78-78) * `FieldRef` (86-86) * `FieldRefs` (87-87) * `FieldPreconditions` (19-19) * `FieldConditions` (50-50) * `FieldCapture` (49-49) * `FieldExpression` (51-51) </details> <details> <summary>internal/criteria/types.go (2)</summary> * `Operator` (10-10) * `OperatorExists` (28-28) </details> </blockquote></details> <details> <summary>test/integration/executor/executor_integration_test.go (1)</summary><blockquote> <details> <summary>internal/config_loader/types.go (5)</summary> * `ActionBase` (145-149) * `APICall` (161-169) * `CaptureField` (182-185) * `FieldExpressionDef` (13-18) * `LogAction` (261-264) </details> </blockquote></details> <details> <summary>internal/config_loader/loader.go (3)</summary><blockquote> <details> <summary>internal/config_loader/validator.go (2)</summary> * `NewValidator` (34-40) * `ValidateAdapterVersion` (572-584) </details> <details> <summary>internal/config_loader/types.go (1)</summary> * `AdapterConfig` (69-74) </details> <details> <summary>internal/config_loader/constants.go (8)</summary> * `FieldSpec` (9-9) * `FieldResources` (20-20) * `FieldManifest` (78-78) * `FieldRef` (86-86) * `FieldRefs` (87-87) * `FieldPost` (21-21) * `FieldPayloads` (41-41) * `FieldBuildRef` (43-43) </details> </blockquote></details> <details> <summary>internal/executor/executor_test.go (2)</summary><blockquote> <details> <summary>internal/config_loader/types.go (3)</summary> * `ActionBase` (145-149) * `LogAction` (261-264) * `Precondition` (153-158) </details> <details> <summary>internal/executor/types.go (1)</summary> * `StatusSuccess` (35-35) </details> </blockquote></details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>internal/executor/README.md</summary> [uncategorized] ~161-~161: If this is a compound adjective that modifies the following noun, use a hyphen. Context: ...oats truncated | | `float`, `float64` | Floating point value | Strings parsed | | `bool` | Boo... (EN_COMPOUND_ADJECTIVE_INTERNAL) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>internal/config_loader/README.md</summary> 119-119: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (40)</summary><blockquote> <details> <summary>go.mod (2)</summary><blockquote> `55-55`: **Validator v10.30.1 is not listed in official releases—verify this version exists.** The specified validator/v10 v10.30.1 does not appear in the official release history; the latest release is v10.28.0 (Oct 5, 2025). Confirm whether v10.30.1 is a pre-release, development version, or version string error, and consider updating to the latest stable v10.28.0 unless v10.30.1 is intentionally pinned for a specific reason. The golang.org/x package versions (crypto v0.46.0, net v0.47.0, term v0.38.0) appear current and align with or exceed the 2025 security fixes (CVE-2025-22869, CVE-2025-22870, CVE-2025-22872, CVE-2025-47911, CVE-2025-58190). Also applies to: 62-64, 78-78 --- `129-129`: **Versions are stable and mutually compatible.** The golang.org/x library updates are routine maintenance versions with no documented breaking changes. Notably, golang.org/x/net v0.47.0 explicitly requires golang.org/x/sys v0.39.0 in its go.mod, confirming compatibility. These minor version bumps contain security fixes and maintenance updates with no intentional API-breaking changes. </blockquote></details> <details> <summary>configs/adapter-config-template.yaml (1)</summary><blockquote> `76-118`: **LGTM! Clear documentation of parameter type support.** The documentation additions clearly explain the supported parameter types (string, int, float, bool) and provide helpful examples. The commented-out examples for `nodeCount` and `enableFeature` demonstrate practical usage patterns. </blockquote></details> <details> <summary>internal/executor/README.md (1)</summary><blockquote> `155-165`: **LGTM! Clear and comprehensive parameter type documentation.** The new section clearly documents supported parameter types and their conversion behavior. The table format is easy to read and the note about unspecified types is helpful. </blockquote></details> <details> <summary>internal/criteria/README.md (2)</summary><blockquote> `181-203`: **LGTM! Clear documentation of the new GetField method.** The documentation thoroughly explains the new `GetField` method's behavior, return values, and error conditions. The table format for error conditions is particularly helpful. --- `45-45`: All `NewEvaluator` call sites across the codebase have been properly updated to include the `context.Context` parameter as the first argument. Verified 40+ call sites in test files and production code—no outdated signatures remain. </blockquote></details> <details> <summary>internal/executor/post_action_executor_test.go (1)</summary><blockquote> `328-369`: **LGTM! Consistent ActionBase refactoring applied.** The test cases have been correctly updated to use the new `ActionBase` wrapper structure. The nested `ActionBase{Name: ..., Log: ...}` pattern is consistently applied across all test cases and matches the type definition. </blockquote></details> <details> <summary>internal/executor/precondition_executor.go (1)</summary><blockquote> `186-200`: **LGTM! Improved logging for exists operator.** The conditional logging based on operator type improves log clarity. For the `exists` operator, logging the field value (when matched) or "nil or empty" (when failed) is more semantically correct than showing an "expected value", since `exists` checks presence rather than equality. </blockquote></details> <details> <summary>internal/config_loader/loader_test.go (1)</summary><blockquote> `341-341`: **LGTM! Test assertions updated for new validation messages.** The shortened error message expectations using `assert.Contains` are more robust than exact string matches. The substring matching will continue to work even if the full error message format changes slightly. </blockquote></details> <details> <summary>internal/executor/executor_test.go (2)</summary><blockquote> `453-497`: **LGTM! Consistent ActionBase refactoring in test cases.** All test cases have been correctly updated to use the `ActionBase` wrapper structure. The pattern `ActionBase{Name: "..."}` with `Expression` at the `Precondition` level is consistently applied throughout the test file. --- `660-662`: **LGTM! PostAction ActionBase usage is correct.** The `PostAction` test cases correctly nest `Log` under `ActionBase`, matching the type definition and the pattern used throughout the codebase. </blockquote></details> <details> <summary>internal/config_loader/struct_validator.go (4)</summary><blockquote> `24-27`: **LGTM! Well-designed resource name pattern for CEL compatibility.** The regex pattern correctly enforces CEL-compatible resource names by requiring a lowercase letter start and excluding hyphens (which would conflict with CEL's minus operator). The comment clearly explains the rationale. --- `50-58`: **LGTM! Clean custom validation implementations.** Both custom validation functions are simple, correct, and delegate to appropriate validation logic (regex pattern and criteria package). They follow the validator function signature correctly. --- `60-82`: **LGTM! Robust ValidateStruct implementation.** The function correctly handles validation errors with proper type assertions and fallback handling. The use of `formatFullErrorMessage` ensures consistent error formatting across the validation system. --- `84-155`: **LGTM! Comprehensive error formatting with clear messages.** The error formatting functions provide user-friendly, consistent error messages. The `formatFieldPath` function correctly handles embedded structs by removing uppercase type names from the path (e.g., `ActionBase` is removed from the path). The complexity in `formatFieldPath` is justified by the need to handle embedded structs properly for clean error messages. </blockquote></details> <details> <summary>internal/executor/param_extractor_test.go (1)</summary><blockquote> `1-203`: **Well-structured test coverage for type conversion utilities.** The table-driven tests comprehensively cover conversion paths with both success and error cases. Good use of `assert.InDelta` for float comparisons at line 154 to handle floating-point precision. </blockquote></details> <details> <summary>test/integration/executor/executor_k8s_integration_test.go (2)</summary><blockquote> `180-199`: **Precondition and CaptureField structures correctly updated to new API shape.** The test configuration properly uses the new `ActionBase` wrapper for preconditions and the `FieldExpressionDef` embedding for capture fields, aligning with the type definitions in `internal/config_loader/types.go`. --- `283-296`: **PostAction structure correctly updated with ActionBase wrapper.** The PostAction now properly nests the `APICall` within `ActionBase`, consistent with the refactored type structure. </blockquote></details> <details> <summary>internal/config_loader/README.md (1)</summary><blockquote> `1-232`: **Comprehensive documentation for the refactored validation architecture.** The README clearly documents the two-phase validation approach, struct embedding patterns, and validation tag usage. The package structure table and usage examples are helpful. </blockquote></details> <details> <summary>internal/config_loader/loader.go (2)</summary><blockquote> `239-266`: **Good path traversal prevention in resolvePath.** The security check correctly uses `filepath.Rel` and checks for `..` prefix to prevent escaping the base directory. This protects against malicious file references in configurations. --- `128-163`: **Well-structured validation pipeline with clear phase separation.** The pipeline correctly orders validation phases: structural first (fail-fast), then adapter version, file references, and finally semantic validation. The optional `skipSemanticValidation` flag is useful for performance-sensitive contexts. </blockquote></details> <details> <summary>internal/config_loader/validator.go (2)</summary><blockquote> `117-129`: **Environment variable validation at config load time aligns with design intent.** This fail-fast validation for required environment parameters (those with `env.*` source, `Required=true`, and no default) ensures clear error messages before deployment. Based on learnings, this is intentional design. --- `230-251`: **Condition value validation handles operator-specific requirements correctly.** The validation properly enforces that `exists` operator should not have a value set, while `in`/`notIn` require array values. Good separation between structural validation (operator enum) and semantic validation (value requirements). </blockquote></details> <details> <summary>internal/config_loader/validator_test.go (2)</summary><blockquote> `97-109`: **Good addition of tests for `exists` operator value validation.** These new tests verify that the `exists` operator correctly rejects both single values and list values, ensuring the semantic validation catches invalid configurations. --- `557-603`: **Capture field validation tests properly updated for FieldExpressionDef.** Tests correctly use the new `FieldExpressionDef` structure and validate both valid cases (field-only, expression-only) and invalid cases (both set, neither set, missing name). </blockquote></details> <details> <summary>test/integration/executor/executor_integration_test.go (3)</summary><blockquote> `429-447`: **CEL expression evaluation test correctly uses new type structure.** The precondition configuration properly uses `ActionBase` wrapper and `FieldExpressionDef` for capture fields. The CEL expression `clusterPhase == "Ready" && nodeCount >= 3` provides good coverage for CEL-based precondition evaluation. --- `865-927`: **Log action test configuration properly structured.** The test creates a comprehensive configuration with multiple log actions at different levels (info, debug, warning) in both preconditions and post-actions. The `ActionBase` wrapper correctly nests `Log` actions. --- `1-1378`: **Comprehensive integration test coverage for executor functionality.** The test file covers: - Full success flow with preconditions, resources, and post-actions - Precondition not met scenarios - API failures at various phases - CEL expression evaluation - Log actions - Context cancellation - Missing/invalid event handling - Payload build failures Good test isolation and proper cleanup patterns. </blockquote></details> <details> <summary>internal/executor/param_extractor.go (1)</summary><blockquote> `322-329`: The comparison `v != 0` in multi-type cases works correctly. Go's comparison rules successfully handle `interface{}` compared to an untyped `0` for all numeric types (int, int8, int16, int32, int64, uint, uint8, float32, float64). The code does not have a type conversion bug and functions as intended. > Likely an incorrect or invalid review comment. </blockquote></details> <details> <summary>internal/config_loader/types.go (11)</summary><blockquote> `10-18`: **LGTM!** Clean implementation of mutual exclusion using `required_without` and `excluded_with` validation tags. The documentation clearly explains the intent. --- `35-38`: **LGTM!** Good use of composition by embedding `FieldExpressionDef` inline. This ensures consistent validation behavior across all types that need field/expression extraction. --- `68-81`: **LGTM!** Appropriate validation constraints for the top-level config structure. The `eq=AdapterConfig` check ensures proper resource kind validation. --- `83-92`: **LGTM!** Correct use of `validate:"dive"` for slice fields to ensure each element is validated. The `omitempty` on the optional `Post` pointer is appropriate. --- `112-121`: **LGTM!** The validation constraints align with the expected behavior where `Name` and `Source` are required. The `omitempty` YAML tag for serialization doesn't conflict with the `required` validation tag. --- `130-141`: **LGTM!** Consistent mutual exclusion pattern for `Build` and `BuildRef`. The documentation clearly explains the constraint. --- `177-185`: **LGTM!** Clean use of embedded `FieldExpressionDef` ensures consistent validation behavior with other types that need field/expression extraction. --- `249-264`: **LGTM!** Appropriate validation for `PostConfig` using `dive` for slice fields. `PostAction` cleanly reuses `ActionBase` through embedding. --- `275-327`: **LGTM!** Clean validation error handling utilities. The `Extend` method's nil check is good defensive programming. Minor observation: `First()` returns only the message without the path. If callers need the full context, they should access `ve.Errors[0]` directly or consider returning the full `ValidationError`. --- `227-235`: > Likely an incorrect or invalid review comment. --- `187-192`: The custom validator `validoperator` is already properly registered in `internal/config_loader/struct_validator.go` (line 36) as part of the singleton validator initialization using `sync.Once`. The corresponding validator function `validateOperator()` exists and delegates to `criteria.IsValidOperator()`. No action is required. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
xueli181114
force-pushed
the
HYPERFLEET-387
branch
from
c02962c to
e6b1886
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/config_loader/types.go (1)
237-247: DiscoveryConfig validation enforces ByName or BySelectors requirement.The
required_withouttags correctly enforce that at least one ofByNameorBySelectorsmust be provided. As confirmed in past review discussion, this validation works correctly despite theomitemptytag.
🧹 Nitpick comments (4)
internal/config_loader/loader_test.go (1)
341-341: Consider more specific error message assertions.The error message assertion changed from checking the full message to just "must specify". While this is more flexible, it may miss regressions where the error message becomes less helpful. Consider asserting on a more specific substring like "must specify apiCall" or "expression" to ensure the error message remains informative.
Same applies to lines 1187 and 1208.
internal/config_loader/struct_validator.go (2)
62-78: Consider handling RegisterValidation errors.While the custom validations are unlikely to fail registration, explicitly handling or logging errors from
RegisterValidationwould make debugging easier if issues arise during initialization.🔎 Potential improvement
structValidatorOnce.Do(func() { structValidator = validator.New() // Register custom validations - _ = structValidator.RegisterValidation("resourcename", validateResourceName) - _ = structValidator.RegisterValidation("validoperator", validateOperator) + if err := structValidator.RegisterValidation("resourcename", validateResourceName); err != nil { + panic(fmt.Sprintf("failed to register resourcename validator: %v", err)) + } + if err := structValidator.RegisterValidation("validoperator", validateOperator); err != nil { + panic(fmt.Sprintf("failed to register validoperator validator: %v", err)) + } // Use yaml tag names for field names in errors structValidator.RegisterTagNameFunc(extractYamlTagName)
181-202: Verify embedded struct name removal logic.The
formatFieldPathfunction removes embedded struct names (uppercase parts without indices) to clean up error paths. This works for the currentActionBaseembedding pattern, but ensure it handles all current and future embedded structs correctly.Consider adding a unit test that verifies paths like:
AdapterConfig.Spec.Preconditions[0].ActionBase.Name→spec.preconditions[0].nameAdapterConfig.Spec.Resources[0].Discovery.BySelectors.LabelSelector→ appropriate outputinternal/config_loader/validator.go (1)
606-619: String-based error filtering in validateResources is fragile.The error filtering on lines 612-614 uses
strings.Containsto detect resource-related errors, which could miss errors with different wording or falsely match unrelated errors containing "resources".🔎 Consider returning structured errors or using error types
func validateResources(config *AdapterConfig) error { v := NewValidator(config, "") // Run struct validation for resources first if errs := ValidateStruct(config); errs != nil && errs.HasErrors() { - // Filter for resource-related errors - for _, e := range errs.Errors { - if strings.Contains(e.Message, "resources") || strings.Contains(e.Message, "Resources") { - return fmt.Errorf("%s", e.Message) - } - } + // Return first resource-related error by path prefix + for _, e := range errs.Errors { + if strings.HasPrefix(e.Path, "spec.resources") { + return fmt.Errorf("%s: %s", e.Path, e.Message) + } + } } // Then check for duplicate resource names return v.validateDuplicateResourceNames() }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
configs/adapter-config-template.yamlgo.modinternal/config_loader/README.mdinternal/config_loader/loader.gointernal/config_loader/loader_test.gointernal/config_loader/struct_validator.gointernal/config_loader/types.gointernal/config_loader/validator.gointernal/config_loader/validator_schema.gointernal/config_loader/validator_test.gointernal/criteria/README.mdinternal/executor/README.mdinternal/executor/executor_test.gointernal/executor/param_extractor.gointernal/executor/param_extractor_test.gointernal/executor/post_action_executor_test.gointernal/executor/precondition_executor.gotest/integration/config-loader/testdata/adapter_config_valid.yamltest/integration/executor/executor_integration_test.gotest/integration/executor/executor_k8s_integration_test.gotest/testdata/adapter_config_valid.yaml
💤 Files with no reviewable changes (3)
- test/testdata/adapter_config_valid.yaml
- internal/config_loader/validator_schema.go
- test/integration/config-loader/testdata/adapter_config_valid.yaml
✅ Files skipped from review due to trivial changes (1)
- configs/adapter-config-template.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- test/integration/executor/executor_k8s_integration_test.go
- internal/executor/param_extractor_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 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:
internal/executor/param_extractor.gointernal/config_loader/README.mdinternal/config_loader/loader.gointernal/config_loader/struct_validator.gointernal/config_loader/validator_test.gointernal/config_loader/types.gointernal/config_loader/validator.go
🧬 Code graph analysis (8)
internal/executor/param_extractor.go (1)
internal/executor/types.go (2)
NewExecutorError(374-381)PhaseParamExtraction(21-21)
internal/executor/precondition_executor.go (2)
internal/criteria/types.go (2)
Operator(10-10)OperatorExists(28-28)internal/config_loader/constants.go (1)
FieldValue(72-72)
internal/config_loader/loader.go (3)
internal/config_loader/validator.go (2)
NewValidator(34-40)ValidateAdapterVersion(572-584)internal/config_loader/types.go (1)
AdapterConfig(69-74)internal/config_loader/constants.go (8)
FieldSpec(9-9)FieldResources(20-20)FieldManifest(78-78)FieldRef(86-86)FieldRefs(87-87)FieldPost(21-21)FieldPayloads(41-41)FieldBuildRef(43-43)
internal/executor/post_action_executor_test.go (1)
internal/config_loader/types.go (2)
ActionBase(145-149)LogAction(261-264)
internal/config_loader/struct_validator.go (2)
internal/config_loader/types.go (2)
AdapterConfig(69-74)ValidationErrors(286-288)internal/criteria/types.go (2)
IsValidOperator(44-51)OperatorStrings(54-60)
internal/executor/executor_test.go (2)
internal/config_loader/types.go (3)
ActionBase(145-149)LogAction(261-264)Precondition(153-158)internal/executor/types.go (1)
StatusSuccess(35-35)
test/integration/executor/executor_integration_test.go (1)
internal/config_loader/types.go (5)
ActionBase(145-149)APICall(161-169)CaptureField(182-185)FieldExpressionDef(13-18)LogAction(261-264)
internal/config_loader/types.go (2)
internal/criteria/types.go (1)
Operator(10-10)internal/k8s_client/discovery.go (2)
Discovery(14-29)DiscoveryConfig(32-42)
🪛 LanguageTool
internal/executor/README.md
[uncategorized] ~161-~161: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...oats truncated | | float, float64 | Floating point value | Strings parsed | | bool | Boo...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.18.1)
internal/config_loader/README.md
119-119: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (28)
internal/executor/precondition_executor.go (1)
188-199: LGTM! Improved logging for exists operator.The logging changes appropriately handle the
existsoperator which doesn't have an expected value. For matched conditions, it now logs only the field value, and for failed conditions, it provides a clearer "field is nil or empty" message instead of confusing comparisons.internal/executor/post_action_executor_test.go (1)
330-364: LGTM! Tests updated for ActionBase embedding.The test data consistently reflects the new structure where
PostActionembedsActionBase. The changes are mechanical and maintain test coverage.internal/executor/README.md (1)
144-165: LGTM! Clear parameter type documentation.The new "Supported Parameter Types" section provides clear guidance on type conversion behavior. The table is well-structured and the note about retaining original types is helpful.
Minor: Consider "floating-point value" instead of "Floating point value" for consistency (though this is a nitpick).
internal/executor/param_extractor.go (2)
32-56: LGTM! Clean defaulting and type conversion flow.The defaulting logic correctly handles nil and empty string cases, and the type conversion is appropriately gated on non-nil values with proper error handling for required vs. non-required parameters.
Note: The duplicate defaulting logic mentioned in past reviews appears to have been resolved in this refactor.
165-330: Type conversion utilities look solid.The conversion functions handle a comprehensive set of type cases with proper error messages. Key strengths:
- uint64 overflow check (lines 209-211) prevents silent truncation
- Bool conversion handles common truthy/falsy strings beyond strconv.ParseBool
- Consistent error messages reference the input value and type
internal/executor/executor_test.go (1)
453-497: LGTM! Tests consistently updated for ActionBase embedding.All precondition test literals now use the
ActionBasewrapper, maintaining test coverage while reflecting the new type structure. The changes are mechanical and consistent.internal/config_loader/struct_validator.go (1)
26-29: Good: Resource name pattern enforces CEL compatibility.The regex
^[a-z][a-zA-Z0-9_]*$correctly prevents hyphens which would conflict with CEL's minus operator. The pattern is well-commented.internal/config_loader/README.md (2)
78-124: Excellent validation documentation!The two-phase validation explanation with detailed examples and error message formats is very helpful for users. The custom validators table and validation tag examples are clear and comprehensive.
Minor: Line 119 has a fenced code block without a language specifier (static analysis hint). Consider adding the language for proper rendering.
141-173: Clear documentation of new patterns.The struct embedding section and
ValidationErrorsusage examples provide excellent guidance for understanding the refactored architecture. The code examples are practical and easy to follow.internal/config_loader/loader.go (3)
129-163: Well-structured validation pipeline with clear phase separation.The refactored validation pipeline cleanly separates concerns: structural validation (fail-fast), adapter version validation, file reference validation, and semantic validation. This modular approach improves maintainability and allows selective validation.
170-200: File reference loading handles single vs multiple refs correctly.The logic correctly differentiates between single
ref(backward compatible, populatesresource.Manifest) and multiplerefs(populatesresource.ManifestItems). The error messages include appropriate path context for debugging.
239-266: Path containment check doesn't account for symlinks.The
resolvePathfunction validates that the logical path stays within the base directory usingfilepath.Rel, but this check operates on path strings without resolving symlinks. A symlink within the base directory could resolve to a location outside it, allowingos.Statto access files beyond the intended boundary.If symlink-following is a security concern in your deployment, consider using
filepath.EvalSymlinksto resolve the target path before the containment check:// Resolve symlinks after cleaning targetPath, err := filepath.EvalSymlinks(targetPath) if err != nil { return "", fmt.Errorf("path %q cannot be resolved: %w", refPath, err) } // Then check if the resolved path is within base directory rel, err := filepath.Rel(baseClean, targetPath)However, this approach will fail on broken symlinks. If your use case requires handling broken symlinks gracefully, consider using
os.Lstatinstead ofos.Statin the calling code to avoid following symlinks entirely.internal/config_loader/validator_test.go (3)
97-109: Good addition of edge case tests for theexistsoperator.These tests verify that the
existsoperator correctly rejects configurations where a value is provided, ensuring proper validation of operator semantics.
477-490: Updated test assertions for ValidateStruct return type.The test correctly handles the new
*ValidationErrorsreturn type fromValidateStruct, checking for nil before accessingHasErrors().
606-648: New tests verify yaml field name mapping and cache population.These tests ensure the field name cache is correctly populated during validator initialization, which is essential for accurate error path reporting in validation messages.
internal/criteria/README.md (2)
45-57: Documentation updated for two-value NewEvaluator return.The example correctly shows the updated
NewEvaluatorsignature returning(evaluator, error). Note that the example ignores the error with_- in production code, this should be handled.
181-203: Clear documentation of GetField error semantics.The table clearly distinguishes between parse errors (returned as second value), field-not-found (nil Value, no error), and runtime extraction failures (result.Error). This distinction is important for callers to handle appropriately.
test/integration/executor/executor_integration_test.go (3)
429-447: Integration test correctly uses ActionBase wrapper for preconditions.The test properly constructs
Preconditionwith the newActionBasewrapper containingNameandAPICall, andCaptureFieldentries usingFieldExpressionDeffor field extraction.
865-927: LogAction integration tests demonstrate new ActionBase structure.The tests correctly construct log-only preconditions and post-actions using
ActionBase.Log, validating that log actions work independently of API calls.
1110-1127: Precondition with CEL expression and captures correctly structured.The test properly combines
ActionBase(withAPICall),Capture(withFieldExpressionDef), andConditionsin a single precondition, demonstrating the flexible validation model.internal/config_loader/validator.go (4)
42-65: Structural validation with fail-fast behavior is well-designed.The two-phase approach (struct tag validation followed by custom validators) provides clear separation. Returning on first error is appropriate for structural issues that indicate fundamental configuration problems.
116-129: Environment variable validation at load time is intentional design.Based on learnings, this fail-fast validation ensures environment variables are set before deployment, providing clear error messages early. This is the correct approach for required
env.*parameters.
230-251: Improved exists operator validation rejects values correctly.The updated logic correctly ensures that the
existsoperator doesn't accept values, matching the semantic that it only checks for field presence. This addresses the edge case where users might incorrectly provide a value.
410-444: CEL environment initialization handles dynamic variables well.The logic correctly deduplicates root variables and adds fallback declarations for
resourcesandadapterif not already defined. The capacity estimation at line 411 is a minor optimization detail.internal/config_loader/types.go (4)
10-18: FieldExpressionDef provides clean mutual exclusivity pattern.The inline embedding pattern with
required_withoutandexcluded_withtags enforces that exactly one ofFieldorExpressionmust be set. This is reused effectively inValueDefandCaptureField.
270-327: ValidationErrors provides comprehensive error aggregation utilities.The
ValidationErrorstype withAdd,Extend,First,Count, andHasErrorsmethods provides a clean API for collecting and reporting multiple validation errors. The formattedError()output is user-friendly.
255-258: PostAction simplified to embed ActionBase.The refactored
PostActionnow only embedsActionBase, eliminating duplication withPreconditionwhile maintaining the same capabilities (Name, APICall, Log).
143-158: Nested field references in validation tags are correctly supported.The go-playground/validator library supports nested field references through embedded structs using dotted notation (e.g.,
ActionBase.APICall). Therequired_without_allvalidator correctly resolves these references. Integration tests in the codebase (executor_k8s_integration_test.go, config_criteria_integration_test.go) validate Precondition structs with these validation rules, and they pass successfully.Likely an incorrect or invalid review comment.
| return strings.Join(cleanParts, ".") | ||
| } | ||
|
|
||
|
|
| `, | ||
| wantError: true, | ||
| errorMsg: "must specify apiCall, expression, or conditions", | ||
| errorMsg: "must specify", |
| }, | ||
| }, | ||
| wantErr: true, | ||
| errMsg: "must have either byName or bySelectors", |
There was a problem hiding this comment.
It seems that removing the details would reduce the test quality.
| Log *LogAction `yaml:"log,omitempty"` | ||
| ActionBase `yaml:",inline"` | ||
| Capture []CaptureField `yaml:"capture,omitempty" validate:"dive"` | ||
| Conditions []Condition `yaml:"conditions,omitempty" validate:"dive,required_without_all=ActionBase.APICall Expression"` |
There was a problem hiding this comment.
Have you tested if this validation works correctly with the embedded field syntax?
The go-playground/validator may not support the ActionBase.APICall syntax for referencing embedded struct fields. The validator uses Go field names directly, not nested paths.
2 participants
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.