HYPERFLEET-569 - feat: Implement maestro DSL loader #41
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 |
rh-amarin
force-pushed
the
resource-transport
branch
from
e904536 to
8a0c194
Compare
|
Caution Review failedFailed to post review comments WalkthroughThis PR introduces Maestro transport support as an alternative to Kubernetes for resource delivery. It adds new transport configuration types (TransportConfig, MaestroTransportConfig) to enable per-resource transport backend selection. The executor and resource executor are refactored to route resources between Kubernetes and Maestro delivery paths based on configuration. Maestro client integration is added to the adapter's main initialization flow. Configuration loader is updated to conditionally load ManifestWork references. Deprecated configuration templates and broker documentation are removed. Sequence Diagram(s)sequenceDiagram
participant Adapter as Adapter (main.go)
participant ConfigLoader as Config Loader
participant Executor as Executor
participant ResExecutor as Resource Executor
participant K8sAPI as Kubernetes API
participant MaestroAPI as Maestro API
Adapter->>ConfigLoader: Load adapter config & task config
ConfigLoader->>ConfigLoader: Parse transport config (Kubernetes/Maestro)
ConfigLoader->>ConfigLoader: Load ManifestWork references if Maestro
Adapter->>Adapter: Create Maestro client (if configured)
Adapter->>Executor: Initialize with K8s & Maestro clients
Executor->>ResExecutor: Execute resource
ResExecutor->>ResExecutor: Check transport client type
alt Client Type is Kubernetes
ResExecutor->>ResExecutor: applyResourceK8s()
ResExecutor->>K8sAPI: Apply manifest
K8sAPI-->>ResExecutor: Result
else Client Type is Maestro
ResExecutor->>ResExecutor: applyResourceMaestro()
ResExecutor->>ResExecutor: Build ManifestWork
ResExecutor->>ResExecutor: Apply ManifestWork settings
ResExecutor->>MaestroAPI: Apply ManifestWork
MaestroAPI-->>ResExecutor: Result
end
ResExecutor-->>Executor: Resource result
Executor-->>Adapter: Execution complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
190-196:⚠️ Potential issue | 🟡 MinorFix typo: “overriden” → “overridden.”
Minor doc polish.✏️ Proposed fix
- - Configure topic/subscription in the `adapter-config.yaml` but can be overriden with env variables or cli params + - Configure topic/subscription in the `adapter-config.yaml` but can be overridden with env variables or CLI params
🤖 Fix all issues with AI agents
In `@internal/executor/resource_executor.go`:
- Around line 356-366: The generation annotation is being lost because
work.SetAnnotations is called twice and the second call in
applyManifestWorkSettings overwrites all annotations; instead merge annotations:
when copying generation from manifest (using manifestAnnotations and
constants.AnnotationGeneration) read existing work.GetAnnotations(), create a
merged map that preserves existing keys (including any metadata.annotations from
RefContent), set/overwrite the generation key in that merged map, and then call
work.SetAnnotations once with the merged map; similarly, update
applyManifestWorkSettings to merge incoming metadata.annotations with
work.GetAnnotations() rather than replacing them so existing generation and
other annotations are preserved.
🧹 Nitpick comments (3)
internal/executor/types.go (1)
63-67: Add config-time validation for MaestroClient when maestro transport is used.While
applyResourceMaestroalready validates at runtime thatmaestroClientis not nil, validation at config initialization time would catch the issue earlier. Consider adding a check invalidateExecutorConfigto iterate over resources and requireMaestroClientwhen any resource usesTransportClientMaestro.internal/executor/resource_executor.go (2)
388-405: Labels frommetadata.labelsoverwrite rather than merge.On line 403,
work.SetLabels(labelMap)overwrites any existing labels. This is inconsistent with the root-level labels handling (lines 436-444) which merges with existing. Consider using the same merge pattern for consistency:♻️ Suggested fix for consistency
if labels, ok := metadata["labels"].(map[string]interface{}); ok { labelMap := make(map[string]string) for k, v := range labels { if str, ok := v.(string); ok { rendered, err := renderTemplate(str, params) if err != nil { return fmt.Errorf("failed to render label value for key %s: %w", k, err) } labelMap[k] = rendered } } - work.SetLabels(labelMap) + // Merge with existing labels + existing := work.GetLabels() + if existing == nil { + existing = make(map[string]string) + } + for k, v := range labelMap { + existing[k] = v + } + work.SetLabels(existing) re.log.Debugf(ctx, "Applied labels: %v", labelMap) }
297-299: Operation result is alwaysOperationCreatebut the actual operation is an upsert.The comment correctly states "ManifestWork apply is always an upsert", but the operation is recorded as
OperationCreate. This could be misleading in logs and metrics. Consider using a more accurate operation type or adding a new constant likeOperationApplyorOperationUpsert.
| // Copy the generation annotation from the manifest to the ManifestWork | ||
| // This is required by the maestro client for generation-based idempotency | ||
| manifestAnnotations := manifest.GetAnnotations() | ||
| if manifestAnnotations != nil { | ||
| if gen, ok := manifestAnnotations[constants.AnnotationGeneration]; ok { | ||
| work.SetAnnotations(map[string]string{ | ||
| constants.AnnotationGeneration: gen, | ||
| }) | ||
| re.log.Debugf(ctx, "Set ManifestWork generation annotation: %s", gen) | ||
| } | ||
| } |
There was a problem hiding this comment.
Generation annotation may be overwritten by RefContent settings.
The generation annotation is set on lines 361-364, but applyManifestWorkSettings calls work.SetAnnotations() (line 419) which overwrites all annotations. This could lose the generation annotation if metadata.annotations is present in RefContent.
Consider merging annotations instead of overwriting:
🐛 Proposed fix
// Apply annotations from metadata
if annotations, ok := metadata["annotations"].(map[string]interface{}); ok {
annotationMap := make(map[string]string)
for k, v := range annotations {
if str, ok := v.(string); ok {
rendered, err := renderTemplate(str, params)
if err != nil {
return fmt.Errorf("failed to render annotation value for key %s: %w", k, err)
}
annotationMap[k] = rendered
}
}
- work.SetAnnotations(annotationMap)
+ // Merge with existing annotations to preserve generation annotation
+ existing := work.GetAnnotations()
+ if existing == nil {
+ existing = make(map[string]string)
+ }
+ for k, v := range annotationMap {
+ existing[k] = v
+ }
+ work.SetAnnotations(existing)
re.log.Debugf(ctx, "Applied annotations: %v", annotationMap)
}Also applies to: 403-421
🤖 Prompt for AI Agents
In `@internal/executor/resource_executor.go` around lines 356 - 366, The
generation annotation is being lost because work.SetAnnotations is called twice
and the second call in applyManifestWorkSettings overwrites all annotations;
instead merge annotations: when copying generation from manifest (using
manifestAnnotations and constants.AnnotationGeneration) read existing
work.GetAnnotations(), create a merged map that preserves existing keys
(including any metadata.annotations from RefContent), set/overwrite the
generation key in that merged map, and then call work.SetAnnotations once with
the merged map; similarly, update applyManifestWorkSettings to merge incoming
metadata.annotations with work.GetAnnotations() rather than replacing them so
existing generation and other annotations are preserved.
| } | ||
|
|
||
| // Configure TLS if auth type is "tls" | ||
| if maestroConfig.Auth.Type == "tls" && maestroConfig.Auth.TLSConfig != nil { |
There was a problem hiding this comment.
This could lead to a confusing scenario where a user
configures auth.type: tls but forgets to provide tlsConfig — the client would silently
connect without mTLS, potentially exposing traffic.
Consider returning an explicit error when the configuration is inconsistent:
if maestroConfig.Auth.Type == "tls" {
if maestroConfig.Auth.TLSConfig == nil {
return nil, fmt.Errorf("maestro auth type is 'tls' but tlsConfig is not provided")
}
clientConfig.CAFile = maestroConfig.Auth.TLSConfig.CAFile
clientConfig.ClientCertFile = maestroConfig.Auth.TLSConfig.CertFile
clientConfig.ClientKeyFile = maestroConfig.Auth.TLSConfig.KeyFile
}
| } | ||
|
|
||
| // Error if env var is not set and no default provided | ||
| if envValue == "" && param.Default == nil { |
There was a problem hiding this comment.
Previously, a parameter like:
- name: myParam
source: "env.MY_REQUIRED_VAR"
required: true
would fail validation if MY_REQUIRED_VAR was not set and no default was provided. Now it
passes validation regardless of whether the env var exists.
Was this change intentional? If so, it would be good to document why the runtime env check was
removed (e.g., validation runs at build time, not in the target environment). If not, this is
a regression that could lead to silent failures at runtime when required env vars are
missing.
2 participants
Summary
kubernetes/maestroclient directlytransportconfiguration block supporting bothkubernetes(direct) andmaestro(ManifestWork-based) client typesKey Changes
Transport Layer
transport.clientfield:kubernetes(default) ormaestrotargetClusterandmanifestWork.namefieldsCleanup
/configsdirectory containing outdated templates and documentation/charts/examplesSummary by CodeRabbit
New Features
Documentation