feat: maestro client implemented in adapter framework #37
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 introduces Maestro transport support and enhances resource deployment capabilities. It adds a new generation-based resource tracking package for generation-aware create/update/skip decisions, implements a complete Maestro client with ManifestWork operations and TLS support, shifts adapter configuration from image-bundled defaults to ConfigMap-mounted deployment, and extends logging with Maestro consumer context awareness. Additionally, it updates dependencies including OCM APIs, refactors the Kubernetes client with generation-based apply operations, and adds comprehensive integration tests using Testcontainers for local Maestro environment simulation. Sequence Diagram(s)sequenceDiagram
participant Adapter as Adapter Framework
participant Gen as Generation Package
participant K8sClient as Kubernetes Client
participant K8sAPI as Kubernetes API
Adapter->>Gen: CompareGenerations(newGen, existingGen, exists)
Gen-->>Adapter: CompareResult (Create/Skip/Update)
alt Create Path
Adapter->>K8sClient: CreateResource(obj)
K8sClient->>K8sAPI: POST (create)
K8sAPI-->>K8sClient: Created resource
K8sClient-->>Adapter: Created resource
else Skip Path
Adapter->>Adapter: Log skip (generation match)
else Update Path
Adapter->>K8sClient: UpdateResource(obj)
K8sClient->>K8sAPI: PUT/PATCH (update)
K8sAPI-->>K8sClient: Updated resource
K8sClient-->>Adapter: Updated resource
end
sequenceDiagram
participant Adapter as Adapter Framework
participant MaestroClient as Maestro Client
participant WorkClient as ManifestWork Client (gRPC)
participant MaestroAPI as Maestro API (REST)
participant Cluster as Target Cluster
Adapter->>MaestroClient: ApplyManifestWork(consumerName, work)
MaestroClient->>MaestroAPI: GetManifestWork (check existence)
MaestroAPI-->>MaestroClient: Existing work or not found
alt Create Path
MaestroClient->>WorkClient: CreateManifestWork(work)
else Update Path
MaestroClient->>MaestroAPI: PatchManifestWork (update)
else Skip Path
MaestroClient-->>Adapter: Return existing (generation match)
end
WorkClient->>Cluster: Apply manifests via ManifestWork
Cluster-->>WorkClient: Manifests applied
MaestroAPI-->>MaestroClient: ManifestWork result
MaestroClient-->>Adapter: Applied ManifestWork
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ 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 }
| # ==== Behavior Flags ==== | ||
| # Default: false - infrastructure must exist (recommended for production) | ||
| # Set to true to automatically create topics/subscriptions if they don't exist | ||
| create_topic_if_missing: true | ||
| create_subscription_if_missing: true | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n configs/broker-configmap-pubsub-template.yaml | head -110 | tail -30Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1362
🏁 Script executed:
# Search for context around these flags in the file
rg -B 10 -A 5 "create_topic_if_missing|create_subscription_if_missing" configs/broker-configmap-pubsub-template.yamlRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1444
🏁 Script executed:
# Search for any documentation or comments about default behavior
rg -i "default.*false|recommended.*false" configs/ -A 2 -B 2Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 880
🏁 Script executed:
# Check if there are other broker config files that might show intended patterns
fd -e yaml -e yml . configs/ | grep -i broker | head -20Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 128
🌐 Web query:
hyperfleet-broker googlepubsub create_topic_if_missing create_subscription_if_missing default
💡 Result:
Web search failed: Server error: no LLM provider could handle the message
🏁 Script executed:
# Check for README or documentation that explains the template purpose
fd -e md -e txt . | xargs rg -l "template|demo|development" 2>/dev/null | head -10Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 358
🏁 Script executed:
# Look at the file header and context to understand its purpose
head -50 configs/broker-configmap-pubsub-template.yamlRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 2162
🏁 Script executed:
# Search for any guidance about production vs demo configurations
rg -i "production|demo|development|example" configs/ -B 2 -A 2 | head -50Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 3363
🏁 Script executed:
# Check if there's a non-template version of this config
fd "broker-configmap" configs/Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 128
🏁 Script executed:
# Check the configs README for guidance on template usage and defaults
cat configs/README.mdRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 4751
🏁 Script executed:
# Look for any other references to these flags or their intended defaults
rg "create_topic_if_missing|create_subscription_if_missing" --type yamlRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 400
🏁 Script executed:
# Check if the example file has different settings
cat configs/broker-configmap-pubsub-example.yamlRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 159
🏁 Script executed:
# List all files in configs directory
ls -la configs/Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 571
🏁 Script executed:
# Check the actual hyperfleet-broker library or source code for defaults
rg "create_topic_if_missing" --type go -B 3 -A 3 | head -40Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
# Look for any test configs or examples that show intended patterns
fd -type f \( -name "*.yaml" -o -name "*.yml" \) | xargs grep -l "create_topic_if_missing" 2>/dev/nullRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 255
🏁 Script executed:
# Check the templates subdirectory
ls -la configs/templates/Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 394
🏁 Script executed:
# Search for Go code that defines these flags
find . -name "*.go" -type f | xargs grep -l "create_topic_if_missing" 2>/dev/null | head -5Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
🏁 Script executed:
# Look for external broker library documentation or references
rg "create_topic_if_missing" --type-add 'go:*.go' -g '*.go' -B 3 -A 3 | head -50Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 65
Default auto-create flags contradict documented recommendation in the template.
The comment states "Default: false - infrastructure must exist (recommended for production)," but the template sets both create_topic_if_missing and create_subscription_if_missing to true. This creates confusion about whether the permissive settings are intentional for quick-start/demo usage or inadvertent. Either align the defaults to false for production safety, or add a clarifying comment explaining these are intentionally permissive for development/demo environments.
🔧 Proposed alignment
- create_topic_if_missing: true
- create_subscription_if_missing: true
+ create_topic_if_missing: false
+ create_subscription_if_missing: false📝 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.
| # ==== Behavior Flags ==== | |
| # Default: false - infrastructure must exist (recommended for production) | |
| # Set to true to automatically create topics/subscriptions if they don't exist | |
| create_topic_if_missing: true | |
| create_subscription_if_missing: true | |
| # ==== Behavior Flags ==== | |
| # Default: false - infrastructure must exist (recommended for production) | |
| # Set to true to automatically create topics/subscriptions if they don't exist | |
| create_topic_if_missing: false | |
| create_subscription_if_missing: false | |
🤖 Prompt for AI Agents
In `@configs/broker-configmap-pubsub-template.yaml` around lines 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.
| for _, obj := range objs { | ||
| resource, err := c.ApplyResource(ctx, obj) | ||
| if err != nil { | ||
| results = append(results, ApplyResourceResult{Error: err}) | ||
| return results, err | ||
| } | ||
| // Fall back to metadata.name for deterministic ordering when generations are equal | ||
| return list.Items[i].GetName() < list.Items[j].GetName() | ||
| }) | ||
| results = append(results, ApplyResourceResult{Resource: resource}) | ||
| } |
There was a problem hiding this comment.
ApplyResourceResult.Operation field is not populated.
The ApplyResourceResult struct has an Operation field (line 35), but it's never set in ApplyResources. This makes it impossible for callers to determine what action was taken for each resource.
🔧 Proposed fix to populate Operation
This requires ApplyResource to return the operation or modifying the approach:
+// applyResourceWithOperation is an internal helper that returns the operation performed
+func (c *Client) applyResourceWithOperation(ctx context.Context, obj *unstructured.Unstructured) (*unstructured.Unstructured, generation.Operation, error) {
+ // ... same logic as ApplyResource but return operation from compareResult
+}
func (c *Client) ApplyResources(ctx context.Context, objs []*unstructured.Unstructured) ([]ApplyResourceResult, error) {
// ...
for _, obj := range objs {
- resource, err := c.ApplyResource(ctx, obj)
+ resource, op, err := c.applyResourceWithOperation(ctx, obj)
if err != nil {
- results = append(results, ApplyResourceResult{Error: err})
+ results = append(results, ApplyResourceResult{Error: err, Operation: op})
return results, err
}
- results = append(results, ApplyResourceResult{Resource: resource})
+ results = append(results, ApplyResourceResult{Resource: resource, Operation: op})
}
// ...
}🤖 Prompt for AI Agents
In `@internal/k8s_client/client.go` around lines 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.
There was a problem hiding this comment.
@xueli181114, thanks for confirming! Let me know if you need any clarification on the suggested approach for populating the Operation field. Happy to help if you have questions while implementing the fix.
| // configureTLS sets up TLS configuration for the gRPC connection | ||
| func configureTLS(config *Config, grpcOptions *grpc.GRPCOptions) error { | ||
| // Insecure mode: plaintext connection (no TLS at all) | ||
| if config.Insecure { | ||
| grpcOptions.Dialer.TLSConfig = nil | ||
| return nil | ||
| } | ||
|
|
||
| // Option 1: Mutual TLS with certificates | ||
| if config.CAFile != "" && config.ClientCertFile != "" && config.ClientKeyFile != "" { | ||
| certConfig := cert.CertConfig{ | ||
| CAFile: config.CAFile, | ||
| ClientCertFile: config.ClientCertFile, | ||
| ClientKeyFile: config.ClientKeyFile, | ||
| } | ||
| if err := certConfig.EmbedCerts(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| tlsConfig, err := cert.AutoLoadTLSConfig( | ||
| certConfig, | ||
| func() (*cert.CertConfig, error) { | ||
| c := cert.CertConfig{ | ||
| CAFile: config.CAFile, | ||
| ClientCertFile: config.ClientCertFile, | ||
| ClientKeyFile: config.ClientKeyFile, | ||
| } | ||
| if err := c.EmbedCerts(); err != nil { | ||
| return nil, err | ||
| } | ||
| return &c, nil | ||
| }, | ||
| grpcOptions.Dialer, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| grpcOptions.Dialer.TLSConfig = tlsConfig | ||
| return nil | ||
| } | ||
|
|
||
| // Option 2: Token-based authentication with CA | ||
| if config.CAFile != "" && config.TokenFile != "" { | ||
| token, err := readTokenFile(config.TokenFile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| grpcOptions.Dialer.Token = token | ||
|
|
||
| certConfig := cert.CertConfig{ | ||
| CAFile: config.CAFile, | ||
| } | ||
| if err := certConfig.EmbedCerts(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| tlsConfig, err := cert.AutoLoadTLSConfig( | ||
| certConfig, | ||
| func() (*cert.CertConfig, error) { | ||
| c := cert.CertConfig{ | ||
| CAFile: config.CAFile, | ||
| } | ||
| if err := c.EmbedCerts(); err != nil { | ||
| return nil, err | ||
| } | ||
| return &c, nil | ||
| }, | ||
| grpcOptions.Dialer, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| grpcOptions.Dialer.TLSConfig = tlsConfig | ||
| return nil | ||
| } | ||
|
|
||
| // Option 3: CA only (server verification without client auth) | ||
| if config.CAFile != "" { | ||
| certConfig := cert.CertConfig{ | ||
| CAFile: config.CAFile, | ||
| } | ||
| if err := certConfig.EmbedCerts(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| tlsConfig, err := cert.AutoLoadTLSConfig( | ||
| certConfig, | ||
| func() (*cert.CertConfig, error) { | ||
| c := cert.CertConfig{ | ||
| CAFile: config.CAFile, | ||
| } | ||
| if err := c.EmbedCerts(); err != nil { | ||
| return nil, err | ||
| } | ||
| return &c, nil | ||
| }, | ||
| grpcOptions.Dialer, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| grpcOptions.Dialer.TLSConfig = tlsConfig | ||
| } | ||
|
|
||
| // No TLS configuration - will use insecure connection | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Potential unintended insecure fallthrough in configureTLS.
When Insecure is false but no CA/cert files are provided, the function falls through to line 334 and returns nil without configuring TLS. The comment says "No TLS configuration - will use insecure connection" but this may not be the intended behavior for production scenarios where the user expects secure connections.
Consider either:
- Returning an error when secure mode is expected but no TLS config is provided
- Using system root CAs as a fallback
🔧 Proposed fix to require TLS config in secure mode
// Option 3: CA only (server verification without client auth)
if config.CAFile != "" {
// ... existing code ...
}
- // No TLS configuration - will use insecure connection
+ // No TLS configuration provided in secure mode
+ // This is likely a configuration error - return error to make it explicit
+ // Alternatively, use system root CAs by leaving TLSConfig with default RootCAs
+ // For now, we'll use system default which provides server verification
+ grpcOptions.Dialer.TLSConfig = &tls.Config{
+ MinVersion: tls.VersionTLS12,
+ }
return nil
}Or to require explicit configuration:
- // No TLS configuration - will use insecure connection
- return nil
+ return fmt.Errorf("TLS configuration required: set CAFile for server verification, or set Insecure=true for plaintext connections")📝 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.
| // configureTLS sets up TLS configuration for the gRPC connection | |
| func configureTLS(config *Config, grpcOptions *grpc.GRPCOptions) error { | |
| // Insecure mode: plaintext connection (no TLS at all) | |
| if config.Insecure { | |
| grpcOptions.Dialer.TLSConfig = nil | |
| return nil | |
| } | |
| // Option 1: Mutual TLS with certificates | |
| if config.CAFile != "" && config.ClientCertFile != "" && config.ClientKeyFile != "" { | |
| certConfig := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| ClientCertFile: config.ClientCertFile, | |
| ClientKeyFile: config.ClientKeyFile, | |
| } | |
| if err := certConfig.EmbedCerts(); err != nil { | |
| return err | |
| } | |
| tlsConfig, err := cert.AutoLoadTLSConfig( | |
| certConfig, | |
| func() (*cert.CertConfig, error) { | |
| c := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| ClientCertFile: config.ClientCertFile, | |
| ClientKeyFile: config.ClientKeyFile, | |
| } | |
| if err := c.EmbedCerts(); err != nil { | |
| return nil, err | |
| } | |
| return &c, nil | |
| }, | |
| grpcOptions.Dialer, | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| grpcOptions.Dialer.TLSConfig = tlsConfig | |
| return nil | |
| } | |
| // Option 2: Token-based authentication with CA | |
| if config.CAFile != "" && config.TokenFile != "" { | |
| token, err := readTokenFile(config.TokenFile) | |
| if err != nil { | |
| return err | |
| } | |
| grpcOptions.Dialer.Token = token | |
| certConfig := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| } | |
| if err := certConfig.EmbedCerts(); err != nil { | |
| return err | |
| } | |
| tlsConfig, err := cert.AutoLoadTLSConfig( | |
| certConfig, | |
| func() (*cert.CertConfig, error) { | |
| c := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| } | |
| if err := c.EmbedCerts(); err != nil { | |
| return nil, err | |
| } | |
| return &c, nil | |
| }, | |
| grpcOptions.Dialer, | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| grpcOptions.Dialer.TLSConfig = tlsConfig | |
| return nil | |
| } | |
| // Option 3: CA only (server verification without client auth) | |
| if config.CAFile != "" { | |
| certConfig := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| } | |
| if err := certConfig.EmbedCerts(); err != nil { | |
| return err | |
| } | |
| tlsConfig, err := cert.AutoLoadTLSConfig( | |
| certConfig, | |
| func() (*cert.CertConfig, error) { | |
| c := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| } | |
| if err := c.EmbedCerts(); err != nil { | |
| return nil, err | |
| } | |
| return &c, nil | |
| }, | |
| grpcOptions.Dialer, | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| grpcOptions.Dialer.TLSConfig = tlsConfig | |
| } | |
| // No TLS configuration - will use insecure connection | |
| return nil | |
| } | |
| // Option 3: CA only (server verification without client auth) | |
| if config.CAFile != "" { | |
| certConfig := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| } | |
| if err := certConfig.EmbedCerts(); err != nil { | |
| return err | |
| } | |
| tlsConfig, err := cert.AutoLoadTLSConfig( | |
| certConfig, | |
| func() (*cert.CertConfig, error) { | |
| c := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| } | |
| if err := c.EmbedCerts(); err != nil { | |
| return nil, err | |
| } | |
| return &c, nil | |
| }, | |
| grpcOptions.Dialer, | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| grpcOptions.Dialer.TLSConfig = tlsConfig | |
| } | |
| // No TLS configuration provided in secure mode | |
| // This is likely a configuration error - return error to make it explicit | |
| // Alternatively, use system root CAs by leaving TLSConfig with default RootCAs | |
| // For now, we'll use system default which provides server verification | |
| grpcOptions.Dialer.TLSConfig = &tls.Config{ | |
| MinVersion: tls.VersionTLS12, | |
| } | |
| return nil | |
| } |
| // configureTLS sets up TLS configuration for the gRPC connection | |
| func configureTLS(config *Config, grpcOptions *grpc.GRPCOptions) error { | |
| // Insecure mode: plaintext connection (no TLS at all) | |
| if config.Insecure { | |
| grpcOptions.Dialer.TLSConfig = nil | |
| return nil | |
| } | |
| // Option 1: Mutual TLS with certificates | |
| if config.CAFile != "" && config.ClientCertFile != "" && config.ClientKeyFile != "" { | |
| certConfig := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| ClientCertFile: config.ClientCertFile, | |
| ClientKeyFile: config.ClientKeyFile, | |
| } | |
| if err := certConfig.EmbedCerts(); err != nil { | |
| return err | |
| } | |
| tlsConfig, err := cert.AutoLoadTLSConfig( | |
| certConfig, | |
| func() (*cert.CertConfig, error) { | |
| c := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| ClientCertFile: config.ClientCertFile, | |
| ClientKeyFile: config.ClientKeyFile, | |
| } | |
| if err := c.EmbedCerts(); err != nil { | |
| return nil, err | |
| } | |
| return &c, nil | |
| }, | |
| grpcOptions.Dialer, | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| grpcOptions.Dialer.TLSConfig = tlsConfig | |
| return nil | |
| } | |
| // Option 2: Token-based authentication with CA | |
| if config.CAFile != "" && config.TokenFile != "" { | |
| token, err := readTokenFile(config.TokenFile) | |
| if err != nil { | |
| return err | |
| } | |
| grpcOptions.Dialer.Token = token | |
| certConfig := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| } | |
| if err := certConfig.EmbedCerts(); err != nil { | |
| return err | |
| } | |
| tlsConfig, err := cert.AutoLoadTLSConfig( | |
| certConfig, | |
| func() (*cert.CertConfig, error) { | |
| c := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| } | |
| if err := c.EmbedCerts(); err != nil { | |
| return nil, err | |
| } | |
| return &c, nil | |
| }, | |
| grpcOptions.Dialer, | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| grpcOptions.Dialer.TLSConfig = tlsConfig | |
| return nil | |
| } | |
| // Option 3: CA only (server verification without client auth) | |
| if config.CAFile != "" { | |
| certConfig := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| } | |
| if err := certConfig.EmbedCerts(); err != nil { | |
| return err | |
| } | |
| tlsConfig, err := cert.AutoLoadTLSConfig( | |
| certConfig, | |
| func() (*cert.CertConfig, error) { | |
| c := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| } | |
| if err := c.EmbedCerts(); err != nil { | |
| return nil, err | |
| } | |
| return &c, nil | |
| }, | |
| grpcOptions.Dialer, | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| grpcOptions.Dialer.TLSConfig = tlsConfig | |
| } | |
| // No TLS configuration - will use insecure connection | |
| return nil | |
| } | |
| // Option 3: CA only (server verification without client auth) | |
| if config.CAFile != "" { | |
| certConfig := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| } | |
| if err := certConfig.EmbedCerts(); err != nil { | |
| return err | |
| } | |
| tlsConfig, err := cert.AutoLoadTLSConfig( | |
| certConfig, | |
| func() (*cert.CertConfig, error) { | |
| c := cert.CertConfig{ | |
| CAFile: config.CAFile, | |
| } | |
| if err := c.EmbedCerts(); err != nil { | |
| return nil, err | |
| } | |
| return &c, nil | |
| }, | |
| grpcOptions.Dialer, | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| grpcOptions.Dialer.TLSConfig = tlsConfig | |
| } | |
| return fmt.Errorf("TLS configuration required: set CAFile for server verification, or set Insecure=true for plaintext connections") | |
| } |
🤖 Prompt for AI Agents
In `@internal/maestro_client/client.go` around lines 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.
There was a problem hiding this comment.
good catch. Updating
There was a problem hiding this comment.
@xueli181114, sounds good! Either approach from the suggestion would work well—returning an error for explicit configuration or using system root CAs as a fallback. Looking forward to seeing the update! 👍
| // 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.
| // createManifestWorkPatch creates a JSON merge patch for updating a ManifestWork | ||
| func createManifestWorkPatch(work *workv1.ManifestWork) ([]byte, error) { | ||
| // Create patch with metadata (labels, annotations) and spec | ||
| patch := map[string]interface{}{ | ||
| "metadata": map[string]interface{}{ | ||
| "labels": work.Labels, | ||
| "annotations": work.Annotations, | ||
| }, | ||
| "spec": work.Spec, | ||
| } | ||
| return json.Marshal(patch) | ||
| } |
There was a problem hiding this comment.
Potential issue with nil labels/annotations in patch.
If work.Labels or work.Annotations are nil, the JSON patch will include "labels": null and "annotations": null, which with a merge patch will delete existing labels/annotations on the server. This may not be the intended behavior if the caller simply didn't set labels.
🔧 Proposed fix to handle nil maps
func createManifestWorkPatch(work *workv1.ManifestWork) ([]byte, error) {
- // Create patch with metadata (labels, annotations) and spec
- patch := map[string]interface{}{
- "metadata": map[string]interface{}{
- "labels": work.Labels,
- "annotations": work.Annotations,
- },
- "spec": work.Spec,
+ metadata := map[string]interface{}{}
+
+ // Only include labels if not nil to avoid clearing existing labels
+ if work.Labels != nil {
+ metadata["labels"] = work.Labels
+ }
+ // Only include annotations if not nil to avoid clearing existing annotations
+ if work.Annotations != nil {
+ metadata["annotations"] = work.Annotations
+ }
+
+ patch := map[string]interface{}{
+ "metadata": metadata,
+ "spec": work.Spec,
}
return json.Marshal(patch)
}📝 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.
| // createManifestWorkPatch creates a JSON merge patch for updating a ManifestWork | |
| func createManifestWorkPatch(work *workv1.ManifestWork) ([]byte, error) { | |
| // Create patch with metadata (labels, annotations) and spec | |
| patch := map[string]interface{}{ | |
| "metadata": map[string]interface{}{ | |
| "labels": work.Labels, | |
| "annotations": work.Annotations, | |
| }, | |
| "spec": work.Spec, | |
| } | |
| return json.Marshal(patch) | |
| } | |
| // createManifestWorkPatch creates a JSON merge patch for updating a ManifestWork | |
| func createManifestWorkPatch(work *workv1.ManifestWork) ([]byte, error) { | |
| metadata := map[string]interface{}{} | |
| // Only include labels if not nil to avoid clearing existing labels | |
| if work.Labels != nil { | |
| metadata["labels"] = work.Labels | |
| } | |
| // Only include annotations if not nil to avoid clearing existing annotations | |
| if work.Annotations != nil { | |
| metadata["annotations"] = work.Annotations | |
| } | |
| patch := map[string]interface{}{ | |
| "metadata": metadata, | |
| "spec": work.Spec, | |
| } | |
| return json.Marshal(patch) | |
| } |
🤖 Prompt for AI Agents
In `@internal/maestro_client/operations.go` around lines 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.
xueli181114
force-pushed
the
HYPERFLEET-564
branch
from
83d04cb to
95cfa2a
Compare
|
@xueli181114: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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.
| github.com/google/cel-go v0.26.1 | ||
| github.com/mitchellh/copystructure v1.2.0 | ||
| github.com/openshift-hyperfleet/hyperfleet-broker v1.0.1 | ||
| github.com/openshift-online/maestro v0.0.0-20260202062555-48b47506a254 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the go.mod file to confirm the dependency
head -30 go.modRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1053
🏁 Script executed:
# Search for the maestro dependency in go.mod
rg "maestro" go.modRepository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 154
🌐 Web query:
openshift-online/maestro releases github
💡 Result:
The GitHub repo openshift-online/maestro currently has no GitHub Releases published (“There aren’t any releases here”). [1]
It also appears to not use tagged versions (Go tooling shows “Tagged version” as unchecked, and versions are Go pseudo-versions like v0.0.0-...-18a68bb, with one published Feb 3, 2026). [2]
Sources: [1] [2]
Use a stable tagged release for the Maestro dependency instead of a pseudo-version.
The Maestro dependency is pinned to a pseudo-version (v0.0.0-20260202062555-48b47506a254) because the project has no published GitHub Releases or tagged semantic versions. This points to a commit from February 2, 2026, with no stability guarantees.
Pseudo-versions are unsuitable for production dependencies—they lack version semantics, make upgrades difficult to track, and provide no API stability guarantees. Verify with the Maestro maintainers whether a stable release will be published, or if using a pseudo-version is acceptable for this project's maturity level.
🤖 Prompt for AI Agents
In `@go.mod` at 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.
| 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 |
There was a problem hiding this comment.
Unexpected fallthrough returns nil, nil silently.
If compareResult.Operation has an unexpected value (not Create, Skip, or Update), the function silently returns nil, nil at line 172. This could mask bugs or unexpected states. Consider returning an error for unknown operations.
🛡️ Proposed fix to handle unexpected operations
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)
+ default:
+ return nil, fmt.Errorf("unexpected operation: %s", compareResult.Operation)
}
-
- return nil, 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.
| 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 | |
| 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) | |
| default: | |
| return nil, fmt.Errorf("unexpected operation: %s", compareResult.Operation) | |
| } |
🤖 Prompt for AI Agents
In `@internal/k8s_client/mock.go` around lines 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.
There was a problem hiding this comment.
@xueli181114, sounds good! Let me know if you have any questions about the suggested approach.
| { | ||
| name: "negative generation", | ||
| meta: metav1.ObjectMeta{ | ||
| Annotations: map[string]string{ | ||
| constants.AnnotationGeneration: "-5", | ||
| }, | ||
| }, | ||
| expectError: true, | ||
| errorMsg: "must be >= 0", | ||
| }, |
There was a problem hiding this comment.
Incorrect expected error message.
The test expects error message "must be >= 0" but ValidateGeneration in generation.go returns "must be > 0" since generation must be strictly positive. The test assertion at lines 100-104 doesn't actually verify the error message content, so this won't cause a test failure, but the comment is misleading.
Proposed fix
{
name: "negative generation",
meta: metav1.ObjectMeta{
Annotations: map[string]string{
constants.AnnotationGeneration: "-5",
},
},
expectError: true,
- errorMsg: "must be >= 0",
+ errorMsg: "must be > 0",
},🤖 Prompt for AI Agents
In `@internal/maestro_client/operations_test.go` around lines 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.
There was a problem hiding this comment.
I guess CodeRabbit is right here
There was a problem hiding this comment.
@rh-amarin, thank you for confirming! The test case's errorMsg field should indeed be updated to "must be > 0" to match the actual validation logic. Additionally, strengthening the assertion to explicitly check the error message would help catch this type of mismatch in the future.
| # - name: CONFIG_FILE | ||
| # value: /path/to/custom/config.yaml | ||
| # - name: ADAPTER_CONFIG_PATH | ||
| # value: /etc/adapter/config/adapter-config.yaml |
There was a problem hiding this comment.
The configuration path is inconsistent between files:
- README.md (line 150):
/etc/adapter/config/adapter-deployment-config.yaml - Dockerfile (line 34):
/etc/adapter/config/adapter-config.yaml
Since the old configs/adapter.yaml was deleted and replaced with
configs/adapter-deployment-config.yaml, the Dockerfile comment should be updated to match
| # value: /etc/adapter/config/adapter-config.yaml | |
| # value: /etc/adapter/config/adapter-deployment-config.yaml |
There was a problem hiding this comment.
What about having a default value for the configuration in production?
e.g. /etc/adapter/adapterconfig.yaml
This will make easier for everyone to debug issues if we always look in the same place
We could keep environment variables and cli-params to override the default
There was a problem hiding this comment.
Let's talk about the name convention on cross over meeting
| # | ||
| # This is the minimal default configuration packaged with the container image. | ||
| # In production, this should be overridden via: | ||
| # 1. CONFIG_FILE environment variable (highest priority) |
There was a problem hiding this comment.
There are three different environment variable names referenced for the config file path:
| File | Variable Name |
|---|---|
| README.md:148 | ADAPTER_CONFIG_FILE |
| configs/adapter-deployment-config.yaml:11 | CONFIG_FILE |
| Dockerfile:34 | ADAPTER_CONFIG_PATH |
Please standardize on a single environment variable name across all documentation and code.
Based on the existing codebase patterns, I'd suggest using ADAPTER_CONFIG_PATH consistently.
| # 1. CONFIG_FILE environment variable (highest priority) | |
| # 1. ADAPTER_CONFIG_PATH environment variable (highest priority) |
There was a problem hiding this comment.
... and if we follow the configuration standard it should be HYPERFLEET_ADAPTER_CONFIG_PATH don't you all love verbose environment names? 😄
| @@ -148,7 +148,7 @@ Tool versions are tracked in `.bingo/*.mod` files and loaded automatically via ` | |||
| The adapter supports multiple configuration sources with the following priority order: | |||
|
|
|||
| 1. **Environment Variable** (`ADAPTER_CONFIG_FILE`) - Highest priority | |||
There was a problem hiding this comment.
There are three different environment variable names referenced for the config file path:
| File | Variable Name |
|---|---|
| README.md:148 | ADAPTER_CONFIG_FILE |
| configs/adapter-deployment-config.yaml:11 | CONFIG_FILE |
| Dockerfile:34 | ADAPTER_CONFIG_PATH |
Please standardize on a single environment variable name across all documentation and code.
Based on the existing codebase patterns, I'd suggest using ADAPTER_CONFIG_PATH consistently
| 1. **Environment Variable** (`ADAPTER_CONFIG_FILE`) - Highest priority | |
| 1. **Environment Variable** (`ADAPTER_CONFIG_PATH`) - Highest priority |
There was a problem hiding this comment.
I will use HYPERFLEET_ADAPTER_DEPLOYMENT_CONFIG
| return nil, apperrors.MaestroError("work for manifestwork cannot be nil") | ||
| } | ||
|
|
||
| // Validate that generation annotations are present (set by template) |
There was a problem hiding this comment.
According to HyperFleet architecture docs (adapter-frame-design.md), both the ManifestWork
metadata AND each manifest within the workload should have hyperfleet.io/generation
annotations for proper traceability on remote clusters.
Currently, ValidateManifestWorkGeneration only validates the ManifestWork's own metadata,
not the individual manifests inside spec.workload.manifests.
Consider adding validation (or at least a warning log) for manifests without generation
annotations:
// After validating ManifestWork generation, optionally warn about manifests without generation
for i, manifest := range work.Spec.Workload.Manifests {
obj := &unstructured.Unstructured{}
if err := json.Unmarshal(manifest.Raw, obj); err != nil {
continue
}
if generation.GetGenerationFromUnstructured(obj) == 0 {
c.log.Warn(ctx, "Manifest %d in ManifestWork %s has no generation annotation", i,
work.Name)
}
}This ensures full generation tracking as per the architecture patterns documented in
hyperfleet/components/adapter/framework/adapter-frame-design.md.
| ref: "./manifestwork-prams-manifests.yaml" | ||
|
|
||
| # Multiple manifests defined here (injected into ManifestWork template) | ||
| manifests: |
There was a problem hiding this comment.
What would be the reason to define the manifests in this file instead of defining it in the referenced file?
In this file we define a namespace, but the manifestConfigs is in the reference one... far away from this definition
IMO this is an example of how to organize code with Helm, or is this some best practice that you got from maestro docs/exploration?
Why is this even a concern for me?
For someone taking a look at the examples... it may be hard to spot that the difference is just embedding or splitting the manifestwork definition.
There was a problem hiding this comment.
I will remove all examples to avoid confusion. And this can leave to the charts organization part.
| # ManifestWork template that receives manifests from business logic | ||
| # The template uses: {{ .resources.clusterSetup.manifests | toJson }} | ||
| manifestWork: | ||
| ref: "./manifestwork-prams-manifests.yaml" |
There was a problem hiding this comment.
I'm curious, why is it have prams in the name?
manifestwork-prams-manifests.yaml
| // Step 2: Check for existing resource using discovery | ||
| // Step 2: Use simple ApplyResource when no discovery or recreateOnChange | ||
| // This is the common case - apply by name with generation comparison | ||
| if resource.Discovery == nil && !resource.RecreateOnChange { |
There was a problem hiding this comment.
I find the code here could be simplified.
This function executeResource contains a lot of code for the case where resource.Discovery == nil but the else branch delegates to another function
- For me, it will make more sense if it delegates the work to other functions, so the responsibility of the
executeResourceis more clear, delegates work - But, I also wonder if the same behavior of the
resource.Discovery==nilis accomplished already with theexecuteResourceWithDiscoveryfunction- Imagine
Discovery==nil, thenexistingResource=niland the operation ends up being "create" - It is the same effect as if Discovery exists but finds nothing
- Imagine
| compareResult := generation.CompareGenerations(manifestGen, existingGen, existingResource != nil) | ||
|
|
||
| // Map manifest package operations to executor operations | ||
| switch compareResult.Operation { |
There was a problem hiding this comment.
This switch block and the following are a bit strange, could both be merged?
Could we reuse the generation.CompareResult returned bygeneration.CompareGenerations instead of translating to ResourceOperations ?
In fact, CompareResult is a bit of a misleading name IMO, we are really returning a decision, not a quantifier like "bigger than, equals..." we are returning "create, update" which are already actions
| expected: 0, | ||
| }, | ||
| { | ||
| name: "with other annotations", |
There was a problem hiding this comment.
Should we have a test for when there are "other annotations" besides the AnnotationGeneration one?
I mean
{
name: "with other annotations",
meta: metav1.ObjectMeta{
Annotations: map[string]string{
"other": "value",
constants.AnnotationGeneration: "5",
},
},
expected: 5,
},
| // The ManifestWork object should be pre-constructed from a template with: | ||
| // - hyperfleet.io/generation annotation on ManifestWork metadata | ||
| // - hyperfleet.io/generation annotation on each manifest within the workload |
There was a problem hiding this comment.
Since our API resources manage a single spec.generation ... is it possible for the ManifestWork and manifest to have different hyperfleet.io/generation values?
There was a problem hiding this comment.
There is no possible to have manifestwork and manifest with different generation value.
| { | ||
| name: "negative generation", | ||
| meta: metav1.ObjectMeta{ | ||
| Annotations: map[string]string{ | ||
| constants.AnnotationGeneration: "-5", | ||
| }, | ||
| }, | ||
| expectError: true, | ||
| errorMsg: "must be >= 0", | ||
| }, |
There was a problem hiding this comment.
I guess CodeRabbit is right here
| } | ||
|
|
||
| // BuildManifestWorkName generates a consistent ManifestWork name for testing | ||
| // Format: <adapter-name>-<resource-name>-<cluster-id> |
There was a problem hiding this comment.
Do we have to test for limits on the manifestWork name length?
| env := GetSharedEnv(t) | ||
|
|
||
| log, err := logger.NewLogger(logger.Config{ | ||
| Level: "debug", | ||
| Format: "text", | ||
| Component: "maestro-integration-test", | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| defer cancel() | ||
|
|
||
| config := &maestro_client.Config{ | ||
| MaestroServerAddr: env.MaestroServerAddr, | ||
| GRPCServerAddr: env.MaestroGRPCAddr, | ||
| SourceID: "integration-test-source", | ||
| Insecure: true, | ||
| } | ||
|
|
||
| client, err := maestro_client.NewMaestroClient(ctx, config, log) | ||
| require.NoError(t, err, "Should create Maestro client successfully") | ||
| defer client.Close() //nolint:errcheck | ||
|
|
||
| assert.NotNil(t, client.WorkClient(), "WorkClient should not be nil") | ||
| assert.Equal(t, "integration-test-source", client.SourceID()) |
There was a problem hiding this comment.
Could we extract this to a function and reduce the file size by half?
Every test is repeating this lines only to create a connection
Is not because of the size per se, but for every test... we have to read it to find if there is any difference with the previous function .... and is tiresome (at least for me)
| @@ -0,0 +1,277 @@ | |||
| package maestro_client_integration | |||
There was a problem hiding this comment.
I got errors when running once the integration tests, but says ok at the end
2026/02/03 17:35:02 ⏳ Waiting for container id eeae5334ebb6 image: quay.io/redhat-user-workloads/maestro-rhtap-tenant/maestro/maestro:latest. Waiting for: container to exit
2026/02/03 17:35:02 🔔 Container is ready: eeae5334ebb6
Migration logs: Error: unknown flag: --db-host
2026/02/03 17:35:02 🐳 Stopping container: eeae5334ebb6
2026/02/03 17:35:02 ✅ Container stopped: eeae5334ebb6
2026/02/03 17:35:02 🐳 Terminating container: eeae5334ebb6
2026/02/03 17:35:02 🚫 Container terminated: eeae5334ebb6
2026/02/03 17:35:02 🐳 Stopping container: b2cdb9fb9f77
2026/02/03 17:35:02 ✅ Container stopped: b2cdb9fb9f77
2026/02/03 17:35:02 🐳 Terminating container: b2cdb9fb9f77
2026/02/03 17:35:02 🚫 Container terminated: b2cdb9fb9f77
❌ Failed to set up Maestro environment: failed to run Maestro migration: migration failed with exit code 1
Tests will be skipped
=== RUN TestMaestroClientConnection
client_integration_test.go:21: Maestro environment setup failed: failed to run Maestro migration: migration failed with exit code 1
--- SKIP: TestMaestroClientConnection (0.00s)
=== RUN TestMaestroClientCreateManifestWork
client_integration_test.go:50: Maestro environment setup failed: failed to run Maestro migration: migration failed with exit code 1
--- SKIP: TestMaestroClientCreateManifestWork (0.00s)
=== RUN TestMaestroClientListManifestWorks
client_integration_test.go:135: Maestro environment setup failed: failed to run Maestro migration: migration failed with exit code 1
--- SKIP: TestMaestroClientListManifestWorks (0.00s)
=== RUN TestMaestroClientApplyManifestWork
client_integration_test.go:174: Maestro environment setup failed: failed to run Maestro migration: migration failed with exit code 1
--- SKIP: TestMaestroClientApplyManifestWork (0.00s)
=== RUN TestMaestroClientGenerationSkip
client_integration_test.go:268: Maestro environment setup failed: failed to run Maestro migration: migration failed with exit code 1
--- SKIP: TestMaestroClientGenerationSkip (0.00s)
PASS
ok github.com/openshift-hyperfleet/hyperfleet-adapter/test/integration/maestro_client 39.315s
? github.com/openshift-hyperfleet/hyperfleet-adapter/test/integration/testutil [no test files]
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
✅ Integration tests passed!
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
There was a problem hiding this comment.
As a side note... being able to test with a real maestro instance in testcontainers still amazes me 😄
3 participants
Summary by CodeRabbit
New Features
Configuration
Documentation
Improvements