feat: Implemented transportation interface for k8s client and maestro client #42
Conversation
WalkthroughThis PR introduces a transport client abstraction layer to decouple resource application from Kubernetes-specific implementations, enabling support for multiple backends (Kubernetes, Maestro). The refactoring replaces direct Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip We've launched Issue Planner and it is currently in beta. Please try it out and share your feedback on Discord! Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/executor/param_extractor.go (1)
29-58:⚠️ Potential issue | 🟡 MinorConvert defaults to the declared type when falling back.
Defaults are assigned as-is in error/default paths, so a string default for anintparam can remain a string. Normalize defaults viaconvertParamTypewhenparam.Typeis set (apply similarly in the other default branches).💡 Suggested normalization for defaults
- if param.Default != nil { - execCtx.Params[param.Name] = param.Default - } + if param.Default != nil { + v := param.Default + if param.Type != "" { + if converted, convErr := convertParamType(v, param.Type); convErr == nil { + v = converted + } + } + execCtx.Params[param.Name] = v + }internal/executor/resource_executor.go (1)
87-184:⚠️ Potential issue | 🟠 MajorGuard against nil ApplyResourcesResult to avoid panic.
TransportClient implementations can return(nil, err); callingapplyResult.Failed()without a nil check can panic.🛠️ Suggested fix
applyResult, err := re.client.ApplyResources(ctx, []transport_client.ResourceToApply{ { Name: resource.Name, Manifest: resourceManifest, Existing: existingResource, Options: applyOpts, }, }) - if err != nil || applyResult.Failed() { + if err != nil || applyResult == nil || applyResult.Failed() { result.Status = StatusFailed - if err == nil && len(applyResult.Results) > 0 { - err = applyResult.Results[0].Error - } + if err == nil { + switch { + case applyResult == nil: + err = fmt.Errorf("transport client returned nil ApplyResourcesResult") + case len(applyResult.Results) > 0: + err = applyResult.Results[0].Error + default: + err = fmt.Errorf("apply resources failed without result details") + } + } result.Error = err // Set ExecutionError for K8s operation failure execCtx.Adapter.ExecutionError = &ExecutionError{ Phase: string(PhaseResources), Step: resource.Name, Message: err.Error(),
🤖 Fix all issues with AI agents
In `@internal/k8s_client/apply.go`:
- Around line 72-83: In ApplyResource, guard against missing/invalid generation
annotations by checking the results of manifest.GetGenerationFromUnstructured
for both newManifest and existing (when existing != nil); if newGen == 0 (or
existingGen == 0 when existing != nil) return a clear error instead of
proceeding to manifest.CompareGenerations so the call site fails fast rather
than silently skipping/incorrectly updating resources; reference the variables
newGen, existingGen and the functions GetGenerationFromUnstructured and
CompareGenerations when implementing this validation and error return in
ApplyResource.
In `@internal/k8s_client/mock.go`:
- Around line 101-148: The mock currently assumes non-nil manifests and
stores/returns the same object reference; update MockK8sClient.ApplyResource and
ApplyResources to (1) guard against nil newManifest (fall back to existing or
return a clear error) when handling ResourceToApply.Manifest, and (2) store and
return deep copies of manifests when writing to m.Resources and when returning
ApplyResult.Resource and ResourceApplyResult.ApplyResult.Resource to avoid
shared references in tests (use unstructured.Unstructured.DeepCopy or
equivalent). Ensure ApplyResources uses ApplyResource for each ResourceToApply
but passes nil-checks through and records failures/successes as before.
In `@internal/maestro_client/client.go`:
- Around line 585-594: determineOperation currently inspects the returned
*workv1.ManifestWork.ResourceVersion which is always set after
ApplyManifestWork, causing it to mis-classify creates as updates; instead,
propagate the operation decision made by CompareGenerations from
ApplyManifestWork (or capture the decision immediately before calling the
create/update path) and return that manifest.Operation; modify ApplyManifestWork
to either return the manifest.Operation it decided or store that decision in a
field you pass into determineOperation so determineOperation uses the original
operation choice (from CompareGenerations) rather than inspecting
ResourceVersion.
In `@internal/maestro_client/operations_test.go`:
- Around line 3-4: Update the stale package path in the top-of-file comment that
currently references internal/generation/generation_test.go to point to the new
location internal/manifest/generation_test.go so the note about where
manifest.ValidateGeneration, manifest.ValidateGenerationFromUnstructured, and
manifest.ValidateManifestWorkGeneration tests live remains accurate; modify the
comment text accordingly (the comment containing "Tests for
manifest.ValidateGeneration..." in operations_test.go).
In `@internal/manifest/generation.go`:
- Around line 241-249: The doc comment refers to a missing ValidateManifest
function; fix by either removing the orphan comment or implementing
ValidateManifest to match the described behavior: accept the manifest object
used elsewhere, return an error when the object is nil, when apiVersion or kind
or metadata.name are empty, or when the generation annotation is missing or
invalid; ensure the function name ValidateManifest and its error messages match
the doc comment so callers (e.g., k8s_client/maestro_client usages) compile and
tests can validate the same validation rules.
In `@pkg/utils/map.go`:
- Around line 130-150: splitPath currently drops empty segments (e.g., "a..b" or
"a.") which masks invalid paths; change splitPath to preserve empty segments
(e.g., use strings.Split or equivalent logic that appends even when start==i and
includes a final empty segment for trailing dot) and update GetNestedValue to
validate the returned slice and return an error if any segment == "" so
malformed paths are rejected instead of silently skipped. Ensure you modify
splitPath and add the empty-segment check in GetNestedValue to fail fast with a
clear error.
🧹 Nitpick comments (10)
pkg/utils/template.go (1)
86-114:floatandfloat64functions are identical.Both functions have the same signature (
func(v interface{}) float64) and identical implementation. Consider removing one or aliasing to reduce duplication.♻️ Suggested refactor to eliminate duplication
- "float": func(v interface{}) float64 { - switch val := v.(type) { - case int: - return float64(val) - case int64: - return float64(val) - case float64: - return val - case string: - f, _ := strconv.ParseFloat(val, 64) //nolint:errcheck // returns 0 on error, which is acceptable - return f - default: - return 0 - } - }, - "float64": func(v interface{}) float64 { - switch val := v.(type) { - case int: - return float64(val) - case int64: - return float64(val) - case float64: - return val - case string: - f, _ := strconv.ParseFloat(val, 64) //nolint:errcheck // returns 0 on error, which is acceptable - return f - default: - return 0 - } - }, + "float": toFloat64, + "float64": toFloat64,Then define a helper function outside the map:
func toFloat64(v interface{}) float64 { switch val := v.(type) { case int: return float64(val) case int64: return float64(val) case float64: return val case string: f, _ := strconv.ParseFloat(val, 64) //nolint:errcheck // returns 0 on error, which is acceptable return f default: return 0 } }internal/manifest/render.go (1)
48-57: Consider including slice index in error messages for easier debugging.When rendering fails for an element within a slice, the error doesn't indicate which index caused the failure, making it harder to debug nested structures.
💡 Proposed improvement
case []interface{}: result := make([]interface{}, len(val)) for i, item := range val { rendered, err := renderValue(item, params) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to render slice element [%d]: %w", i, err) } result[i] = rendered } return result, nilpkg/utils/convert.go (1)
92-104: Consider adding overflow/NaN checks for float-to-int conversions.Converting large float values (or NaN/Inf) to int64 can produce undefined behavior. While this may be acceptable for the current use case, it's worth noting.
💡 Optional: Add validation for extreme float values
case float32: + if math.IsNaN(float64(v)) || math.IsInf(float64(v), 0) { + return 0, fmt.Errorf("cannot convert %v to int", v) + } + if float64(v) > float64(math.MaxInt64) || float64(v) < float64(math.MinInt64) { + return 0, fmt.Errorf("float32 value %v overflows int64", v) + } return int64(v), nil case float64: + if math.IsNaN(v) || math.IsInf(v, 0) { + return 0, fmt.Errorf("cannot convert %v to int", v) + } + if v > float64(math.MaxInt64) || v < float64(math.MinInt64) { + return 0, fmt.Errorf("float64 value %v overflows int64", v) + } return int64(v), nilinternal/manifest/generation.go (2)
372-386: Silently ignoring malformed label selector pairs may mask configuration errors.When a selector pair cannot be split into exactly 2 parts (e.g.,
"app"without=value), the code silently skips it withcontinue. This could cause unexpected matching behavior if callers pass malformed selectors.Consider logging a warning or returning an error for malformed selectors:
Suggested improvement
pairs := strings.Split(labelSelector, ",") for _, pair := range pairs { kv := strings.SplitN(pair, "=", 2) if len(kv) != 2 { - continue + // Malformed selector pair - treat as non-match for safety + return false }
405-410: Inconsistent error wrapping pattern.Other validation errors in this file use
.AsError()(e.g., lines 145, 150, 154), but here the error is returned directly without calling.AsError(). This may cause inconsistent error handling behavior.Suggested fix for consistency
if err := obj.UnmarshalJSON(m.Raw); err != nil { - return nil, apperrors.Validation("ManifestWork %q manifest[%d]: failed to unmarshal: %v", + return nil, apperrors.Validation("ManifestWork %q manifest[%d]: failed to unmarshal: %v", - work.Name, i, err) + work.Name, i, err).AsError() }internal/maestro_client/operations.go (1)
318-333: Consider ifDiscoverManifestInWorkadds value as a method.This method is a one-line delegation to
manifest.DiscoverInManifestWork. Since it doesn't use anyClientstate and callers could invoke the manifest package function directly, consider whether this method provides sufficient value or if it should be removed to reduce API surface area.internal/maestro_client/client.go (3)
459-473: Returning both partial results and an error may cause inconsistent caller behavior.On failure, this function returns
resultwith populatedResults(all marked as failed) and a non-nil error. Some callers may check only the error and miss the partial results, while others may process results without checking the error. Consider either:
- Returning
nilresult on error (simpler contract)- Clearly documenting that callers should always check the error first
506-512:GetResourcesilently skips manifests that fail to unmarshal.When
manifestToUnstructured(m)fails (line 516-518), the error is silently swallowed withcontinue. This could mask data corruption or configuration issues. Consider logging a warning when unmarshalling fails.Suggested improvement
for _, m := range work.Spec.Workload.Manifests { obj, err := manifestToUnstructured(m) if err != nil { + c.log.Warnf(ctx, "Failed to unmarshal manifest in %s/%s: %v", + c.config.ConsumerName, c.config.WorkName, err) continue }
508-509:GroupResourceusesKindinstead of resource name.When constructing the
NotFounderror,gvk.Kindis used as theResourcefield. Kubernetes conventions typically use the plural lowercase resource name (e.g., "namespaces" not "Namespace"). While this may work for error messages, it could cause confusion or issues with tooling that expects standard resource names.Note
If precise resource naming is important, consider using a discovery client to map GVK to the proper resource name, or pass the resource name explicitly. For simple error messages, the current approach may be acceptable.
pkg/utils/path.go (1)
27-52: Implementation correctly prevents directory traversal attacks; symlink handling is acceptable for the deployment context.The secure path resolution is well-implemented using
filepath.Reland checking for".."prefix to prevent directory traversal. The use offilepath.Absandfilepath.Cleanensures proper path normalization.Regarding symlinks: the function does not resolve symlinks, so a path like
baseDir/symlink_to_outside/filewould be followed. However, this is not a security gap in the current usage context, since config file paths come from admin-controlled sources (CLI arguments and environment variables), not untrusted user input. If the threat model changes to allow user-controlled symlinks in config directories, consider addingfilepath.EvalSymlinksresolution.
| // Get generation from new manifest | ||
| newGen := manifest.GetGenerationFromUnstructured(newManifest) | ||
|
|
||
| // Get existing generation (0 if not found) | ||
| var existingGen int64 | ||
| if existing != nil { | ||
| existingGen = manifest.GetGenerationFromUnstructured(existing) | ||
| } | ||
|
|
||
| // Compare generations to determine operation | ||
| decision := manifest.CompareGenerations(newGen, existingGen, existing != nil) | ||
|
|
There was a problem hiding this comment.
Reject manifests missing generation annotation in ApplyResource.
GetGenerationFromUnstructured returns 0 for missing/invalid annotation, which can cause incorrect skip/update decisions. Fail fast to avoid silent no-ops.
🛡️ Suggested guard
import (
"context"
"fmt"
"time"
"github.com/openshift-hyperfleet/hyperfleet-adapter/internal/manifest"
+ "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/constants"
"github.com/openshift-hyperfleet/hyperfleet-adapter/internal/transport_client"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
)
@@
// Get generation from new manifest
newGen := manifest.GetGenerationFromUnstructured(newManifest)
+ if newGen == 0 {
+ return nil, fmt.Errorf("manifest missing required annotation %q", constants.AnnotationGeneration)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get generation from new manifest | |
| newGen := manifest.GetGenerationFromUnstructured(newManifest) | |
| // Get existing generation (0 if not found) | |
| var existingGen int64 | |
| if existing != nil { | |
| existingGen = manifest.GetGenerationFromUnstructured(existing) | |
| } | |
| // Compare generations to determine operation | |
| decision := manifest.CompareGenerations(newGen, existingGen, existing != nil) | |
| // Get generation from new manifest | |
| newGen := manifest.GetGenerationFromUnstructured(newManifest) | |
| if newGen == 0 { | |
| return nil, fmt.Errorf("manifest missing required annotation %q", constants.AnnotationGeneration) | |
| } | |
| // Get existing generation (0 if not found) | |
| var existingGen int64 | |
| if existing != nil { | |
| existingGen = manifest.GetGenerationFromUnstructured(existing) | |
| } | |
| // Compare generations to determine operation | |
| decision := manifest.CompareGenerations(newGen, existingGen, existing != nil) |
🤖 Prompt for AI Agents
In `@internal/k8s_client/apply.go` around lines 72 - 83, In ApplyResource, guard
against missing/invalid generation annotations by checking the results of
manifest.GetGenerationFromUnstructured for both newManifest and existing (when
existing != nil); if newGen == 0 (or existingGen == 0 when existing != nil)
return a clear error instead of proceeding to manifest.CompareGenerations so the
call site fails fast rather than silently skipping/incorrectly updating
resources; reference the variables newGen, existingGen and the functions
GetGenerationFromUnstructured and CompareGenerations when implementing this
validation and error return in ApplyResource.
| // ApplyResource implements K8sClient.ApplyResource | ||
| func (m *MockK8sClient) ApplyResource(ctx context.Context, newManifest *unstructured.Unstructured, existing *unstructured.Unstructured, opts *ApplyOptions) (*ApplyResult, error) { | ||
| if m.ApplyResourceError != nil { | ||
| return nil, m.ApplyResourceError | ||
| } | ||
| if m.DiscoverResult != nil { | ||
| return m.DiscoverResult, nil | ||
| if m.ApplyResourceResult != nil { | ||
| return m.ApplyResourceResult, nil | ||
| } | ||
| return &unstructured.UnstructuredList{}, nil | ||
| // Default behavior: store the resource and return create result | ||
| key := newManifest.GetNamespace() + "/" + newManifest.GetName() | ||
| m.Resources[key] = newManifest | ||
| return &ApplyResult{ | ||
| Resource: newManifest, | ||
| Operation: manifest.OperationCreate, | ||
| Reason: "mock apply", | ||
| }, nil | ||
| } | ||
|
|
||
| // ExtractFromSecret implements K8sClient.ExtractFromSecret | ||
| func (m *MockK8sClient) ExtractFromSecret(ctx context.Context, path string) (string, error) { | ||
| if m.ExtractSecretError != nil { | ||
| return "", m.ExtractSecretError | ||
| // ApplyResources implements K8sClient.ApplyResources | ||
| func (m *MockK8sClient) ApplyResources(ctx context.Context, resources []ResourceToApply) (*ApplyResourcesResult, error) { | ||
| if m.ApplyResourcesError != nil { | ||
| return nil, m.ApplyResourcesError | ||
| } | ||
| if m.ApplyResourcesResult != nil { | ||
| return m.ApplyResourcesResult, nil | ||
| } | ||
| // Default behavior: apply each resource using ApplyResource | ||
| result := &ApplyResourcesResult{ | ||
| Results: make([]*ResourceApplyResult, 0, len(resources)), | ||
| } | ||
| return m.ExtractSecretResult, nil | ||
| for _, r := range resources { | ||
| applyResult, err := m.ApplyResource(ctx, r.Manifest, r.Existing, r.Options) | ||
| resourceResult := &ResourceApplyResult{ | ||
| Name: r.Name, | ||
| Kind: r.Manifest.GetKind(), | ||
| Namespace: r.Manifest.GetNamespace(), | ||
| ResourceName: r.Manifest.GetName(), | ||
| ApplyResult: applyResult, | ||
| Error: err, | ||
| } | ||
| result.Results = append(result.Results, resourceResult) | ||
| if err != nil { | ||
| result.FailedCount++ | ||
| return result, err | ||
| } | ||
| result.SuccessCount++ | ||
| } | ||
| return result, nil |
There was a problem hiding this comment.
Handle nil manifests and deep copy stored resources in the mock.
Aligns behavior with the real client and avoids shared references in tests.
🧪 Suggested fixes
import (
"context"
+ "fmt"
"github.com/openshift-hyperfleet/hyperfleet-adapter/internal/manifest"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
)
@@
func (m *MockK8sClient) ApplyResource(ctx context.Context, newManifest *unstructured.Unstructured, existing *unstructured.Unstructured, opts *ApplyOptions) (*ApplyResult, error) {
+ if newManifest == nil {
+ return nil, fmt.Errorf("new manifest cannot be nil")
+ }
if m.ApplyResourceError != nil {
return nil, m.ApplyResourceError
}
@@
// Default behavior: store the resource and return create result
key := newManifest.GetNamespace() + "/" + newManifest.GetName()
- m.Resources[key] = newManifest
+ m.Resources[key] = newManifest.DeepCopy()
return &ApplyResult{
Resource: newManifest,
Operation: manifest.OperationCreate,
Reason: "mock apply",
}, nil
}
@@
for _, r := range resources {
+ if r.Manifest == nil {
+ resourceResult := &ResourceApplyResult{
+ Name: r.Name,
+ Error: fmt.Errorf("manifest cannot be nil for resource %s", r.Name),
+ }
+ result.Results = append(result.Results, resourceResult)
+ result.FailedCount++
+ return result, resourceResult.Error
+ }
applyResult, err := m.ApplyResource(ctx, r.Manifest, r.Existing, r.Options)🤖 Prompt for AI Agents
In `@internal/k8s_client/mock.go` around lines 101 - 148, The mock currently
assumes non-nil manifests and stores/returns the same object reference; update
MockK8sClient.ApplyResource and ApplyResources to (1) guard against nil
newManifest (fall back to existing or return a clear error) when handling
ResourceToApply.Manifest, and (2) store and return deep copies of manifests when
writing to m.Resources and when returning ApplyResult.Resource and
ResourceApplyResult.ApplyResult.Resource to avoid shared references in tests
(use unstructured.Unstructured.DeepCopy or equivalent). Ensure ApplyResources
uses ApplyResource for each ResourceToApply but passes nil-checks through and
records failures/successes as before.
| // determineOperation determines the operation that was performed based on the ManifestWork. | ||
| func (c *Client) determineOperation(work *workv1.ManifestWork) manifest.Operation { | ||
| if work == nil { | ||
| return manifest.OperationCreate | ||
| } | ||
| if work.ResourceVersion == "" { | ||
| return manifest.OperationCreate | ||
| } | ||
| return manifest.OperationUpdate | ||
| } |
There was a problem hiding this comment.
determineOperation logic may not correctly identify the performed operation.
After ApplyManifestWork succeeds, the returned *workv1.ManifestWork will always have a ResourceVersion populated (Kubernetes sets it on both create and update). This means determineOperation will always return OperationUpdate for successful applies, even when the resource was created.
To correctly determine the operation, you should use the decision from CompareGenerations in ApplyManifestWork rather than inferring from the result.
Suggested approach
Pass the operation from ApplyManifestWork or track it separately:
-// determineOperation determines the operation that was performed based on the ManifestWork.
-func (c *Client) determineOperation(work *workv1.ManifestWork) manifest.Operation {
- if work == nil {
- return manifest.OperationCreate
- }
- if work.ResourceVersion == "" {
- return manifest.OperationCreate
- }
- return manifest.OperationUpdate
-}Instead, modify ApplyManifestWork to return the operation it performed, or store the operation decision before calling the underlying create/update methods.
🤖 Prompt for AI Agents
In `@internal/maestro_client/client.go` around lines 585 - 594, determineOperation
currently inspects the returned *workv1.ManifestWork.ResourceVersion which is
always set after ApplyManifestWork, causing it to mis-classify creates as
updates; instead, propagate the operation decision made by CompareGenerations
from ApplyManifestWork (or capture the decision immediately before calling the
create/update path) and return that manifest.Operation; modify ApplyManifestWork
to either return the manifest.Operation it decided or store that decision in a
field you pass into determineOperation so determineOperation uses the original
operation choice (from CompareGenerations) rather than inspecting
ResourceVersion.
| // Note: Tests for manifest.ValidateGeneration, manifest.ValidateGenerationFromUnstructured, | ||
| // and manifest.ValidateManifestWorkGeneration are in internal/generation/generation_test.go. |
There was a problem hiding this comment.
Stale documentation comment references incorrect package path.
The comment references internal/generation/generation_test.go, but based on the PR changes, the tests are now in internal/manifest/generation_test.go.
Suggested fix
-// Note: Tests for manifest.ValidateGeneration, manifest.ValidateGenerationFromUnstructured,
-// and manifest.ValidateManifestWorkGeneration are in internal/generation/generation_test.go.
+// Note: Tests for manifest.ValidateGeneration, manifest.ValidateGenerationFromUnstructured,
+// and manifest.ValidateManifestWorkGeneration are in internal/manifest/generation_test.go.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Note: Tests for manifest.ValidateGeneration, manifest.ValidateGenerationFromUnstructured, | |
| // and manifest.ValidateManifestWorkGeneration are in internal/generation/generation_test.go. | |
| // Note: Tests for manifest.ValidateGeneration, manifest.ValidateGenerationFromUnstructured, | |
| // and manifest.ValidateManifestWorkGeneration are in internal/manifest/generation_test.go. |
🤖 Prompt for AI Agents
In `@internal/maestro_client/operations_test.go` around lines 3 - 4, Update the
stale package path in the top-of-file comment that currently references
internal/generation/generation_test.go to point to the new location
internal/manifest/generation_test.go so the note about where
manifest.ValidateGeneration, manifest.ValidateGenerationFromUnstructured, and
manifest.ValidateManifestWorkGeneration tests live remains accurate; modify the
comment text accordingly (the comment containing "Tests for
manifest.ValidateGeneration..." in operations_test.go).
| // ValidateManifest validates a Kubernetes manifest has all required fields and annotations. | ||
| // Returns error if: | ||
| // - Object is nil | ||
| // - apiVersion is missing | ||
| // - kind is missing | ||
| // - metadata.name is missing | ||
| // - generation annotation is missing or invalid | ||
| // | ||
| // This is used by both k8s_client (for direct K8s resources) and maestro_client (for ManifestWork payloads). |
There was a problem hiding this comment.
Orphan documentation comment references non-existent ValidateManifest function.
Lines 241-249 contain a doc comment for ValidateManifest that describes validation behavior, but the function itself is not defined in this file. Either the function implementation is missing, or this orphan comment should be removed.
Suggested fix
Either add the ValidateManifest function implementation, or remove the orphan documentation:
-// ValidateManifest validates a Kubernetes manifest has all required fields and annotations.
-// Returns error if:
-// - Object is nil
-// - apiVersion is missing
-// - kind is missing
-// - metadata.name is missing
-// - generation annotation is missing or invalid
-//
-// This is used by both k8s_client (for direct K8s resources) and maestro_client (for ManifestWork payloads).
-
// GetLatestGenerationFromList returns the resource with the highest generation annotation from a list.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ValidateManifest validates a Kubernetes manifest has all required fields and annotations. | |
| // Returns error if: | |
| // - Object is nil | |
| // - apiVersion is missing | |
| // - kind is missing | |
| // - metadata.name is missing | |
| // - generation annotation is missing or invalid | |
| // | |
| // This is used by both k8s_client (for direct K8s resources) and maestro_client (for ManifestWork payloads). |
🤖 Prompt for AI Agents
In `@internal/manifest/generation.go` around lines 241 - 249, The doc comment
refers to a missing ValidateManifest function; fix by either removing the orphan
comment or implementing ValidateManifest to match the described behavior: accept
the manifest object used elsewhere, return an error when the object is nil, when
apiVersion or kind or metadata.name are empty, or when the generation annotation
is missing or invalid; ensure the function name ValidateManifest and its error
messages match the doc comment so callers (e.g., k8s_client/maestro_client
usages) compile and tests can validate the same validation rules.
| // splitPath splits a dot-separated path into parts. | ||
| func splitPath(path string) []string { | ||
| if path == "" { | ||
| return nil | ||
| } | ||
| // Simple split - doesn't handle escaped dots | ||
| result := make([]string, 0) | ||
| start := 0 | ||
| for i := 0; i < len(path); i++ { | ||
| if path[i] == '.' { | ||
| if i > start { | ||
| result = append(result, path[start:i]) | ||
| } | ||
| start = i + 1 | ||
| } | ||
| } | ||
| if start < len(path) { | ||
| result = append(result, path[start:]) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
Reject empty path segments instead of silently skipping them.
Current splitting drops empty segments, so paths like "a..b" or "a." resolve as valid and may return the wrong value instead of an error. Consider preserving empty segments and erroring in GetNestedValue.
🛠️ Proposed fix
func GetNestedValue(data map[string]interface{}, path string) (interface{}, error) {
parts := splitPath(path)
var current interface{} = data
for i, part := range parts {
+ if part == "" {
+ return nil, fmt.Errorf("invalid empty path segment at '%s'", joinPath(parts[:i+1]))
+ }
switch v := current.(type) {
case map[string]interface{}:
val, ok := v[part]
if !ok {
return nil, fmt.Errorf("field '%s' not found at path '%s'", part, joinPath(parts[:i+1]))
@@
func splitPath(path string) []string {
- if path == "" {
- return nil
- }
// Simple split - doesn't handle escaped dots
result := make([]string, 0)
start := 0
for i := 0; i < len(path); i++ {
if path[i] == '.' {
- if i > start {
- result = append(result, path[start:i])
- }
+ result = append(result, path[start:i])
start = i + 1
}
}
- if start < len(path) {
- result = append(result, path[start:])
- }
+ result = append(result, path[start:])
return result
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // splitPath splits a dot-separated path into parts. | |
| func splitPath(path string) []string { | |
| if path == "" { | |
| return nil | |
| } | |
| // Simple split - doesn't handle escaped dots | |
| result := make([]string, 0) | |
| start := 0 | |
| for i := 0; i < len(path); i++ { | |
| if path[i] == '.' { | |
| if i > start { | |
| result = append(result, path[start:i]) | |
| } | |
| start = i + 1 | |
| } | |
| } | |
| if start < len(path) { | |
| result = append(result, path[start:]) | |
| } | |
| return result | |
| } | |
| // splitPath splits a dot-separated path into parts. | |
| func splitPath(path string) []string { | |
| // Simple split - doesn't handle escaped dots | |
| result := make([]string, 0) | |
| start := 0 | |
| for i := 0; i < len(path); i++ { | |
| if path[i] == '.' { | |
| result = append(result, path[start:i]) | |
| start = i + 1 | |
| } | |
| } | |
| result = append(result, path[start:]) | |
| return result | |
| } |
🤖 Prompt for AI Agents
In `@pkg/utils/map.go` around lines 130 - 150, splitPath currently drops empty
segments (e.g., "a..b" or "a.") which masks invalid paths; change splitPath to
preserve empty segments (e.g., use strings.Split or equivalent logic that
appends even when start==i and includes a final empty segment for trailing dot)
and update GetNestedValue to validate the returned slice and return an error if
any segment == "" so malformed paths are rejected instead of silently skipped.
Ensure you modify splitPath and add the empty-segment check in GetNestedValue to
fail fast with a clear error.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-amarin 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 dc68d24
into
openshift-hyperfleet:main
2 participants
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests
Documentation