feat: maestro client implemented in adapter framework#37
feat: maestro client implemented in adapter framework#37xueli181114 wants to merge 3 commits intoopenshift-hyperfleet:mainfrom
Conversation
WalkthroughThis PR adds a generation-based resource tracking package and integrates it into the executor (new applyResource flow), removes legacy generation helpers, and updates ResourceResult to use generation.Operation. It introduces a Maestro client (gRPC + OpenAPI) with ManifestWork CRUD/upsert and an OCM logger adapter, plus Maestro integration tests. It adds a sample AdapterDeploymentConfig, removes the default adapter config, centralizes build/version info in pkg/version, migrates logging/context keys to string constants, updates README/config docs, and bumps multiple dependencies and build/linker settings. Sequence Diagram(s)sequenceDiagram
participant Executor as ResourceExecutor
participant Apply as applyResource
participant Gen as generation.Package
participant K8s as K8sClient
participant ExecCtx as ExecutionContext
Executor->>Apply: executeResource(resource, manifest)
Apply->>K8s: discover existing resource (selector/name)
alt resource exists
K8s-->>Apply: existing resource
Apply->>Gen: GetGenerationFromUnstructured(existing)
Gen-->>Apply: existingGen
else not found
K8s-->>Apply: NotFound
Note right of Apply: existingGen = 0
end
Apply->>Gen: GetGenerationFromUnstructured(manifest)
Gen-->>Apply: newGen
Apply->>Gen: CompareGenerations(newGen, existingGen, exists)
Gen-->>Apply: ApplyDecision(operation, reason)
alt create
Apply->>K8s: CreateResource(manifest)
else update
Apply->>K8s: PatchResource(manifest)
else recreate
Apply->>K8s: DeleteResource(existing)
Apply->>K8s: CreateResource(manifest)
else skip
Apply-->>Executor: return existing (skip)
end
Apply-->>ExecCtx: store ResourceResult(operation, resource)
sequenceDiagram
participant Caller as Adapter
participant Maestro as MaestroClient
participant Gen as generation.Package
participant OCM as WorkV1Client
participant OpenAPI as MaestroOpenAPI
Caller->>Maestro: ApplyManifestWork(consumer, work)
Maestro->>Gen: ValidateManifestWorkGeneration(work)
Maestro->>OCM: GetManifestWork(consumer, name)
alt exists
OCM-->>Maestro: existing work
Maestro->>Gen: CompareGenerations(newGen, existingGen, true)
Gen-->>Maestro: ApplyDecision
else not found
Note right of Maestro: decision = create
end
alt create
Maestro->>OCM: CreateManifestWork(consumer, work)
else update
Maestro->>Maestro: createManifestWorkPatch(work)
Maestro->>OCM: PatchManifestWork(consumer, name, patch)
else skip
Maestro-->>Caller: return existing
end
Maestro->>OpenAPI: (optional) register consumer / health checks
Maestro-->>Caller: result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
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)
pkg/errors/error_test.go (1)
74-102:⚠️ Potential issue | 🟡 MinorMissing new error codes in
requiredCodesvalidation.The expected count was updated to 17, but
requiredCodesstill only contains 15 codes. AddErrorMaestroErrorandErrorConfigurationErrorto ensure these new error codes are explicitly validated.Proposed fix
requiredCodes := []ServiceErrorCode{ ErrorNotFound, ErrorValidation, ErrorConflict, ErrorForbidden, ErrorUnauthorized, ErrorUnauthenticated, ErrorBadRequest, ErrorMalformedRequest, ErrorNotImplemented, ErrorGeneral, ErrorAdapterConfigNotFound, ErrorBrokerConnectionError, ErrorKubernetesError, ErrorHyperFleetAPIError, ErrorInvalidCloudEvent, + ErrorMaestroError, + ErrorConfigurationError, }internal/k8s_client/mock.go (1)
3-10:⚠️ Potential issue | 🟠 MajorPopulate
ApplyResourceResult.Operationand avoid silentnilon unknown ops.
ApplyResourcescurrently appends results withoutOperation, so callers inspecting create/update/skip always see the zero value. Also,ApplyResourcefalls through toreturn nil, nilon an unexpected operation, which can mask bugs. Consider centralizing apply logic into a helper that returnsApplyResourceResultwithOperationset and returns an explicit error for unknown operations.✅ Suggested refactor to preserve Operation and return explicit errors
import ( "context" + "fmt" "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/generation" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" ) @@ -// ApplyResource implements K8sClient.ApplyResource -// It creates or updates a resource based on generation comparison -func (m *MockK8sClient) ApplyResource(ctx context.Context, obj *unstructured.Unstructured) (*unstructured.Unstructured, error) { - if m.ApplyResourceError != nil { - return nil, m.ApplyResourceError - } - if m.ApplyResourceResult != nil { - return m.ApplyResourceResult, nil - } - - gvk := obj.GroupVersionKind() - namespace := obj.GetNamespace() - name := obj.GetName() - newGeneration := generation.GetGenerationFromUnstructured(obj) - - // Check if resource exists - existingObj, err := m.GetResource(ctx, gvk, namespace, name) - exists := err == nil - if err != nil && !apierrors.IsNotFound(err) { - return nil, err - } - - // Get existing generation (0 if not found) - var existingGeneration int64 - if exists { - existingGeneration = generation.GetGenerationFromUnstructured(existingObj) - } - - // Compare generations to determine operation - compareResult := generation.CompareGenerations(newGeneration, existingGeneration, exists) - - // Execute operation based on comparison result - switch compareResult.Operation { - case generation.OperationCreate: - return m.CreateResource(ctx, obj) - case generation.OperationSkip: - return existingObj, nil - case generation.OperationUpdate: - obj.SetResourceVersion(existingObj.GetResourceVersion()) - return m.UpdateResource(ctx, obj) - } - - return nil, nil -} +// applyResourceResult centralizes apply logic for ApplyResource/ApplyResources. +func (m *MockK8sClient) applyResourceResult(ctx context.Context, obj *unstructured.Unstructured) (ApplyResourceResult, error) { + if m.ApplyResourceError != nil { + return ApplyResourceResult{Error: m.ApplyResourceError}, m.ApplyResourceError + } + if m.ApplyResourceResult != nil { + return ApplyResourceResult{Resource: m.ApplyResourceResult}, nil + } + + gvk := obj.GroupVersionKind() + namespace := obj.GetNamespace() + name := obj.GetName() + newGeneration := generation.GetGenerationFromUnstructured(obj) + + existingObj, err := m.GetResource(ctx, gvk, namespace, name) + exists := err == nil + if err != nil && !apierrors.IsNotFound(err) { + return ApplyResourceResult{Error: err}, err + } + + var existingGeneration int64 + if exists { + existingGeneration = generation.GetGenerationFromUnstructured(existingObj) + } + + compareResult := generation.CompareGenerations(newGeneration, existingGeneration, exists) + + switch compareResult.Operation { + case generation.OperationCreate: + res, err := m.CreateResource(ctx, obj) + return ApplyResourceResult{Resource: res, Operation: compareResult.Operation, Error: err}, err + case generation.OperationSkip: + return ApplyResourceResult{Resource: existingObj, Operation: compareResult.Operation}, nil + case generation.OperationUpdate: + obj.SetResourceVersion(existingObj.GetResourceVersion()) + res, err := m.UpdateResource(ctx, obj) + return ApplyResourceResult{Resource: res, Operation: compareResult.Operation, Error: err}, err + default: + err := fmt.Errorf("unsupported operation %q", compareResult.Operation) + return ApplyResourceResult{Error: err}, err + } +} + +// ApplyResource implements K8sClient.ApplyResource +func (m *MockK8sClient) ApplyResource(ctx context.Context, obj *unstructured.Unstructured) (*unstructured.Unstructured, error) { + result, err := m.applyResourceResult(ctx, obj) + return result.Resource, err +} @@ func (m *MockK8sClient) ApplyResources(ctx context.Context, objs []*unstructured.Unstructured) ([]ApplyResourceResult, error) { results := make([]ApplyResourceResult, 0, len(objs)) for _, obj := range objs { - resource, err := m.ApplyResource(ctx, obj) - if err != nil { - results = append(results, ApplyResourceResult{Error: err}) - return results, err - } - results = append(results, ApplyResourceResult{Resource: resource}) + result, err := m.applyResourceResult(ctx, obj) + results = append(results, result) + if err != nil { + return results, err + } } return results, nil }Also applies to: 130-190
🤖 Fix all issues with AI agents
In `@configs/broker-configmap-pubsub-template.yaml`:
- Around line 89-94: The template's documented default ("Default: false")
contradicts the actual flag values; update the behavior flags so
`create_topic_if_missing` and `create_subscription_if_missing` either default to
false (recommended for production) or keep true but add a clarifying comment
that these permissive settings are intentional for development/demo usage;
modify the block containing `create_topic_if_missing` and
`create_subscription_if_missing` accordingly and ensure the comment above the
flags reflects the chosen default behavior.
In `@examples/maestro_client/adapter-deployment-config.yaml`:
- Around line 72-76: The example uses the key "httpAPI" but the main config uses
"hyperfleetApi", so update the example YAML to use "hyperfleetApi" (rename the
"httpAPI:" mapping to "hyperfleetApi:") while keeping the same nested fields
(timeout, retryAttempts, retryBackoff) and values; ensure any documentation or
nearby comments in the example are consistent with the new key so users
referencing hyperfleetApi see the same structure as the main config.
In `@internal/k8s_client/client.go`:
- Around line 479-486: The ApplyResources loop never sets
ApplyResourceResult.Operation, so callers cannot know what action was taken;
update either ApplyResource to return an operation string/enum alongside the
resource (e.g., change ApplyResource(ctx, obj) to return (resource, operation,
error)) or fetch the operation value returned by the existing ApplyResource call
and populate ApplyResourceResult.Operation before appending; modify the
ApplyResources function to capture that operation and set
ApplyResourceResult{Resource: resource, Operation: operation} for each
iteration, and update any callers/tests to handle the new return signature of
ApplyResource if you choose that route.
In `@internal/maestro_client/client.go`:
- Around line 229-335: The configureTLS function can silently return nil (no
TLS) when config.Insecure is false but no CA/cert/token files are provided;
change it to fail fast: after handling the three TLS branches (mutual TLS,
token+CA, CA-only) detect the case where config.Insecure is false and
grpcOptions.Dialer.TLSConfig is still nil and return a descriptive error (e.g.,
"no TLS configuration provided: provide CAFile or client certs or set
Insecure=true"); update configureTLS to reference Config fields (Insecure,
CAFile, ClientCertFile, ClientKeyFile, TokenFile) and
grpcOptions.Dialer.TLSConfig so callers get a clear failure instead of silently
falling back to plaintext; alternatively (optional) implement a fallback using
system root CAs if you prefer that behavior instead of returning an error.
In `@internal/maestro_client/ocm_logger_adapter.go`:
- Around line 77-84: The Fatal method on ocmLoggerAdapter currently calls
a.log.Errorf and does not terminate the process; replace that call with
a.log.Fatal and pass a single formatted message (use fmt.Sprintf(format,
args...)) so the adapter's Fatal(ctx, ...) uses a.log.Fatal(ctx, formattedMsg)
and triggers the underlying logger's os.Exit behavior instead of just logging an
error.
In `@internal/maestro_client/operations.go`:
- Around line 253-264: createManifestWorkPatch currently always includes
"labels" and "annotations" in the JSON patch which will set them to null (and
delete server-side values) if work.Labels or work.Annotations are nil; change
createManifestWorkPatch so it only adds the "labels" and "annotations" keys to
the patch map when work.Labels != nil and work.Annotations != nil respectively
(leave them out when nil) while still including "spec" (refer to
createManifestWorkPatch, work.Labels, work.Annotations, work.Spec); then marshal
and return the patch as before.
🧹 Nitpick comments (11)
configs/adapter-deployment-config.yaml (1)
85-95: Minor formatting inconsistency in YAML values.The
hyperfleetApisection uses unquoted values (2s,exponential) while themaestrosection uses quoted values ("30s","exponential"). While YAML parsers accept both, consistent quoting improves readability.Suggested fix for consistency
# HyperFleet HTTP API client hyperfleetApi: - timeout: 2s + timeout: "2s" retryAttempts: 3 - retryBackoff: exponential + retryBackoff: "exponential"examples/maestro_client/adapter-deployment-config.yaml (1)
46-59: MissinghttpCaFilein TLS config.The main deployment config includes
httpCaFilefor HTTP API TLS configuration (allowing different CA than gRPC), but this example omits it. Consider adding it for completeness, or document when it's optional.Suggested addition
# Server name for TLS verification # Environment variable: HYPERFLEET_MAESTRO_SERVER_NAME serverName: "maestro-grpc.maestro.svc.cluster.local" + + # HTTP API TLS configuration (may use different CA than gRPC) + # If not set, falls back to caFile for backwards compatibility + # Environment variable: HYPERFLEET_MAESTRO_HTTP_CA_FILE + httpCaFile: "/etc/maestro/certs/https/ca.crt"internal/generation/generation.go (1)
232-255: Avoid mutating the input list during sort.
Sortinglist.Itemsin place can surprise callers that reuse the list order. Consider copying before sorting.♻️ Suggested change to avoid in-place mutation
- // Sort by generation annotation (descending) to return the one with the latest generation - // Secondary sort by metadata.name for consistency when generations are equal - sort.Slice(list.Items, func(i, j int) bool { - genI := GetGenerationFromUnstructured(&list.Items[i]) - genJ := GetGenerationFromUnstructured(&list.Items[j]) + // Sort by generation annotation (descending) to return the one with the latest generation + // Secondary sort by metadata.name for consistency when generations are equal + items := append([]unstructured.Unstructured(nil), list.Items...) + sort.Slice(items, func(i, j int) bool { + genI := GetGenerationFromUnstructured(&items[i]) + genJ := GetGenerationFromUnstructured(&items[j]) if genI != genJ { return genI > genJ // Descending order - latest generation first } // Fall back to metadata.name for deterministic ordering when generations are equal - return list.Items[i].GetName() < list.Items[j].GetName() + return items[i].GetName() < items[j].GetName() }) - return &list.Items[0] + return &items[0]internal/maestro_client/operations_test.go (2)
100-104: Error message assertion is incomplete whenexpectErroris true.When
tt.expectErroris true andtt.errorMsgis set, the test doesn't verify that the actual error contains the expected message. This reduces test effectiveness for validating specific error conditions.🔧 Proposed fix to verify error messages
t.Run(tt.name, func(t *testing.T) { err := generation.ValidateGeneration(tt.meta) if tt.expectError { if err == nil { t.Errorf("expected error containing %q, got nil", tt.errorMsg) } + if tt.errorMsg != "" && !strings.Contains(err.Error(), tt.errorMsg) { + t.Errorf("expected error containing %q, got %q", tt.errorMsg, err.Error()) + } return }Add
"strings"to the imports.
205-206: IgnoredMarshalJSONerrors in test helpers could mask issues.While unlikely to fail for these simple objects, silently ignoring errors can hide bugs during test development.
🔧 Proposed fix to handle errors
createManifest := func(kind, name, generation string) workv1.Manifest { + t.Helper() obj := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": kind, "metadata": map[string]interface{}{ "name": name, "annotations": map[string]interface{}{ constants.AnnotationGeneration: generation, }, }, }, } - raw, _ := obj.MarshalJSON() + raw, err := obj.MarshalJSON() + if err != nil { + t.Fatalf("failed to marshal manifest: %v", err) + } return workv1.Manifest{RawExtension: runtime.RawExtension{Raw: raw}} }Apply similar changes to
createManifestNoGeneration.Also applies to: 220-221
test/integration/maestro_client/main_test.go (3)
19-19: Using:latesttag reduces test reproducibility.The
MaestroImageuses:latestwhich can lead to non-reproducible test results as the image content may change between runs. Consider pinning to a specific version or digest.🔧 Proposed fix
- MaestroImage = "quay.io/redhat-user-workloads/maestro-rhtap-tenant/maestro/maestro:latest" + // TODO: Pin to a specific version for reproducible tests + // Example: MaestroImage = "quay.io/redhat-user-workloads/maestro-rhtap-tenant/maestro/maestro:v0.1.0" + MaestroImage = "quay.io/redhat-user-workloads/maestro-rhtap-tenant/maestro/maestro:latest"
100-101: Redundantfmt.Sprintfwithprintln.Using
fmt.Sprintfwithprintlnis verbose. Consider usingfmt.Printforfmt.Printlndirectly for consistency.🔧 Proposed fix
- println(fmt.Sprintf(" HTTP API: %s", env.MaestroServerAddr)) - println(fmt.Sprintf(" gRPC: %s", env.MaestroGRPCAddr)) + fmt.Printf(" HTTP API: %s\n", env.MaestroServerAddr) + fmt.Printf(" gRPC: %s\n", env.MaestroGRPCAddr)Add
"fmt"to imports if not already present.
80-81: Consider loggingprovider.Close()errors.While the error from
provider.Close()may not be critical, silently ignoring it could hide resource cleanup issues during debugging.🔧 Proposed fix
- _ = provider.Close() + if closeErr := provider.Close(); closeErr != nil { + println("⚠️ Warning: Failed to close provider:", closeErr.Error()) + }test/integration/maestro_client/client_integration_test.go (2)
249-254: Unsafe type assertions could cause test panic.The chained type assertions on lines 251-252 can panic if the map structure doesn't match expectations. While this is test code, a panic provides less informative failure messages than explicit checks.
🔧 Proposed fix for safer assertions
// Now apply again with updated generation (should update) work.Annotations[constants.AnnotationGeneration] = "2" - configMapManifest["metadata"].(map[string]interface{})["annotations"].(map[string]interface{})[constants.AnnotationGeneration] = "2" - configMapManifest["data"].(map[string]interface{})["key2"] = "value2" - configMapJSON, _ = json.Marshal(configMapManifest) + metadata := configMapManifest["metadata"].(map[string]interface{}) + annotations := metadata["annotations"].(map[string]interface{}) + annotations[constants.AnnotationGeneration] = "2" + data := configMapManifest["data"].(map[string]interface{}) + data["key2"] = "value2" + configMapJSON, err = json.Marshal(configMapManifest) + require.NoError(t, err, "Failed to marshal updated configmap") work.Spec.Workload.Manifests[0].Raw = configMapJSON
33-38: Minor formatting inconsistency withInsecurefield.The
Insecure: truefield is not aligned with other struct fields in theConfiginitialization. While not affecting functionality, consistent formatting improves readability.🔧 Example fix
config := &maestro_client.Config{ MaestroServerAddr: env.MaestroServerAddr, GRPCServerAddr: env.MaestroGRPCAddr, SourceID: "integration-test-source", - Insecure: true, + Insecure: true, }Also applies to: 62-67, 147-152, 186-191, 280-285
internal/maestro_client/client.go (1)
217-221: Silent failure if CA certificate PEM is malformed.
AppendCertsFromPEMreturnsfalseif no valid certificates were found, but the error message doesn't indicate whether the file was empty, contained invalid PEM data, or had valid PEM but invalid certificate content.🔧 Proposed improvement for better diagnostics
if httpCAFile != "" { caCert, err := os.ReadFile(httpCAFile) if err != nil { return nil, err } + if len(caCert) == 0 { + return nil, apperrors.ConfigurationError("CA certificate file %s is empty", httpCAFile).AsError() + } caCertPool := x509.NewCertPool() if !caCertPool.AppendCertsFromPEM(caCert) { - return nil, apperrors.ConfigurationError("failed to parse CA certificate from %s", httpCAFile).AsError() + return nil, apperrors.ConfigurationError("failed to parse CA certificate from %s: file may not contain valid PEM-encoded certificates", httpCAFile).AsError() } tlsConfig.RootCAs = caCertPool }
| // Fatal logs at error level with formatting. | ||
| // Note: Does not exit - the underlying logger handles that behavior. | ||
| func (a *ocmLoggerAdapter) Fatal(ctx context.Context, format string, args ...interface{}) { | ||
| if ctx == nil { | ||
| ctx = context.Background() | ||
| } | ||
| a.log.Errorf(ctx, "FATAL: "+format, args...) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OCM SDK go logging.Logger Fatal method interface expectations
💡 Result:
In ocm-sdk-go, the logger you pass to ConnectionBuilder.Logger(...) must implement the logging.Logger interface. That interface expects a printf-style Fatal method with this exact shape:
Fatal(ctx context.Context, format string, args ...interface{})Along with the other methods:
Debug(ctx, format, args...),Info(...),Warn(...),Error(...)DebugEnabled() bool,InfoEnabled() bool,WarnEnabled() bool,ErrorEnabled() bool(there is noFatalEnabled) [1]
and it’s wired into the SDK viaConnectionBuilder.Logger(logger logging.Logger)[2].
Two key “expectations” to be aware of:
-
formatis a format string (likefmt.Sprintf). The project even provides an analyzer (OcmLoggerLinter) that checks calls likelogger.Warn(ctx, "x=%s y=%v", a, b)have the right number of formatting args, and it only applies when the receiver isgithub.com/openshift-online/ocm-sdk-go/logging.Logger(or pointer to it) [3]. -
ctxmay benilin SDK calls (explicitly called out for Debug/Info/Warn/Error), so your implementation should handlenilcontexts without panicking [1]. (In practice, it’s safest to acceptnilforFataltoo.)
Sources: [1] [2] [3]
🏁 Script executed:
# First, let's understand the underlying logger type and the adapter structure
head -100 internal/maestro_client/ocm_logger_adapter.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 2517
🏁 Script executed:
# Search for the logger type definition and Fatal method implementations in OCM SDK
rg -t go "logging\.Logger" internal/maestro_client/Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 434
🏁 Script executed:
# Check if there are any imports from the OCM SDK
rg -t go "import.*ocm\|import.*logging" internal/maestro_client/ocm_logger_adapter.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
# Find the logger package files
fd -t f "\.go$" pkg/loggerRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 240
🏁 Script executed:
# Look at the logger interface definition
rg -t go "interface.*Logger\|func.*Fatal" pkg/logger --context=3Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
# Check if Fatal is called elsewhere in the codebase
rg -t go "\.Fatal\(" --context=2Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 6195
🏁 Script executed:
# Read the logger interface definition
cat -n pkg/logger/logger.goRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 10248
🏁 Script executed:
# Also check for Fatal specifically in logger implementations
rg -t go "func.*Fatal" pkg/logger/Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 165
Fix Fatal to exit the process as expected by OCM SDK callers.
The Fatal method should call a.log.Fatal() instead of a.log.Errorf() to ensure the process terminates. The underlying logger's Fatal implementation already calls os.Exit(1), but the adapter currently bypasses it by logging at error level without exiting. Update to:
a.log.Fatal(ctx, fmt.Sprintf(format, args...))This ensures the adapter's Fatal method matches OCM SDK expectations for fatal error handling.
🤖 Prompt for AI Agents
In `@internal/maestro_client/ocm_logger_adapter.go` around lines 77 - 84, The
Fatal method on ocmLoggerAdapter currently calls a.log.Errorf and does not
terminate the process; replace that call with a.log.Fatal and pass a single
formatted message (use fmt.Sprintf(format, args...)) so the adapter's Fatal(ctx,
...) uses a.log.Fatal(ctx, formattedMsg) and triggers the underlying logger's
os.Exit behavior instead of just logging an error.
There was a problem hiding this comment.
Maestro failure will reflect in runtime error. I don't want maestro client failure cause my adapter exit unexpectedly.
There was a problem hiding this comment.
@xueli181114, understood! Your design decision makes sense—keeping the adapter resilient by treating Maestro client failures as runtime errors instead of fatal exits is appropriate for your use case. The current implementation is correct for your requirements.
✏️ Learnings added
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 37
File: internal/maestro_client/ocm_logger_adapter.go:77-84
Timestamp: 2026-02-04T06:51:47.082Z
Learning: In the hyperfleet-adapter repository, the ocmLoggerAdapter's Fatal method intentionally calls a.log.Errorf instead of a.log.Fatal to prevent Maestro client failures from causing the adapter process to exit unexpectedly. Maestro failures should be reflected as runtime errors, not fatal exits.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 13
File: internal/executor/post_action_executor.go:198-205
Timestamp: 2025-12-04T14:06:51.656Z
Learning: Logger refactor is tracked in HYPERFLEET-304 for the hyperfleet-adapter repository, which will address how CEL evaluation failures and similar errors are handled and logged.
xueli181114
force-pushed
the
HYPERFLEET-564
branch
from
83d04cb to
95cfa2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
30-171:⚠️ Potential issue | 🟡 MinorCorrect the vulnerability scanning approach for indirect dependency updates.
Verifying security posture for indirect dependency updates is appropriate given the volume of changes. However, the referenced tool (
sonatard/go-vulncheck) does not exist. Use the officialgolang.org/x/vuln/cmd/govulncheckinstead:
go install golang.org/x/vuln/cmd/govulncheck@latest && govulncheck ./...Spot checks of key packages (golang.org/x/crypto v0.47.0, google.golang.org/grpc v1.78.0, getsentry/sentry-go v0.20.0) show no active published vulnerabilities, though running the above scan on the complete dependency tree remains a best practice.
🤖 Fix all issues with AI agents
In `@configs/broker-configmap-pubsub-template.yaml`:
- Around line 131-135: The example deployment comments out the required
environment variable BROKER_TOPIC, causing copy-paste deployments to fail;
update the template so the BROKER_TOPIC env var block (the configMapKeyRef
referencing name: hyperfleet-broker-config key: BROKER_TOPIC) is uncommented in
the example, or alternatively change the top-level label that marks BROKER_TOPIC
as REQUIRED to OPTIONAL and document the optionality—ensure the configMapKeyRef
stanza for BROKER_TOPIC is present and consistent with the documentation.
In `@go.mod`:
- Line 12: The go.mod entry for the Maestro dependency
(github.com/openshift-online/maestro) is using a pseudo-version
(v0.0.0-20260202062555-48b47506a254); replace it with a stable tagged release
(e.g., vX.Y.Z) if one exists, or coordinate with the Maestro maintainers to
obtain a proper semver tag; if a tagged release is not available and you must
proceed, document the rationale in a comment and consider adding a go.mod
replace directive or using a forked module with a tag to provide stability —
update the go.mod entry for github.com/openshift-online/maestro accordingly and
run go mod tidy to ensure module graph consistency.
In `@internal/k8s_client/mock.go`:
- Around line 162-172: The switch over compareResult.Operation currently falls
through to silently returning nil, nil; add a default case that returns a clear
error instead of falling through. Update the switch in the function that calls
m.CreateResource/m.UpdateResource (referencing compareResult.Operation,
generation.OperationCreate, generation.OperationSkip,
generation.OperationUpdate, m.CreateResource, m.UpdateResource, existingObj) to
handle unexpected operation values by returning an error that includes the
unknown operation value so callers can detect and surface this bug.
In `@internal/maestro_client/operations_test.go`:
- Around line 84-93: The test case "negative generation" in operations_test.go
expects the wrong error text; ValidateGeneration in generation.go returns "must
be > 0" for non-positive generations. Update the test case's errorMsg from "must
be >= 0" to "must be > 0" and strengthen the assertion in the test (around the
error checks) to explicitly compare the returned error string to errorMsg so the
message mismatch will fail the test if it regresses; reference the "negative
generation" test entry and the ValidateGeneration function when making the
change.
🧹 Nitpick comments (7)
examples/maestro_client/1.manifestwork-prams-manifests.yaml (1)
18-18: Typo in the filename reference.The filename
manifestwork-prams-manifests.yamlappears to have a typo - should likely bemanifestwork-params-manifests.yamlto match the intended "parameters" meaning. This also affects the actual filename of this file.internal/generation/generation.go (1)
238-256: In-place sort mutates the caller's list.
GetLatestGenerationFromListusessort.Slicewhich sorts thelist.Itemsslice in-place. This is a side effect that may surprise callers who don't expect their input to be modified.Consider either:
- Documenting this behavior explicitly in the function comment
- Creating a copy of the slice before sorting
Option 1: Document the side effect
// GetLatestGenerationFromList returns the resource with the highest generation annotation from a list. // It sorts by generation annotation (descending) and uses metadata.name as a secondary sort key // for deterministic behavior when generations are equal. // Returns nil if the list is nil or empty. // +// Note: This function sorts the list.Items slice in-place, modifying the input list. +// // Useful for finding the most recent version of a resource when multiple versions exist. func GetLatestGenerationFromList(list *unstructured.UnstructuredList) *unstructured.Unstructured {Option 2: Sort a copy to avoid mutation
func GetLatestGenerationFromList(list *unstructured.UnstructuredList) *unstructured.Unstructured { if list == nil || len(list.Items) == 0 { return nil } + // Create a copy to avoid mutating the input + items := make([]unstructured.Unstructured, len(list.Items)) + copy(items, list.Items) + // Sort by generation annotation (descending) to return the one with the latest generation // Secondary sort by metadata.name for consistency when generations are equal - sort.Slice(list.Items, func(i, j int) bool { - genI := GetGenerationFromUnstructured(&list.Items[i]) - genJ := GetGenerationFromUnstructured(&list.Items[j]) + sort.Slice(items, func(i, j int) bool { + genI := GetGenerationFromUnstructured(&items[i]) + genJ := GetGenerationFromUnstructured(&items[j]) if genI != genJ { return genI > genJ // Descending order - latest generation first } // Fall back to metadata.name for deterministic ordering when generations are equal - return list.Items[i].GetName() < list.Items[j].GetName() + return items[i].GetName() < items[j].GetName() }) - return &list.Items[0] + return &items[0] }internal/maestro_client/operations_test.go (1)
14-112: Consider asserting on error message content.The
errorMsgfield is defined in the test struct but only used in the error output when an expected error is nil. Consider adding assertions to verify the error message actually contains the expected substring, which would catch mismatches like the one above.Example assertion
if tt.expectError { if err == nil { t.Errorf("expected error containing %q, got nil", tt.errorMsg) } + if tt.errorMsg != "" && !strings.Contains(err.Error(), tt.errorMsg) { + t.Errorf("expected error containing %q, got %q", tt.errorMsg, err.Error()) + } return }Note: This would require adding
"strings"to the imports.internal/k8s_client/mock.go (1)
175-190:ApplyResourcesdoesn't populate theOperationfield in results.The
ApplyResourceResultstruct has anOperationfield (per the relevant code snippets), but this mock implementation only setsResourceorError. Tests relying on operation tracking won't get accurate results.♻️ Proposed fix to track operations
func (m *MockK8sClient) ApplyResources(ctx context.Context, objs []*unstructured.Unstructured) ([]ApplyResourceResult, error) { results := make([]ApplyResourceResult, 0, len(objs)) for _, obj := range objs { - resource, err := m.ApplyResource(ctx, obj) - if err != nil { - results = append(results, ApplyResourceResult{Error: err}) - return results, err + // Determine operation for result tracking + gvk := obj.GroupVersionKind() + existingObj, getErr := m.GetResource(ctx, gvk, obj.GetNamespace(), obj.GetName()) + exists := getErr == nil + + newGen := generation.GetGenerationFromUnstructured(obj) + var existingGen int64 + if exists { + existingGen = generation.GetGenerationFromUnstructured(existingObj) } - results = append(results, ApplyResourceResult{Resource: resource}) + compareResult := generation.CompareGenerations(newGen, existingGen, exists) + + resource, err := m.ApplyResource(ctx, obj) + results = append(results, ApplyResourceResult{ + Resource: resource, + Operation: compareResult.Operation, + Error: err, + }) + if err != nil { + return results, err + } } return results, nil }test/integration/maestro_client/setup_test.go (2)
134-146: Consider extracting duplicated PostgreSQL IP extraction logic.The code for getting the PostgreSQL container IP from network settings (with
host.docker.internalfallback) is duplicated in bothrunMaestroMigrationandstartMaestroServer. Extracting this to a helper function would reduce duplication.♻️ Proposed helper function
+// getContainerIP extracts the IP address from a container's network settings +// Falls back to host.docker.internal for Docker Desktop compatibility +func getContainerIP(ctx context.Context, container testcontainers.Container) (string, error) { + inspect, err := container.Inspect(ctx) + if err != nil { + return "", fmt.Errorf("failed to inspect container: %w", err) + } + + for _, network := range inspect.NetworkSettings.Networks { + if network.IPAddress != "" { + return network.IPAddress, nil + } + } + + // Fallback to host.docker.internal for Docker Desktop + return "host.docker.internal", nil +}Then use it in both functions:
pgIP, err := getContainerIP(ctx, env.PostgresContainer) if err != nil { return nil, err }Also applies to: 205-215
182-192: Log reading may be truncated for debugging failures.The single
Readcall with a fixed 4096-byte buffer may not capture all migration logs on failure. Consider usingio.ReadAllfor complete log output when debugging.♻️ Proposed fix for complete log reading
if state.ExitCode != 0 { // Get logs for debugging logs, _ := container.Logs(ctx) if logs != nil { defer logs.Close() //nolint:errcheck - buf := make([]byte, 4096) - n, _ := logs.Read(buf) - println(fmt.Sprintf(" Migration logs: %s", string(buf[:n]))) + logBytes, _ := io.ReadAll(logs) + println(fmt.Sprintf(" Migration logs: %s", string(logBytes))) } return fmt.Errorf("migration failed with exit code %d", state.ExitCode) }test/integration/maestro_client/client_integration_test.go (1)
249-254: Unchecked type assertions could panic on unexpected structure.The chained type assertions on
configMapManifestcould panic if the map structure differs from expected. While safe here since you control the structure, using checked assertions would be more defensive.🛡️ Safer alternative using helper or checked assertions
// Now apply again with updated generation (should update) work.Annotations[constants.AnnotationGeneration] = "2" - configMapManifest["metadata"].(map[string]interface{})["annotations"].(map[string]interface{})[constants.AnnotationGeneration] = "2" - configMapManifest["data"].(map[string]interface{})["key2"] = "value2" + if meta, ok := configMapManifest["metadata"].(map[string]interface{}); ok { + if ann, ok := meta["annotations"].(map[string]interface{}); ok { + ann[constants.AnnotationGeneration] = "2" + } + } + if data, ok := configMapManifest["data"].(map[string]interface{}); ok { + data["key2"] = "value2" + } configMapJSON, _ = json.Marshal(configMapManifest)Or simply rebuild the manifest from scratch for clarity.
| # - name: BROKER_TOPIC | ||
| # valueFrom: | ||
| # configMapKeyRef: | ||
| # name: hyperfleet-broker-config | ||
| # key: BROKER_TOPIC |
There was a problem hiding this comment.
Example deployment omits a required env var.
BROKER_TOPIC is labeled REQUIRED above, but the example deployment comments it out, which can lead to a broken copy‑paste deployment. Consider uncommenting it or marking it optional above.
✅ Suggested fix (uncomment in example)
-# - name: BROKER_TOPIC
-# valueFrom:
-# configMapKeyRef:
-# name: hyperfleet-broker-config
-# key: BROKER_TOPIC
+ - name: BROKER_TOPIC
+ valueFrom:
+ configMapKeyRef:
+ name: hyperfleet-broker-config
+ key: BROKER_TOPIC📝 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.
| # - name: BROKER_TOPIC | |
| # valueFrom: | |
| # configMapKeyRef: | |
| # name: hyperfleet-broker-config | |
| # key: BROKER_TOPIC | |
| - name: BROKER_TOPIC | |
| valueFrom: | |
| configMapKeyRef: | |
| name: hyperfleet-broker-config | |
| key: BROKER_TOPIC |
🤖 Prompt for AI Agents
In `@configs/broker-configmap-pubsub-template.yaml` around lines 131 - 135, The
example deployment comments out the required environment variable BROKER_TOPIC,
causing copy-paste deployments to fail; update the template so the BROKER_TOPIC
env var block (the configMapKeyRef referencing name: hyperfleet-broker-config
key: BROKER_TOPIC) is uncommented in the example, or alternatively change the
top-level label that marks BROKER_TOPIC as REQUIRED to OPTIONAL and document the
optionality—ensure the configMapKeyRef stanza for BROKER_TOPIC is present and
consistent with the documentation.
examples/maestro_client/1.adapter-business-logic-with-manifests.yaml
Outdated
Show resolved
Hide resolved
examples/maestro_client/1.adapter-business-logic-with-manifests.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@configs/adapter-config-template.yaml`:
- Around line 36-37: The Kind rename is incomplete: update all occurrences of
the old AdapterConfig type/name to the new AdapterWorkflowConfig across code and
tests; specifically rename the Go struct type AdapterConfig in
internal/config_loader/types.go to AdapterWorkflowConfig (and update its
constructor/usage sites), update any JSON/YAML type/kind fields in testdata
files (e.g., adapter_config_valid.yaml and adapter-config-template.yaml) from
"AdapterConfig" to "AdapterWorkflowConfig", and update test code references in
internal/config_loader/loader_test.go and validator_test.go to use the new
struct/type name and Kind string so all symbol names (AdapterWorkflowConfig, any
constructors or methods previously named for AdapterConfig) match consistently.
In `@pkg/version/version.go`:
- Around line 26-33: The comment for UserAgent incorrectly references
"USER_AGENT" while the code checks EnvUserAgent (HYPERFLEET_USER_AGENT); update
the comment above func UserAgent() to mention HYPERFLEET_USER_AGENT (or
EnvUserAgent) so it matches the actual environment variable used, keeping the
rest of the description unchanged and referencing the UserAgent function and
EnvUserAgent constant to locate the fix.
In `@README.md`:
- Around line 155-167: Add a missing environment variable row for
HYPERFLEET_API_TOKEN to the Environment Variables table so the README documents
that API auth token; update the table near the existing entries like
ADAPTER_CONFIG_PATH and HYPERFLEET_API_BASE_URL to include
`HYPERFLEET_API_TOKEN` with a short description such as "API authentication
token for HyperFleet" and mark its default as "(required)" or provide the actual
default if applicable; ensure the new row follows the same column ordering and
formatting as the other rows (Variable | Description | Default).
In `@test/integration/config-loader/config_criteria_integration_test.go`:
- Around line 50-51: The test currently sets ctx.Set("clusterPhase", "Ready")
but the "Ready should fail" case now contradicts the updated allowed phases;
update the failing-preconditions test case so it uses a phase outside the
allowed list (for example change the test input that sets clusterPhase from
"Ready" to "Terminating") or alternatively adjust the expected outcome for the
"Ready should fail" case to expect success; locate the test case in
test/integration/config-loader/config_criteria_integration_test.go where
ctx.Set("clusterPhase", ...) and the "Ready should fail" assertion occur and
make the corresponding value or expectation change.
🧹 Nitpick comments (6)
internal/hyperfleet_api/client.go (1)
324-325: Don’t clobber an explicit User-Agent header.
Right now this overwrites any per-request or defaultUser-Agentthe caller set. Consider only setting it when absent.♻️ Suggested adjustment
-// Set User-Agent header -httpReq.Header.Set("User-Agent", version.UserAgent()) +// Set User-Agent header (respect explicit caller override) +if httpReq.Header.Get("User-Agent") == "" { + httpReq.Header.Set("User-Agent", version.UserAgent()) +}internal/maestro_client/operations_test.go (2)
15-117: Consider consolidating duplicate test coverage.This
TestValidateGenerationtest duplicates coverage already present ininternal/generation/generation_test.go(Lines 215-317). Both test the samegeneration.ValidateGenerationfunction with similar test cases. Consider whether this duplication is intentional (testing from consumer perspective) or if it should be consolidated.
119-193: Duplicate test coverage noted.This test also duplicates
TestValidateGenerationFromUnstructuredininternal/generation/generation_test.go(Lines 319-442). Same consolidation consideration applies.test/integration/maestro_client/client_integration_test.go (1)
164-236: Type assertions could panic if manifest structure changes.Lines 223-224 use chained type assertions that will panic if the manifest structure doesn't match expectations:
configMapManifest["metadata"].(map[string]interface{})["annotations"].(map[string]interface{})[...]While this is test code with controlled data, consider using safer access or adding a guard comment.
Safer alternative using helper or explicit checks
// Option 1: Use a helper function func setNestedAnnotation(manifest map[string]interface{}, key, value string) { metadata := manifest["metadata"].(map[string]interface{}) annotations := metadata["annotations"].(map[string]interface{}) annotations[key] = value } // Option 2: Add comment acknowledging the risk // Safe: manifest structure is defined above in this test configMapManifest["metadata"].(map[string]interface{})["annotations"].(map[string]interface{})[constants.AnnotationGeneration] = "2"test/integration/maestro_client/setup_test.go (1)
213-221: Consider increasing log buffer size for debugging.The 4096-byte buffer may truncate migration error logs, making debugging difficult. Consider reading the full output or using a larger buffer.
♻️ Proposed fix to read full logs
// Get logs for debugging logs, _ := container.Logs(ctx) if logs != nil { defer logs.Close() //nolint:errcheck - buf := make([]byte, 4096) - n, _ := logs.Read(buf) - println(fmt.Sprintf(" Migration logs: %s", string(buf[:n]))) + logBytes, _ := io.ReadAll(logs) + println(fmt.Sprintf(" Migration logs: %s", string(logBytes))) }Note: You'll need to add
"io"to the imports.internal/generation/generation.go (1)
235-259: Consider documenting or avoiding the in-place sort side effect.
sort.Slicemodifieslist.Itemsin-place. Callers may not expect their list to be reordered after calling this function. Consider either documenting this behavior or making a copy before sorting.♻️ Option 1: Document the side effect
// GetLatestGenerationFromList returns the resource with the highest generation annotation from a list. // It sorts by generation annotation (descending) and uses metadata.name as a secondary sort key // for deterministic behavior when generations are equal. // Returns nil if the list is nil or empty. // +// Note: This function sorts list.Items in-place. Callers should be aware that +// the order of items in the list will be modified. +// // Useful for finding the most recent version of a resource when multiple versions exist.♻️ Option 2: Avoid modifying input by copying
func GetLatestGenerationFromList(list *unstructured.UnstructuredList) *unstructured.Unstructured { if list == nil || len(list.Items) == 0 { return nil } - // Sort by generation annotation (descending) to return the one with the latest generation - // Secondary sort by metadata.name for consistency when generations are equal - sort.Slice(list.Items, func(i, j int) bool { + // Copy items to avoid modifying input + items := make([]unstructured.Unstructured, len(list.Items)) + copy(items, list.Items) + + // Sort by generation annotation (descending) + sort.Slice(items, func(i, j int) bool { - genI := GetGenerationFromUnstructured(&list.Items[i]) - genJ := GetGenerationFromUnstructured(&list.Items[j]) + genI := GetGenerationFromUnstructured(&items[i]) + genJ := GetGenerationFromUnstructured(&items[j]) if genI != genJ { return genI > genJ } - return list.Items[i].GetName() < list.Items[j].GetName() + return items[i].GetName() < items[j].GetName() }) - return &list.Items[0] + return &items[0] }
| ### Environment Variables | ||
|
|
||
| | Variable | Description | Default | | ||
| |----------|-------------|---------| | ||
| | `ADAPTER_CONFIG_PATH` | Path to adapter configuration file | `/etc/adapter/config/adapter-deployment-config.yaml` | | ||
| | `HYPERFLEET_USER_AGENT` | Custom User-Agent string for HTTP clients (Maestro, HyperFleet API) | `hyperfleet-adapter/{version}` | | ||
| | `HYPERFLEET_API_BASE_URL` | Base URL for HyperFleet API | (from config) | | ||
| | `HYPERFLEET_API_VERSION` | API version for HyperFleet API | (from config) | | ||
| | `BROKER_SUBSCRIPTION_ID` | Message broker subscription ID | (required) | | ||
| | `BROKER_TOPIC` | Message broker topic | (required) | | ||
| | `LOG_LEVEL` | Log level (debug, info, warn, error) | `info` | | ||
| | `LOG_FORMAT` | Log format (text, json) | `json` | | ||
| | `LOG_OUTPUT` | Log output (stdout, stderr) | `stdout` | |
There was a problem hiding this comment.
Document HYPERFLEET_API_TOKEN here if the template requires it.
Integration tests now set this variable; if it’s required for API auth, the env var table should include it (with default/required status).
📌 Suggested doc update
| `HYPERFLEET_API_VERSION` | API version for HyperFleet API | (from config) |
+| `HYPERFLEET_API_TOKEN` | API token for HyperFleet API authentication | (required) |📝 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.
| ### Environment Variables | |
| | Variable | Description | Default | | |
| |----------|-------------|---------| | |
| | `ADAPTER_CONFIG_PATH` | Path to adapter configuration file | `/etc/adapter/config/adapter-deployment-config.yaml` | | |
| | `HYPERFLEET_USER_AGENT` | Custom User-Agent string for HTTP clients (Maestro, HyperFleet API) | `hyperfleet-adapter/{version}` | | |
| | `HYPERFLEET_API_BASE_URL` | Base URL for HyperFleet API | (from config) | | |
| | `HYPERFLEET_API_VERSION` | API version for HyperFleet API | (from config) | | |
| | `BROKER_SUBSCRIPTION_ID` | Message broker subscription ID | (required) | | |
| | `BROKER_TOPIC` | Message broker topic | (required) | | |
| | `LOG_LEVEL` | Log level (debug, info, warn, error) | `info` | | |
| | `LOG_FORMAT` | Log format (text, json) | `json` | | |
| | `LOG_OUTPUT` | Log output (stdout, stderr) | `stdout` | | |
| ### Environment Variables | |
| | Variable | Description | Default | | |
| |----------|-------------|---------| | |
| | `ADAPTER_CONFIG_PATH` | Path to adapter configuration file | `/etc/adapter/config/adapter-deployment-config.yaml` | | |
| | `HYPERFLEET_USER_AGENT` | Custom User-Agent string for HTTP clients (Maestro, HyperFleet API) | `hyperfleet-adapter/{version}` | | |
| | `HYPERFLEET_API_BASE_URL` | Base URL for HyperFleet API | (from config) | | |
| | `HYPERFLEET_API_VERSION` | API version for HyperFleet API | (from config) | | |
| | `HYPERFLEET_API_TOKEN` | API token for HyperFleet API authentication | (required) | | |
| | `BROKER_SUBSCRIPTION_ID` | Message broker subscription ID | (required) | | |
| | `BROKER_TOPIC` | Message broker topic | (required) | | |
| | `LOG_LEVEL` | Log level (debug, info, warn, error) | `info` | | |
| | `LOG_FORMAT` | Log format (text, json) | `json` | | |
| | `LOG_OUTPUT` | Log output (stdout, stderr) | `stdout` | |
🤖 Prompt for AI Agents
In `@README.md` around lines 155 - 167, Add a missing environment variable row for
HYPERFLEET_API_TOKEN to the Environment Variables table so the README documents
that API auth token; update the table near the existing entries like
ADAPTER_CONFIG_PATH and HYPERFLEET_API_BASE_URL to include
`HYPERFLEET_API_TOKEN` with a short description such as "API authentication
token for HyperFleet" and mark its default as "(required)" or provide the actual
default if applicable; ensure the new row follows the same column ordering and
formatting as the other rows (Variable | Description | Default).
There was a problem hiding this comment.
@xueli181114, understood! I'll skip that suggestion.
🧠 Learnings used
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.
test/integration/config-loader/config_criteria_integration_test.go
Outdated
Show resolved
Hide resolved
xueli181114
force-pushed
the
HYPERFLEET-564
branch
from
802c513 to
1e51a34
Compare
|
/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 |
|
/retest |
|
Claude suggests adding in Would we like to configure this setting by some external config? |
|
New changes are detected. LGTM label has been removed. |
xueli181114
force-pushed
the
HYPERFLEET-564
branch
from
b346879 to
dd52320
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
1-171:⚠️ Potential issue | 🔴 CriticalResolve unresolved merge conflicts blocking build validation.
Multiple unresolved merge conflicts must be resolved before the go.mod changes can be validated. The following files contain conflict markers that prevent compilation:
test/integration/config-loader/loader_template_test.go: lines 46-50 and 100-106test/integration/config-loader/config_criteria_integration_test.go: lines 35-39 and 54-60Example conflict in loader_template_test.go (lines 46-50)
<<<<<<< HEAD configPath := filepath.Join(projectRoot, "configs/adapterconfig-template.yaml") ======= configPath := filepath.Join(projectRoot, "test/integration/config-loader/testdata/adapter-config-template.yaml") >>>>>>> 1e51a34 (fix: Moved version to a package version and fixed maestro integration running failure)Choose the correct path for each conflict location based on your intended file structure, then re-run validation to ensure the dependency changes are compatible and the module builds successfully.
🤖 Fix all issues with AI agents
In `@configs/adapter-deployment-config.yaml`:
- Around line 8-12: The header comment in configs/adapter-deployment-config.yaml
incorrectly states the file is packaged with the container image; update the
comment to clarify that this file is a sample/default and may not be included in
the image unless an explicit build step adds it, and that in production the
configuration should be provided via the HYPERFLEET_ADAPTER_DEPLOYMENT_CONFIG
environment variable (highest priority) or a ConfigMap mounted at
/etc/adapter/adapter-deployment-config.yaml; if this repository does include a
build step that packages the file, mention that build artifact (or Dockerfile
step) explicitly in the comment so the delivery mechanism is unambiguous.
In `@Dockerfile`:
- Around line 31-48: The Dockerfile contains unresolved Git merge conflict
markers (<<<<<<<, =======, >>>>>>>) around the adapter config comment block;
remove the conflict markers and retain the intended comment text about mounting
config via ConfigMap and setting ADAPTER_CONFIG_PATH, ensuring only one clean
commented block remains and no stray conflict lines exist so the Dockerfile is
valid.
In `@internal/maestro_client/ocm_logger_adapter.go`:
- Around line 77-83: The comment for ocmLoggerAdapter.Fatal is misleading about
exit behavior; update the doc for the Fatal method to state explicitly that this
adapter intentionally logs at error level (using a.log.Errorf in Fatal) and does
NOT call the underlying logger's exit behavior so the adapter process won't
terminate on Maestro client failures; reference the ocmLoggerAdapter type and
its Fatal method and replace the existing comment lines to clearly reflect this
intentional non-exit behavior.
In `@test/integration/config-loader/config_criteria_integration_test.go`:
- Around line 35-39: The file has unresolved Git conflict markers in
getConfigPath and TestConfigLoadAndCriteriaEvaluation; remove the markers and
choose the intended branch content (use the updated test path under
test/integration/config-loader/testdata — e.g., getConfigPath should return
filepath.Join(getProjectRoot(),
"test/integration/config-loader/testdata/adapter-config-template.yaml") and any
references in TestConfigLoadAndCriteriaEvaluation should use the same testdata
path), then ensure the code compiles by deleting the <<<<<<<, =======, and
>>>>>>> lines so the Go file contains only valid code.
- Around line 54-60: Resolve the merge conflicts by making the tests
consistently use the template's readyConditionStatus: update getConfigPath() to
return the test template (adapter-config-template.yaml) used by these
integration tests, replace any ctx.Set("clusterPhase", ...) with
ctx.Set("readyConditionStatus", "True") in config_criteria_integration_test.go,
and modify loader_template_test.go assertions to expect a condition named
"readyConditionStatus" with operator "equals" and value "True" (remove/replace
any assertions checking clusterPhase or a different operator/value).
In `@test/integration/config-loader/loader_template_test.go`:
- Around line 46-50: Remove the unresolved merge conflict markers and pick the
correct config path: eliminate the lines starting with <<<<<<<, =======, and
>>>>>>> so the assignment to configPath becomes a single filepath.Join call;
replace the conflicting path with the intended value (use configPath :=
filepath.Join(projectRoot,
"test/integration/config-loader/testdata/adapter-config-template.yaml") or the
projectRoot/ "configs/adapterconfig-template.yaml" variant as decided) and
ensure only one filepath.Join assignment to the configPath variable remains in
loader_template_test.go.
- Around line 93-95: The test currently expects the clusterName capture to
reference "metadata.name" (Kubernetes style) but the HyperFleet API uses a
root-level "name" field; update the assertion in the test that checks
clusterNameCapture.Field (located where findCaptureByName(firstPrecond.Capture,
"clusterName") and clusterNameCapture are used) to assert equality with "name"
instead of "metadata.name" so the precondition capture matches the HyperFleet
API response schema.
- Around line 100-106: Remove the git conflict markers and fix the two
assertions in loader_template_test.go so they assert the intended values for
firstCondition: change the conflicting lines to assert.Equal(t, "clusterPhase",
firstCondition.Field) and assert.Equal(t, "in", firstCondition.Operator), and
delete the <<<<<<<, =======, and >>>>>>> markers so the test compiles and
reflects the intended precondition checks.
🧹 Nitpick comments (6)
pkg/errors/error_test.go (1)
74-102: Cover new error codes explicitly inrequiredCodes.
expectedCountis now 17, butrequiredCodesstill omitsErrorMaestroErrorandErrorConfigurationError, so missing mappings could slip by. Consider adding them (and optionally extending the other tables) to keep coverage aligned.♻️ Proposed update
requiredCodes := []ServiceErrorCode{ ErrorNotFound, ErrorValidation, ErrorConflict, ErrorForbidden, ErrorUnauthorized, ErrorUnauthenticated, ErrorBadRequest, ErrorMalformedRequest, ErrorNotImplemented, ErrorGeneral, ErrorAdapterConfigNotFound, ErrorBrokerConnectionError, ErrorKubernetesError, ErrorHyperFleetAPIError, ErrorInvalidCloudEvent, + ErrorMaestroError, + ErrorConfigurationError, }internal/maestro_client/operations_test.go (1)
195-333: UseerrorMsgin ManifestWork validation assertions.The table includes
errorMsgbut the test doesn’t assert it, so message regressions won’t be caught.♻️ Suggested assertion
if tt.expectError { if err == nil { t.Errorf("expected error containing %q, got nil", tt.errorMsg) return } + if tt.errorMsg != "" && !strings.Contains(err.Error(), tt.errorMsg) { + t.Errorf("expected error containing %q, got %q", tt.errorMsg, err.Error()) + } return }test/integration/maestro_client/client_integration_test.go (2)
221-226: Marshal error silently discarded.Line 225 ignores the error from
json.Marshal. While unlikely to fail for a simple map, this could mask issues during test debugging.🔧 Proposed fix
- configMapJSON, _ = json.Marshal(configMapManifest) + configMapJSON, err = json.Marshal(configMapManifest) + require.NoError(t, err, "failed to marshal updated configmap")
286-289: Consider usingt.Fatalinstead oft.Skipfor first apply failure.If the first apply fails due to a consumer not being registered, the test skips rather than fails. However, per the test setup in
setup_test.go, consumers should be pre-registered. A failure here might indicate a real setup problem that should surface as a test failure.internal/generation/generation.go (1)
241-259: Sort modifies the input list in place.
sort.Slicesorts the slice in place, which mutateslist.Items. This could cause unexpected side effects if the caller expects the list to remain unchanged after callingGetLatestGenerationFromList.🔧 Proposed fix to avoid mutating input
func GetLatestGenerationFromList(list *unstructured.UnstructuredList) *unstructured.Unstructured { if list == nil || len(list.Items) == 0 { return nil } + // Make a copy to avoid mutating the original list + items := make([]unstructured.Unstructured, len(list.Items)) + copy(items, list.Items) + // Sort by generation annotation (descending) to return the one with the latest generation // Secondary sort by metadata.name for consistency when generations are equal - sort.Slice(list.Items, func(i, j int) bool { - genI := GetGenerationFromUnstructured(&list.Items[i]) - genJ := GetGenerationFromUnstructured(&list.Items[j]) + sort.Slice(items, func(i, j int) bool { + genI := GetGenerationFromUnstructured(&items[i]) + genJ := GetGenerationFromUnstructured(&items[j]) if genI != genJ { return genI > genJ // Descending order - latest generation first } // Fall back to metadata.name for deterministic ordering when generations are equal - return list.Items[i].GetName() < list.Items[j].GetName() + return items[i].GetName() < items[j].GetName() }) - return &list.Items[0] + return &items[0] }test/integration/maestro_client/setup_test.go (1)
170-186: Database password embedded in shell script.The password is embedded directly in the shell script passed to the entrypoint. While this is acceptable for local testing, be aware that:
- The password may appear in container logs if verbose logging is enabled
- Container inspection (
docker inspect) would reveal the entrypoint with credentialsFor integration tests with hardcoded test credentials, this is a minor concern but worth noting.
| // Fatal logs at error level with formatting. | ||
| // Note: Does not exit - the underlying logger handles that behavior. | ||
| func (a *ocmLoggerAdapter) Fatal(ctx context.Context, format string, args ...interface{}) { | ||
| if ctx == nil { | ||
| ctx = context.Background() | ||
| } | ||
| a.log.Errorf(ctx, "FATAL: "+format, args...) |
There was a problem hiding this comment.
Clarify the Fatal comment to reflect intentional non-exit.
The comment currently implies the underlying logger handles exiting, but this adapter intentionally avoids exit; the doc should match the intended behavior.
✏️ Suggested fix
-// Note: Does not exit - the underlying logger handles that behavior.
+// Note: intentionally does not exit to avoid terminating the adapter on Maestro client failures.Based on learnings: In the hyperfleet-adapter repository, the ocmLoggerAdapter's Fatal method intentionally calls a.log.Errorf instead of a.log.Fatal to prevent Maestro client failures from causing the adapter process to exit unexpectedly. Maestro failures should be reflected as runtime errors, not fatal exits.
🤖 Prompt for AI Agents
In `@internal/maestro_client/ocm_logger_adapter.go` around lines 77 - 83, The
comment for ocmLoggerAdapter.Fatal is misleading about exit behavior; update the
doc for the Fatal method to state explicitly that this adapter intentionally
logs at error level (using a.log.Errorf in Fatal) and does NOT call the
underlying logger's exit behavior so the adapter process won't terminate on
Maestro client failures; reference the ocmLoggerAdapter type and its Fatal
method and replace the existing comment lines to clearly reflect this
intentional non-exit behavior.
test/integration/config-loader/config_criteria_integration_test.go
Outdated
Show resolved
Hide resolved
test/integration/config-loader/config_criteria_integration_test.go
Outdated
Show resolved
Hide resolved
I fixed this lint issue. How about wait for customer feedback? I make Insecure also handle InsecureSkipVerify. But we may get requirements from service offering team. Till what I know ARO is using token for verify instead of TLS not sure about ROSA and GCP. |
xueli181114
force-pushed
the
HYPERFLEET-564
branch
from
dd52320 to
833cdc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@internal/maestro_client/client.go`:
- Around line 341-348: The readTokenFile function should fail fast when the file
is empty or contains only whitespace: after reading and trimming the token in
readTokenFile(path string), validate that the resulting string is non-empty and
return a descriptive error (e.g., "empty token file" or "token file contains
only whitespace") instead of returning an empty token; update callers to handle
that error as needed so authentication won't proceed with an empty token.
- Around line 101-109: Validate config.MaestroServerAddr to ensure it uses https
when the client is not allowed to be insecure: in the client initialization
(check around config.MaestroServerAddr validation) parse the URL and if
config.Insecure is false and the scheme is "http" (or missing/invalid but not
"https") return an apperrors.ConfigurationError explaining that
MaestroServerAddr must be an https URL unless Insecure is true; otherwise allow
http only when config.Insecure == true. Use the existing
config.MaestroServerAddr and config.Insecure fields to locate and enforce this
check.
- Around line 189-229: The current createHTTPTransport builds a bare
http.Transport and loses important defaults (ProxyFromEnvironment, DialContext,
ForceAttemptHTTP2, pooling and timeouts); instead, clone the default transport
and only override TLS settings: get a clone via
http.DefaultTransport.(*http.Transport).Clone(), set its TLSClientConfig to the
tls.Config you construct (respecting config.Insecure to set InsecureSkipVerify
when needed), and keep the code that loads HTTPCAFile/CAFile and appends to
tls.Config.RootCAs; return the cloned transport so ProxyFromEnvironment,
DialContext, MaxIdleConns, IdleConnTimeout, TLSHandshakeTimeout, etc. are
preserved.
833cdc7 to
f1fb19a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/integration/maestro_client/client_integration_test.go`:
- Around line 129-138: The test currently treats any error from
tc.Client.CreateManifestWork as acceptable; change it to only accept the
specific "consumer not registered" (or equivalent Maestro-specific) error and
fail for all other errors: after calling CreateManifestWork (created, err :=
tc.Client.CreateManifestWork(tc.Ctx, consumerName, work)), inspect err and if
nil continue as success, if err matches the Maestro consumer-not-registered
sentinel/type/message (e.g., check for a specific error type, error.Is(err,
maestro.ErrConsumerNotRegistered) or strings.Contains(err.Error(), "consumer not
registered") or an expected HTTP status), log that as the expected path and
return, otherwise call t.Fatalf or t.Fatalf-like assertion to fail the test with
the unexpected error; update the test code around the CreateManifestWork call
accordingly so only the known consumer-not-registered error is allowed.
🧹 Nitpick comments (2)
test/integration/maestro_client/setup_test.go (2)
24-26: Consider adding an overall timeout for test environment setup.The function creates a
context.Background()without a timeout. While individual container waits have timeouts, an overall timeout would prevent the test from hanging indefinitely if something goes wrong during setup.♻️ Suggested improvement
func setupMaestroTestEnv() (*MaestroTestEnv, error) { - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() env := &MaestroTestEnv{}
171-187: Consider extracting the common shell script setup to reduce duplication.The database secrets file creation logic is duplicated between
runMaestroMigration(lines 171-187) andstartMaestroServer(lines 235-257). Both generate identical/secrets/db.*files.♻️ Suggested helper function
// buildMaestroSetupScript creates a shell script that sets up database secrets // and executes the provided Maestro command with common DB flags. func buildMaestroSetupScript(pgIP, dbUser, dbPassword, dbName, maestroCmd string) string { return fmt.Sprintf(`#!/bin/sh mkdir -p /secrets echo -n '%s' > /secrets/db.host echo -n '5432' > /secrets/db.port echo -n '%s' > /secrets/db.user echo -n '%s' > /secrets/db.password echo -n '%s' > /secrets/db.name exec /usr/local/bin/maestro %s \ --db-host-file=/secrets/db.host \ --db-port-file=/secrets/db.port \ --db-user-file=/secrets/db.user \ --db-password-file=/secrets/db.password \ --db-name-file=/secrets/db.name \ --db-sslmode=disable \ --alsologtostderr \ -v=2 `, pgIP, dbUser, dbPassword, dbName, maestroCmd) }Then use it in both functions:
// In runMaestroMigration: setupScript := buildMaestroSetupScript(pgIP, dbUser, dbPassword, dbName, "migration") // In startMaestroServer: serverCmd := `server \ --server-hostname=0.0.0.0 \ --enable-grpc-server=true \ --grpc-server-bindport=8090 \ --http-server-bindport=8000 \ --health-check-server-bindport=8083 \ --message-broker-type=grpc` setupScript := buildMaestroSetupScript(pgIP, dbUser, dbPassword, dbName, serverCmd)Also applies to: 235-257
xueli181114
force-pushed
the
HYPERFLEET-564
branch
from
f1fb19a to
a7a498f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/maestro_client/client.go`:
- Around line 250-253: The gRPC branch currently sets
grpcOptions.Dialer.TLSConfig = nil when config.Insecure is true (forcing
plaintext), which mismatches the HTTP path's InsecureSkipVerify behavior; change
the gRPC handling in the client initialization (the block referencing
config.Insecure and grpcOptions.Dialer.TLSConfig) to create a tls.Config with
InsecureSkipVerify=true when Insecure is set (unless a CAFile is provided) so
self-signed certs work the same as HTTP, e.g. set grpcOptions.Dialer.TLSConfig =
&tls.Config{InsecureSkipVerify: true} (and continue to honor config.CAFile when
present), or alternatively update the docstring for config.Insecure to state
that for gRPC it currently enables plaintext only and CAFile must be used for
self-signed certs—implement the first option to preserve parity with the HTTP
path.
- Around line 106-116: The current validation allows schemeless or non-HTTP
schemes when config.Insecure is true; update the validation in the
MaestroServerAddr handling (around url.Parse, serverURL.Scheme and
config.Insecure) to always require serverURL.Scheme be either "http" or "https"
and return an apperrors.ConfigurationError if not, then additionally enforce
that serverURL.Scheme == "https" when config.Insecure == false (with a clear
error message referencing MaestroServerAddr and the received scheme). Ensure you
adjust the error messages returned from this block so they mention the invalid
scheme and the expected "http/https" values and still use
apperrors.ConfigurationError for consistency.
a7a498f to
49a6364
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/integration/config-loader/loader_template_test.go`:
- Around line 41-44: TestLoadValidTestConfig is missing the required
HYPERFLEET_API_TOKEN env var causing validation to fail; before calling
config_loader.Load() in the TestLoadValidTestConfig test, add
t.Setenv("HYPERFLEET_API_TOKEN", "test-token-for-integration-tests") alongside
the existing t.Setenv("HYPERFLEET_API_BASE_URL", ...) so the
env.HYPERFLEET_API_TOKEN required field is present during config_loader.Load().
🧹 Nitpick comments (3)
configs/adapter-deployment-config.yaml (2)
34-37: Consider annotating the sample version to avoid staleness.A short note helps future updates keep this aligned with releases.
📝 Suggested tweak
adapter: + # Update to match the released adapter version to avoid staleness in samples. version: "0.1.0"
95-105: Optional: quote scalar strings for consistency with the rest of the file.Purely stylistic, but it keeps duration/backoff values visually consistent.
📝 Suggested tweak
hyperfleetApi: - timeout: 2s + timeout: "2s" retryAttempts: 3 - retryBackoff: exponential + retryBackoff: "exponential"internal/maestro_client/operations_test.go (1)
125-169: Prefer exercising generation.CompareGenerations to avoid test drift.This test reimplements decision logic; using the helper keeps behavior aligned as logic evolves.
♻️ Proposed refactor
- // Logic from ApplyManifestWork: - // if existingGeneration == generation { return existing } - shouldSkipUpdate := tt.existingGeneration == tt.newGeneration - shouldUpdate := !shouldSkipUpdate + decision := generation.CompareGenerations(tt.newGeneration, tt.existingGeneration, true) + shouldUpdate := decision.Operation != generation.OperationSkip
4 participants
Summary by CodeRabbit
New Features
Documentation
Tests