feat: Implemented interface for k8s client and maestro client integrtaion in executor#38
feat: Implemented interface for k8s client and maestro client integrtaion in executor#38xueli181114 wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
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 executor from individual phase executors to a unified pipeline-based architecture. It centralizes utility functions into Sequence Diagram(s)sequenceDiagram
participant Client as Executor Client
participant Pipeline as Pipeline
participant ParamPhase as ParamExtractionPhase
participant PrecondPhase as PreconditionsPhase
participant ResourcePhase as ResourcesPhase
participant PostPhase as PostActionsPhase
participant Applier as ResourceApplier
Client->>Pipeline: Execute(ctx, execCtx)
Pipeline->>ParamPhase: Execute(ctx, execCtx)
Note over ParamPhase: Extract params from env/event/secrets
ParamPhase->>ParamPhase: extractConfigParams()
ParamPhase-->>Pipeline: result
Pipeline->>PrecondPhase: Execute(ctx, execCtx)
Note over PrecondPhase: Check preconditions via API
PrecondPhase->>PrecondPhase: executeAll()
PrecondPhase-->>Pipeline: result
alt Preconditions Met
Pipeline->>ResourcePhase: Execute(ctx, execCtx)
Note over ResourcePhase: Apply resources via applier
ResourcePhase->>ResourcePhase: buildManifest()
ResourcePhase->>Applier: ApplyResources(ctx, resources)
Applier-->>ResourcePhase: ApplyResourcesResult
ResourcePhase-->>Pipeline: result
else Preconditions NotMet
Pipeline->>Pipeline: Skip Resource & Post phases
end
alt Resources Succeeded
Pipeline->>PostPhase: Execute(ctx, execCtx)
Note over PostPhase: Always execute post-actions
PostPhase->>PostPhase: executeAll()
PostPhase-->>Pipeline: result
else Resources Failed
Pipeline->>PostPhase: Execute(ctx, execCtx)
Note over PostPhase: Post-actions run regardless
PostPhase-->>Pipeline: result
end
Pipeline-->>Client: []PhaseResult
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes This is a substantial architectural refactoring spanning 40+ files with heterogeneous changes across multiple subsystems. The changes introduce a new pipeline-phase execution model, establish new abstraction boundaries (resource_applier), centralize utilities into pkg/utils, migrate core logic to internal/manifest, and refactor three major executor components. The high density of logic changes, diverse file types affected, and multiple interconnected architectural patterns require detailed examination of each subsystem's interactions. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@internal/executor/executor.go`:
- Around line 26-49: The stored phase instances on Executor
(paramExtractionPhase, preconditionsPhase, resourcesPhase, postActionsPhase) are
shareable mutable state and can race across concurrent Execute runs; change the
code to create fresh phase instances inside the Executor.Execute method (call
NewParamExtractionPhase, NewPreconditionsPhase, NewResourcesPhase,
NewPostActionsPhase at the start of Execute) and use those local variables for
pipeline execution and for buildExecutionResult reads instead of the Executor
fields; alternatively, if you must keep them on the Executor, protect all phase
Execute and buildExecutionResult accesses with a mutex on the Executor to
serialize runs—update references in buildExecutionResult and Execute to use the
new local instances or to acquire the mutex accordingly.
In `@internal/executor/phase_param_extraction.go`:
- Around line 56-70: The extractConfigParams method currently calls extractParam
with execCtx.Ctx which can differ from the context passed into Execute; change
extractConfigParams (ParamExtractionPhase.extractConfigParams) to accept the
Execute context (ctx context.Context) or otherwise use the ctx from Execute and
pass that ctx into extractParam instead of execCtx.Ctx, and update Execute's
call site to forward its ctx into extractConfigParams; ensure all references to
execCtx.Ctx inside extractConfigParams are replaced with the supplied ctx so
ExecutionContext and extractParam always use the same request-scoped context.
- Around line 171-179: The addMetadataParams function accesses
p.config.Metadata.Name/Namespace/Labels without checking p.config.Metadata; add
a defensive nil check in ParamExtractionPhase.addMetadataParams to handle
missing Metadata (either return/skip adding metadata or populate defaults) or
alternatively mark the AdapterConfig.Metadata field as required
(validate:"required") in the types definition; update addMetadataParams to
safely read p.config.Metadata only after confirming it's non-nil and use empty
values/maps when absent to avoid panics.
In `@internal/k8s_client/apply.go`:
- Around line 228-261: The waitForDeletion loop currently relies solely on the
caller's context and can poll indefinitely; modify Client.waitForDeletion to
impose a built-in timeout by creating a derived context with
context.WithTimeout(ctx, <reasonableDuration>) (e.g., deletionTimeout constant)
and use that derived context in the select loop (remember to defer cancel()), so
the function will stop polling when the deadline is exceeded; update the
error/log path to report the deadline (wrap/return ctx.Err() when deadline
exceeded) and keep using GetResource and apierrors.IsNotFound as the existence
check; alternatively, if you prefer no hard-coded timeout, add clear
documentation to the waitForDeletion comment that callers MUST pass a context
with a deadline.
In `@internal/k8s_client/mock.go`:
- Around line 105-121: The ApplyResource method on MockK8sClient stores the
incoming newManifest directly, risking external mutation; change it to store a
deep copy and return that copy in the ApplyResult: use newManifest.DeepCopy()
when assigning to m.Resources[key] (and return the copied object as the Resource
in the ApplyResult) so behavior matches CreateResource/UpdateResource and
prevents outside changes from affecting the mock's internal state.
In `@internal/maestro_client/resource_applier.go`:
- Around line 248-263: determineOperation currently relies on ResourceVersion
(which is always set) and thus always returns OperationUpdate; instead change
the contract so the caller (ApplyManifestWork) passes the actual operation it
determined: modify determineOperation signature to accept an explicit
manifest.Operation (or a boolean isCreate) and return that operation rather than
inferring from ResourceVersion; update all call sites (notably
ApplyManifestWork) to pass the operation computed there (using its generation
comparison logic) into determineOperation and remove the ResourceVersion-based
logic inside determineOperation.
In `@pkg/utils/convert.go`:
- Around line 92-95: The float32/float64 cases in the type switch in
pkg/utils/convert.go currently cast directly to int64 (cases for float32 and
float64) which can produce undefined/incorrect results for values outside the
int64 range; update those cases to first check bounds using math.MaxInt64 and
math.MinInt64 (and detect NaN/±Inf) and return a clear overflow/error instead of
casting when v > MaxInt64 or v < MinInt64 (for float32 convert via float64 first
or compare using float32 limits), otherwise safely cast to int64; ensure the
error returned is descriptive so callers of the conversion function can handle
the overflow.
🧹 Nitpick comments (10)
internal/manifest/render.go (1)
48-57: Consider adding index to slice rendering errors for better debugging.When rendering fails for a slice element, the error doesn't indicate which index failed. This can make debugging difficult for large arrays.
💡 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("index [%d]: %w", i, err) } result[i] = rendered } return result, nilpkg/utils/template.go (1)
21-26:dateanddateFormatare identical functions.Both functions have the same signature and implementation. Consider consolidating to a single function, or if both are kept for compatibility reasons, add a comment explaining why.
💡 Suggested consolidation
"date": func(layout string, t time.Time) string { return t.Format(layout) }, - "dateFormat": func(layout string, t time.Time) string { - return t.Format(layout) - }, + // dateFormat is an alias for date, provided for template compatibility + "dateFormat": func(layout string, t time.Time) string { + return t.Format(layout) + },Or simply:
"dateFormat": nil, // set after declaration // Then after the map: TemplateFuncs["dateFormat"] = TemplateFuncs["date"]internal/maestro_client/operations_test.go (1)
3-4: Stale file path reference in comment.The comment references
internal/generation/generation_test.go, but the generation functionality has been migrated to themanifestpackage. Consider updating this path to reflect the new location.📝 Suggested fix
// Package maestro_client tests // -// 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. // This file contains tests specific to maestro_client functionality.pkg/utils/map.go (1)
130-162: Consider using standard library functions for path operations.The
splitPathandjoinPathfunctions can be simplified usingstrings.Splitandstrings.Joinfrom the standard library, which are well-tested and more idiomatic.♻️ Suggested simplification
+import "strings" + // 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 + return strings.Split(path, ".") } // joinPath joins path parts with dots. func joinPath(parts []string) string { - if len(parts) == 0 { - return "" - } - result := parts[0] - for i := 1; i < len(parts); i++ { - result += "." + parts[i] - } - return result + return strings.Join(parts, ".") }Note: The current implementation filters empty segments (e.g.,
"a..b"→["a", "b"]), whilestrings.Splitpreserves them (["a", "", "b"]). If filtering is intentional, keep the current implementation or add explicit filtering.internal/executor/phase.go (1)
74-85: Skipped phases due to prior failure are marked asStatusSuccess.When a phase is skipped because a previous phase failed (lines 77-84), the
Statusis set toStatusSuccess, which seems semantically incorrect. A skipped phase didn't actually succeed — it was never executed. This could be misleading when analyzing results.Consider using a distinct status (e.g.,
StatusSkipped) if available inExecutionStatus, or clarify in documentation thatStatusSuccesscombined withSkipped=trueindicates an intentional skip rather than actual execution success.♻️ Potential clarification
If a
StatusSkippedconstant exists or can be added:results = append(results, PhaseResult{ Phase: phaseName, - Status: StatusSuccess, + Status: StatusSkipped, Skipped: true, SkipReason: "previous phase failed", Duration: time.Since(start), })Alternatively, document that
Skipped=trueshould be checked first, andStatusonly applies whenSkipped=false.internal/manifest/generation.go (2)
241-250: Orphaned documentation comment without corresponding function.Lines 241-250 contain a documentation comment for
ValidateManifestbut no function implementation follows. This comment appears to be detached from its function, which based on the relevant snippets exists ininternal/manifest/manifest.go. Consider removing this orphaned comment to avoid confusion.🔧 Suggested fix
-// 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.
374-378: Silently skipping malformed label selector pairs may hide configuration errors.When
MatchesLabelsencounters a malformed selector pair (missing=), it silently continues. This could mask invalid selector strings in configuration. Consider logging a warning or returning an error for malformed selectors.🔧 Suggested improvement
for _, pair := range pairs { kv := strings.SplitN(pair, "=", 2) if len(kv) != 2 { - continue + continue // Skip malformed pair (e.g., empty string between commas) }internal/executor/phase_param_extraction.go (1)
111-127: Default case silently treats unknown source prefixes as event data paths.When an unrecognized source prefix is provided (e.g.,
"unknown.foo"), it falls through toextractFromEvent, which may return confusing errors or unexpected behavior. Consider logging a warning or validating known source prefixes explicitly.internal/maestro_client/resource_applier.go (1)
164-191: IncorrectGroupResource.Resourcefield: should be the resource name (plural), notKind.In Kubernetes API conventions,
GroupResource.Resourceshould be the plural resource name (e.g.,"deployments"), not the Kind (e.g.,"Deployment"). Usinggvk.Kindwill produce incorrect error messages and may not match expected API conventions.However, since this is constructing a NotFound error for a resource within a ManifestWork (not a real K8s resource lookup), this is an internal detail. The error message will still be informative, but it's technically incorrect.
🔧 Consider documenting or fixing the GroupResource usage
If you want technically correct API errors, you'd need a way to determine the plural resource name. For now, adding a comment explaining the limitation would suffice:
// Resource not found in ManifestWork + // Note: Using Kind as Resource since we don't have the plural resource name available gr := schema.GroupResource{Group: gvk.Group, Resource: gvk.Kind} return nil, apierrors.NewNotFound(gr, name)internal/executor/phase_preconditions.go (1)
61-66: Setting bothSetErrorandSetSkippedon precondition failure may cause confusion.When a precondition evaluation fails with an error (e.g., API call failure), the code calls both
SetErrorandSetSkipped. This could lead to ambiguous state whereResourcesSkippedis true but an error also occurred. Consider whetherSetSkippedshould only be called for non-error "not met" cases.🔧 Suggested clarification
if outcome.Error != nil { // Process execution error: precondition evaluation failed execCtx.SetError("PreconditionFailed", outcome.Error.Error()) - execCtx.SetSkipped("PreconditionFailed", outcome.Error.Error()) return fmt.Errorf("precondition evaluation failed: %w", outcome.Error) }If downstream phases need to know resources should be skipped on error, consider a separate flag or letting the returned error handle flow control.
| log := config.Logger | ||
|
|
||
| // Create phases (each phase contains its own business logic) | ||
| paramExtractionPhase := NewParamExtractionPhase(config.AdapterConfig, config.K8sClient, log) | ||
| preconditionsPhase := NewPreconditionsPhase(config.APIClient, config.AdapterConfig, log) | ||
| resourcesPhase := NewResourcesPhase(config.K8sClient, config.AdapterConfig, log) | ||
| postActionsPhase := NewPostActionsPhase(config.APIClient, config.AdapterConfig, log) | ||
|
|
||
| // Create pipeline with all phases | ||
| pipeline := NewPipeline(log, | ||
| paramExtractionPhase, | ||
| preconditionsPhase, | ||
| resourcesPhase, | ||
| postActionsPhase, | ||
| ) | ||
|
|
||
| return &Executor{ | ||
| config: config, | ||
| precondExecutor: newPreconditionExecutor(config), | ||
| resourceExecutor: newResourceExecutor(config), | ||
| postActionExecutor: newPostActionExecutor(config), | ||
| log: config.Logger, | ||
| config: config, | ||
| log: log, | ||
| pipeline: pipeline, | ||
| paramExtractionPhase: paramExtractionPhase, | ||
| preconditionsPhase: preconditionsPhase, | ||
| resourcesPhase: resourcesPhase, | ||
| postActionsPhase: postActionsPhase, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the phase Execute implementations
echo "=== Phase Execute implementations ==="
rg -n --type=go -C5 'func \(.*\*.*Phase\) Execute' internal/executor/
echo ""
echo "=== State assignments in executor package ==="
rg -n --type=go '\.(results|outcome|state)\s*=' internal/executor/
echo ""
echo "=== Phase struct definitions ==="
rg -n --type=go '^type.*Phase struct' internal/executor/Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 4105
🏁 Script executed:
#!/bin/bash
# Get phase struct definitions with their fields
echo "=== Phase struct definitions with fields ==="
rg -n --type=go -A10 '^type.*Phase struct' internal/executor/
echo ""
echo "=== Executor struct definition ==="
rg -n --type=go -A15 '^type Executor struct' internal/executor/
echo ""
echo "=== Executor.Execute method ==="
rg -n --type=go -A20 'func \(\s*e\s*\*Executor\) Execute\(' internal/executor/Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 6361
🏁 Script executed:
#!/bin/bash
# Get full Executor.Execute method
echo "=== Full Executor.Execute method ==="
rg -n --type=go -A100 'func \(\s*e\s*\*Executor\) Execute\(' internal/executor/executor.go | head -120
echo ""
echo "=== How results are retrieved (GetResults methods) ==="
rg -n --type=go 'func.*GetResults\|GetOutcome\|GetPreconditions' internal/executor/
echo ""
echo "=== Pipeline Execute method ==="
rg -n --type=go -A30 'func \(\s*p\s*\*Pipeline\) Execute\(' internal/executor/Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 6039
Guard shared phase state across concurrent Executor execution.
Lines 29-38 store phase instances on the Executor. These phases maintain mutable state fields (results, outcome) that are written during each phase Execute and read in buildExecutionResult (lines 157, 159, 166, 170). If the same Executor instance is used concurrently, results can race: one execution could be writing to a phase's state while another reads it. Consider instantiating phases per Execute call, adding synchronization (mutex), or documenting and enforcing single-threaded use.
🤖 Prompt for AI Agents
In `@internal/executor/executor.go` around lines 26 - 49, The stored phase
instances on Executor (paramExtractionPhase, preconditionsPhase, resourcesPhase,
postActionsPhase) are shareable mutable state and can race across concurrent
Execute runs; change the code to create fresh phase instances inside the
Executor.Execute method (call NewParamExtractionPhase, NewPreconditionsPhase,
NewResourcesPhase, NewPostActionsPhase at the start of Execute) and use those
local variables for pipeline execution and for buildExecutionResult reads
instead of the Executor fields; alternatively, if you must keep them on the
Executor, protect all phase Execute and buildExecutionResult accesses with a
mutex on the Executor to serialize runs—update references in
buildExecutionResult and Execute to use the new local instances or to acquire
the mutex accordingly.
| // extractConfigParams extracts all configured parameters and populates execCtx.Params | ||
| func (p *ParamExtractionPhase) extractConfigParams(execCtx *ExecutionContext) error { | ||
| for _, param := range p.config.Spec.Params { | ||
| value, err := p.extractParam(execCtx.Ctx, param, execCtx.EventData) | ||
| if err != nil { | ||
| if param.Required { | ||
| return NewExecutorError(PhaseParamExtraction, param.Name, | ||
| fmt.Sprintf("failed to extract required parameter '%s' from source '%s'", param.Name, param.Source), err) | ||
| } | ||
| // Use default for non-required params if extraction fails | ||
| if param.Default != nil { | ||
| execCtx.Params[param.Name] = param.Default | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
Inconsistent context usage: extractParam uses execCtx.Ctx instead of the ctx passed to Execute.
The Execute method receives a context.Context parameter, but extractConfigParams passes execCtx.Ctx to extractParam. This could lead to issues if the contexts differ (e.g., different cancellation or deadline). Use the context from Execute consistently.
🔧 Suggested fix
-func (p *ParamExtractionPhase) extractConfigParams(execCtx *ExecutionContext) error {
+func (p *ParamExtractionPhase) extractConfigParams(ctx context.Context, execCtx *ExecutionContext) error {
for _, param := range p.config.Spec.Params {
- value, err := p.extractParam(execCtx.Ctx, param, execCtx.EventData)
+ value, err := p.extractParam(ctx, param, execCtx.EventData)And update the call site in Execute:
func (p *ParamExtractionPhase) Execute(ctx context.Context, execCtx *ExecutionContext) error {
// Extract configured parameters
- if err := p.extractConfigParams(execCtx); err != nil {
+ if err := p.extractConfigParams(ctx, execCtx); err != nil {🤖 Prompt for AI Agents
In `@internal/executor/phase_param_extraction.go` around lines 56 - 70, The
extractConfigParams method currently calls extractParam with execCtx.Ctx which
can differ from the context passed into Execute; change extractConfigParams
(ParamExtractionPhase.extractConfigParams) to accept the Execute context (ctx
context.Context) or otherwise use the ctx from Execute and pass that ctx into
extractParam instead of execCtx.Ctx, and update Execute's call site to forward
its ctx into extractConfigParams; ensure all references to execCtx.Ctx inside
extractConfigParams are replaced with the supplied ctx so ExecutionContext and
extractParam always use the same request-scoped context.
| // addMetadataParams adds adapter and event metadata to execCtx.Params | ||
| func (p *ParamExtractionPhase) addMetadataParams(execCtx *ExecutionContext) { | ||
| // Add metadata from adapter config | ||
| execCtx.Params["metadata"] = map[string]interface{}{ | ||
| "name": p.config.Metadata.Name, | ||
| "namespace": p.config.Metadata.Namespace, | ||
| "labels": p.config.Metadata.Labels, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the Metadata struct definition
rg -nA10 'type Metadata struct' internal/config_loader/Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 912
🏁 Script executed:
# Check validator_schema.go for Metadata validation rules
cat -n internal/config_loader/validator_schema.go | sed -n '1,200p'Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 157
🏁 Script executed:
# Search for Metadata in validator_schema.go to see validation setup
rg -n 'Metadata' internal/config_loader/validator_schema.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 165
🏁 Script executed:
# Find validation-related files
fd -t f 'validat' internal/config_loader/Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 202
🏁 Script executed:
# Check AdapterConfig struct definition for Metadata field
rg -nA30 'type AdapterConfig struct' internal/config_loader/types.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1465
🏁 Script executed:
# Check how ParamExtractionPhase is initialized and config is guaranteed
rg -nB5 'ParamExtractionPhase' internal/executor/ | head -50Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 3841
🏁 Script executed:
# Check validator.go for Metadata validation logic
cat -n internal/config_loader/validator.go | head -100Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 4013
🏁 Script executed:
# Check if there's additional validation for Metadata as required
rg -nA10 'Metadata' internal/config_loader/validator.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 850
🏁 Script executed:
# Get the full context of the addMetadataParams function
cat -n internal/executor/phase_param_extraction.go | sed -n '165,185p'Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 936
🏁 Script executed:
# Check struct_validator.go
cat -n internal/config_loader/struct_validator.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 11377
🏁 Script executed:
# Search for any special handling of Metadata as required
rg -nC5 'Metadata' internal/config_loader/struct_validator.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
# Search for how configs are loaded
rg -n 'LoadConfig\|UnmarshalYAML\|yaml.Unmarshal' internal/config_loader/ -A5 | head -60Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
# Check tests to see if Metadata is always present
fd -t f '*test.go' internal/config_loader/ -exec rg -l 'Metadata' {} \;Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 571
🏁 Script executed:
# Look at example configs or test data
find . -name '*.yaml' -o -name '*.yml' | head -20Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 754
🏁 Script executed:
# Check example adapter config
cat ./charts/examples/adapterconfig.yamlRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 8540
🏁 Script executed:
# Check the adapter config template
cat ./configs/adapter-config-template.yamlRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 13053
🏁 Script executed:
# Search for config loading function
rg -n 'func.*Load' internal/config_loader/ -A10 | head -80Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 5511
🏁 Script executed:
# Get the full Load function and Parse function from loader.go
cat -n internal/config_loader/loader.go | sed -n '80,150p'Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 2880
🏁 Script executed:
# Check if Metadata is ever nil or uninitialized in the config loading process
rg -n 'Metadata' internal/config_loader/loader.go -B2 -A2Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
# Check actual test to see if Metadata is always present in loaded configs
cat -n internal/config_loader/loader_test.go | sed -n '14,75p'Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 2072
Add defensive nil check for p.config.Metadata fields or validate Metadata as required.
The Metadata field in AdapterConfig lacks a validate:"required" tag, so it could theoretically be nil when parsing a config that omits the metadata section. Accessing p.config.Metadata.Name, Namespace, and Labels directly in addMetadataParams without defensive checks risks a panic. Either:
- Add a nil check:
if p.config.Metadata == nil { /* handle error */ } - Mark
Metadataas required in the struct tag:validate:"required"on line 72 oftypes.go
This is low-risk in practice since all example configs include metadata, but defensive coding is safer.
🤖 Prompt for AI Agents
In `@internal/executor/phase_param_extraction.go` around lines 171 - 179, The
addMetadataParams function accesses p.config.Metadata.Name/Namespace/Labels
without checking p.config.Metadata; add a defensive nil check in
ParamExtractionPhase.addMetadataParams to handle missing Metadata (either
return/skip adding metadata or populate defaults) or alternatively mark the
AdapterConfig.Metadata field as required (validate:"required") in the types
definition; update addMetadataParams to safely read p.config.Metadata only after
confirming it's non-nil and use empty values/maps when absent to avoid panics.
| // waitForDeletion polls until the resource is confirmed deleted or context times out. | ||
| // Returns nil when the resource is confirmed gone (NotFound), or an error otherwise. | ||
| func (c *Client) waitForDeletion( | ||
| ctx context.Context, | ||
| gvk schema.GroupVersionKind, | ||
| namespace, name string, | ||
| ) error { | ||
| const pollInterval = 100 * time.Millisecond | ||
|
|
||
| ticker := time.NewTicker(pollInterval) | ||
| defer ticker.Stop() | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| c.log.Warnf(ctx, "Context cancelled/timed out while waiting for deletion of %s/%s", gvk.Kind, name) | ||
| return fmt.Errorf("context cancelled while waiting for resource deletion: %w", ctx.Err()) | ||
| case <-ticker.C: | ||
| _, err := c.GetResource(ctx, gvk, namespace, name) | ||
| if err != nil { | ||
| // NotFound means the resource is deleted - this is success | ||
| if apierrors.IsNotFound(err) { | ||
| c.log.Debugf(ctx, "Resource deletion confirmed: %s/%s", gvk.Kind, name) | ||
| return nil | ||
| } | ||
| // Any other error is unexpected | ||
| c.log.Errorf(ctx, "Error checking deletion status for %s/%s: %v", gvk.Kind, name, err) | ||
| return fmt.Errorf("error checking deletion status: %w", err) | ||
| } | ||
| // Resource still exists, continue polling | ||
| c.log.Debugf(ctx, "Resource %s/%s still exists, waiting for deletion...", gvk.Kind, name) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nC5 'waitForDeletion' --type=goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1676
🏁 Script executed:
sed -n '200,230p' internal/k8s_client/apply.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1421
🏁 Script executed:
rg -nC3 'recreateResource' --type=goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1241
🏁 Script executed:
sed -n '80,120p' internal/k8s_client/apply.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1341
🏁 Script executed:
sed -n '1,80p' internal/k8s_client/apply.go | grep -A 20 "func.*ctx"Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
sed -n '40,100p' internal/k8s_client/apply.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1868
🏁 Script executed:
rg -nC3 '\.ApplyResource\(' --type=goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1512
🏁 Script executed:
sed -n '150,180p' internal/k8s_client/apply.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1038
🏁 Script executed:
rg -nC3 'ApplyResources' --type=goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 11325
🏁 Script executed:
sed -n '80,110p' internal/executor/phase_resources.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1345
🏁 Script executed:
rg -nC3 'executeAll' --type=goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 4782
🏁 Script executed:
rg -nC3 'ResourcesPhase.*Execute\|\.Execute\(ctx' internal/executor --type=go | head -50Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
rg 'context\.WithTimeout|context\.WithCancel' --type=goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 2938
🏁 Script executed:
sed -n '1,100p' cmd/adapter/main.go | grep -A 20 "ctx.*WithCancel"Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
rg -n 'ctx.*WithCancel\|executor.*Execute' cmd/adapter/main.go -A 5 -B 2Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
cat cmd/adapter/main.go | head -200Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 6870
Add a timeout safeguard or document the context requirement for waitForDeletion.
The function has no built-in maximum polling duration and relies entirely on context cancellation for termination. Verification shows that the production context chain (cmd/adapter/main.go → ResourcesPhase → ApplyResources → waitForDeletion) uses context.WithCancel() without a timeout, meaning if a resource deletion hangs indefinitely in Kubernetes, the polling will continue forever with repeated GET requests at 100ms intervals.
Either add a built-in timeout to waitForDeletion itself, or clearly document that callers must provide a context with an explicit timeout deadline.
🤖 Prompt for AI Agents
In `@internal/k8s_client/apply.go` around lines 228 - 261, The waitForDeletion
loop currently relies solely on the caller's context and can poll indefinitely;
modify Client.waitForDeletion to impose a built-in timeout by creating a derived
context with context.WithTimeout(ctx, <reasonableDuration>) (e.g.,
deletionTimeout constant) and use that derived context in the select loop
(remember to defer cancel()), so the function will stop polling when the
deadline is exceeded; update the error/log path to report the deadline
(wrap/return ctx.Err() when deadline exceeded) and keep using GetResource and
apierrors.IsNotFound as the existence check; alternatively, if you prefer no
hard-coded timeout, add clear documentation to the waitForDeletion comment that
callers MUST pass a context with a deadline.
| // 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.ApplyResourceResult != nil { | ||
| return m.ApplyResourceResult, 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 | ||
| } |
There was a problem hiding this comment.
Missing DeepCopy when storing resource in mock.
Other methods (CreateResource, UpdateResource) use DeepCopy() when storing resources to prevent external mutations from affecting the mock's internal state. ApplyResource should follow the same pattern for consistency.
🔧 Proposed fix
// 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,
+ Resource: newManifest.DeepCopy(),
Operation: manifest.OperationCreate,
Reason: "mock apply",
}, nil📝 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.
| // 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.ApplyResourceResult != nil { | |
| return m.ApplyResourceResult, 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 | |
| } | |
| // 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.ApplyResourceResult != nil { | |
| return m.ApplyResourceResult, nil | |
| } | |
| // Default behavior: store the resource and return create result | |
| key := newManifest.GetNamespace() + "/" + newManifest.GetName() | |
| m.Resources[key] = newManifest.DeepCopy() | |
| return &ApplyResult{ | |
| Resource: newManifest.DeepCopy(), | |
| Operation: manifest.OperationCreate, | |
| Reason: "mock apply", | |
| }, nil | |
| } |
🤖 Prompt for AI Agents
In `@internal/k8s_client/mock.go` around lines 105 - 121, The ApplyResource method
on MockK8sClient stores the incoming newManifest directly, risking external
mutation; change it to store a deep copy and return that copy in the
ApplyResult: use newManifest.DeepCopy() when assigning to m.Resources[key] (and
return the copied object as the Resource in the ApplyResult) so behavior matches
CreateResource/UpdateResource and prevents outside changes from affecting the
mock's internal state.
| // determineOperation determines the operation that was performed based on the ManifestWork. | ||
| func (a *ResourceApplier) determineOperation(work *workv1.ManifestWork) manifest.Operation { | ||
| if work == nil { | ||
| return manifest.OperationCreate | ||
| } | ||
|
|
||
| // Check if this is a new ManifestWork (no resourceVersion set by server) | ||
| // or an updated one | ||
| if work.ResourceVersion == "" { | ||
| return manifest.OperationCreate | ||
| } | ||
|
|
||
| // If the generation matches what we set, it could be create or update | ||
| // The actual determination was done in ApplyManifestWork | ||
| return manifest.OperationUpdate | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the ApplyManifestWork function implementation
cd /repo && ast-grep --pattern 'func ($receiver) ApplyManifestWork($$$) {
$$$
}'Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Find ApplyManifestWork function in the codebase
ast-grep --pattern 'func ($receiver) ApplyManifestWork($$$) {
$$$
}'Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
#!/bin/bash
# Search for ApplyManifestWork function definition
rg "func.*ApplyManifestWork" -A 20Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 3510
🏁 Script executed:
#!/bin/bash
# Read the full ApplyManifestWork function
rg "func \(c \*Client\) ApplyManifestWork" -A 100 internal/maestro_client/operations.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 3574
🏁 Script executed:
#!/bin/bash
# Find where determineOperation is called
rg "determineOperation" internal/maestro_client/resource_applier.go -B 5 -A 5Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 872
🏁 Script executed:
#!/bin/bash
# Check what CreateManifestWork and PatchManifestWork return
rg "func \(c \*Client\) CreateManifestWork" -A 30 internal/maestro_client/operations.go | head -40Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1283
🏁 Script executed:
#!/bin/bash
# Check what PatchManifestWork returns
rg "func \(c \*Client\) PatchManifestWork" -A 30 internal/maestro_client/operations.go | head -40Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 887
🏁 Script executed:
#!/bin/bash
# Verify that Kubernetes client operations set ResourceVersion
# Check the full CreateManifestWork to see what's returned
rg "func \(c \*Client\) CreateManifestWork" -A 50 internal/maestro_client/operations.go | head -55Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1889
🏁 Script executed:
#!/bin/bash
# Get more context on where determineOperation is used in resource_applier.go
rg "determineOperation" internal/maestro_client/resource_applier.go -B 10 -A 10Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1467
🏁 Script executed:
#!/bin/bash
# Check the full context of determineOperation function
cat -n internal/maestro_client/resource_applier.go | sed -n '248,270p'Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1061
🏁 Script executed:
#!/bin/bash
# Verify: Check if there's any scenario where ApplyManifestWork might return work with empty ResourceVersion
# Look at what ApplyManifestWork actually returns
rg "switch decision.Operation" -A 20 internal/maestro_client/operations.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 950
🏁 Script executed:
#!/bin/bash
# Double check: understand standard Kubernetes behavior
# Check if there's any documentation or comments about ResourceVersion behavior
rg "ResourceVersion" internal/maestro_client/ -B 2 -A 2 | head -50Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 469
🏁 Script executed:
#!/bin/bash
# Let's check what the actual flow is when ApplyManifestWork is called
# and verify the context of how determineOperation is used
rg "ApplyManifestWork" internal/maestro_client/resource_applier.go -B 5 -A 15Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 2268
🏁 Script executed:
#!/bin/bash
# Check how the result of determineOperation is used
rg "a.determineOperation\|determineOperation\(appliedWork\)" internal/maestro_client/resource_applier.go -A 15Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
#!/bin/bash
# Find how the result of determineOperation is used
rg "op := a.determineOperation" internal/maestro_client/resource_applier.go -A 20Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 817
🏁 Script executed:
#!/bin/bash
# Verify the Kubernetes behavior: when Create API is called, does it set ResourceVersion?
# Check test files to see what the actual behavior is
rg "ResourceVersion" test/ -l | head -5Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 196
determineOperation will always return OperationUpdate because ResourceVersion is set by the Kubernetes API for both create and patch operations.
The function checks if work.ResourceVersion == "" to determine if a new resource was created. However, the Kubernetes API server sets ResourceVersion on all successful responses—whether from Create, Patch, or Update operations. Since ApplyManifestWork returns the result directly from these API calls, the ResourceVersion will never be empty.
This means:
- Create operations:
ResourceVersionis set by the server → function returnsOperationUpdate(incorrect) - Patch operations:
ResourceVersionis set by the server → function returnsOperationUpdate(correct)
The actual operation type was already correctly determined in ApplyManifestWork using generation comparison, but that information is lost by the time the function checks ResourceVersion. The caller receives an incorrect operation type for creates.
🤖 Prompt for AI Agents
In `@internal/maestro_client/resource_applier.go` around lines 248 - 263,
determineOperation currently relies on ResourceVersion (which is always set) and
thus always returns OperationUpdate; instead change the contract so the caller
(ApplyManifestWork) passes the actual operation it determined: modify
determineOperation signature to accept an explicit manifest.Operation (or a
boolean isCreate) and return that operation rather than inferring from
ResourceVersion; update all call sites (notably ApplyManifestWork) to pass the
operation computed there (using its generation comparison logic) into
determineOperation and remove the ResourceVersion-based logic inside
determineOperation.
| case float32: | ||
| return int64(v), nil | ||
| case float64: | ||
| return int64(v), nil |
There was a problem hiding this comment.
Float-to-int64 conversion may produce unexpected results for extreme values.
When converting float64 values outside the int64 range (e.g., 1e20 or -1e20), the conversion produces undefined behavior per the Go spec. Consider adding bounds checking similar to the uint64 overflow check.
🛡️ Proposed fix
case float32:
- return int64(v), nil
+ if v > float32(math.MaxInt64) || v < float32(math.MinInt64) {
+ return 0, fmt.Errorf("float32 value %v overflows int64", v)
+ }
+ return int64(v), nil
case float64:
- return int64(v), nil
+ if v > float64(math.MaxInt64) || v < float64(math.MinInt64) {
+ return 0, fmt.Errorf("float64 value %v overflows int64", v)
+ }
+ return int64(v), nil📝 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.
| case float32: | |
| return int64(v), nil | |
| case float64: | |
| return int64(v), nil | |
| case float32: | |
| if v > float32(math.MaxInt64) || v < float32(math.MinInt64) { | |
| return 0, fmt.Errorf("float32 value %v overflows int64", v) | |
| } | |
| return int64(v), nil | |
| case float64: | |
| if v > float64(math.MaxInt64) || v < float64(math.MinInt64) { | |
| return 0, fmt.Errorf("float64 value %v overflows int64", v) | |
| } | |
| return int64(v), nil |
🤖 Prompt for AI Agents
In `@pkg/utils/convert.go` around lines 92 - 95, The float32/float64 cases in the
type switch in pkg/utils/convert.go currently cast directly to int64 (cases for
float32 and float64) which can produce undefined/incorrect results for values
outside the int64 range; update those cases to first check bounds using
math.MaxInt64 and math.MinInt64 (and detect NaN/±Inf) and return a clear
overflow/error instead of casting when v > MaxInt64 or v < MinInt64 (for float32
convert via float64 first or compare using float32 limits), otherwise safely
cast to int64; ensure the error returned is descriptive so callers of the
conversion function can handle the overflow.
1 participant
Summary by CodeRabbit
Refactor
New Features