HYPERFLEET-864 - feat: support Go template lists and conditionals in manifest resources#96
Open
xueli181114 wants to merge 2 commits intoopenshift-hyperfleet:mainfrom
Open
HYPERFLEET-864 - feat: support Go template lists and conditionals in manifest resources#96xueli181114 wants to merge 2 commits intoopenshift-hyperfleet:mainfrom
xueli181114 wants to merge 2 commits intoopenshift-hyperfleet:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReferenced manifests ( Sequence DiagramsequenceDiagram
participant ConfigLoader as Config Loader
participant FS as File System
participant Validator as Validator
participant Executor as Executor
participant TemplateEngine as Template Engine
participant YAML as YAML Parser
ConfigLoader->>FS: safe-join path, os.ReadFile (loadRawFile)
FS-->>ConfigLoader: raw file content (string with templates)
ConfigLoader->>ConfigLoader: set Resource.Manifest = raw string
Validator->>ConfigLoader: inspect Resource.Manifest
alt Manifest is string
Validator->>Validator: validateTemplateString (skip K8s structural checks)
else Manifest is map
Validator->>Validator: validateTemplateMap + K8s structure/semantic checks
end
Executor->>ConfigLoader: get Resource.Manifest
alt Manifest is string
Executor->>TemplateEngine: render template -> rendered YAML string
TemplateEngine-->>Executor: rendered YAML
Executor->>YAML: parse rendered YAML -> map[string]interface{}
YAML-->>Executor: parsed map
Executor->>Executor: json.Marshal(parsed map) -> bytes
else Manifest is map
Executor->>Executor: render templates recursively in map (manifest.RenderManifestData)
Executor->>Executor: json.Marshal(map) -> bytes
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/configloader/validator.go`:
- Around line 562-567: The current check skips all validation for string
manifests (resource.Manifest as string), which bypasses template-variable
validation; update the branch so that before continuing you run
validateTemplateVariables(...) on the raw string manifest (using the same
context/params used for map manifests), surface/return any validation errors
from validateTemplateVariables, but still skip K8s shape/schema validation for
strings; reference resource.Manifest and validateTemplateVariables() when making
the change and preserve the existing control-flow that continues after template
validation.
In `@internal/executor/resource_executor.go`:
- Around line 225-227: String-backed manifests are rendered in
renderStringManifest but discoverResource still calls resolveGVK(resource) which
returns empty GVK for non-map manifests; change the flow so renderStringManifest
(or the rendering step) computes the actual GVK from the rendered object (call
it renderedGVK) and pass that renderedGVK into discoverResource instead of
letting discoverResource recompute from resource.Manifest; update
discoverResource's signature/usage to accept renderedGVK and use it in place of
resolveGVK(resource) (also apply the same adjustment for the other occurrence
around lines 258-283) so discovery uses the real GVK derived from the rendered
object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83424ad6-6c90-4d54-8041-c21c4c368c81
📒 Files selected for processing (5)
internal/configloader/loader.gointernal/configloader/loader_test.gointernal/configloader/validator.gointernal/executor/resource_executor.gointernal/executor/resource_executor_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/configloader/validator_test.go`:
- Around line 374-386: The test currently assumes any string Resource.Manifest
skips K8s validation; either update the validator to only bypass validation when
the manifest contains template delimiters (e.g., detect "{{" / "}}" in
Resource.Manifest in newTaskValidator/ValidateStructure) or change the test to
expect a validation error for a non-templated YAML string; specifically, narrow
the bypass logic so Resource.Manifest without template delimiters is
parsed/validated (or explicitly detect templated strings before bypassing) and
then update this test that calls newTaskValidator(cfg) and asserts
ValidateStructure()/ValidateSemantic() accordingly.
In `@internal/executor/resource_executor_test.go`:
- Around line 615-628: The test currently accepts an empty manifest producing
JSON "null", but renderToBytes should fail fast for empty string manifests;
update the implementation of re.renderToBytes (the renderer function used in the
test) to detect resource.Manifest == "" (or YAML that parses to a nil/empty
document) and return a clear render/parse error instead of returning "null",
then update the test expectation to require an error (use
require.Error/require.Contains on the returned error) rather than asserting
"null". Ensure the check is applied early in re.renderToBytes so callers like
apply/discovery never receive a null manifest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1f623e9-2179-4f48-8dce-d79e60950487
📒 Files selected for processing (3)
internal/configloader/loader_test.gointernal/configloader/validator_test.gointernal/executor/resource_executor_test.go
3 tasks
xueli181114
force-pushed
the
HYPERFLEET-864
branch
2 times, most recently
from
a326fc9 to
568ed42
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/executor/resource_executor.go (2)
263-280:⚠️ Potential issue | 🟠 MajorReject manifests that render to an empty document.
The guard at Line 263 only catches an empty source string. A template like
{{ if .enabled }}...{{ end }}withenabled=falserenders to empty output,yaml.UnmarshalleavesmanifestDatanil, andjson.Marshal(nil)produces"null"again.Suggested fix
rendered, err := utils.RenderTemplate(manifestStr, params) if err != nil { return nil, fmt.Errorf("failed to render manifest template: %w", err) } + if strings.TrimSpace(rendered) == "" { + return nil, fmt.Errorf("empty manifest: template rendered to an empty document") + } // Parse the rendered string as YAML var manifestData map[string]interface{} if unmarshalErr := yaml.Unmarshal([]byte(rendered), &manifestData); unmarshalErr != nil { return nil, fmt.Errorf("failed to parse rendered manifest as YAML: %w", unmarshalErr) } + if len(manifestData) == 0 { + return nil, fmt.Errorf("empty manifest: rendered YAML did not contain an object") + } // Marshal to JSON bytes data, err := json.Marshal(manifestData)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/executor/resource_executor.go` around lines 263 - 280, The rendered template output (from utils.RenderTemplate called on manifestStr) can be empty even when the source manifestStr is non-empty, which leads to yaml.Unmarshal leaving manifestData nil and json.Marshal producing "null"; update the code after rendering to check that the rendered string is not empty or only whitespace (e.g., trim and error if empty) and also verify manifestData is non-nil after yaml.Unmarshal (return an error like "rendered manifest is empty" if manifestData == nil) before calling json.Marshal; modify the logic around rendered, manifestData, and json.Marshal to return a clear error when the rendered document is empty.
82-95:⚠️ Potential issue | 🟠 MajorUse the rendered object's GVK for discovery.
discoverResource()still resolves GVK fromresource.Manifest, soapiVersion/kindtemplates in either map or string manifests can apply successfully and then fail the post-apply lookup. Lines 89-95 already decoderenderedBytesintoobj, so pass that GVK forward instead of rescanning the source manifest.Suggested fix
func (re *ResourceExecutor) executeResource( ctx context.Context, resource configloader.Resource, execCtx *ExecutionContext, ) (ResourceResult, error) { @@ var obj unstructured.Unstructured + var renderedGVK schema.GroupVersionKind if unmarshalErr := json.Unmarshal(renderedBytes, &obj.Object); unmarshalErr == nil { result.Kind = obj.GetKind() result.Namespace = obj.GetNamespace() result.ResourceName = obj.GetName() + if gv, gvErr := schema.ParseGroupVersion(obj.GetAPIVersion()); gvErr == nil && obj.GetKind() != "" { + renderedGVK = gv.WithKind(obj.GetKind()) + } } @@ - discovered, discoverErr := re.discoverResource(ctx, resource, execCtx, transportTarget) + discovered, discoverErr := re.discoverResource(ctx, resource, execCtx, transportTarget, renderedGVK)func (re *ResourceExecutor) discoverResource( ctx context.Context, resource configloader.Resource, execCtx *ExecutionContext, transportTarget transportclient.TransportContext, + renderedGVK schema.GroupVersionKind, ) (*unstructured.Unstructured, error) { @@ - gvk := re.resolveGVK(resource) + gvk := renderedGVK + if gvk.Empty() { + gvk = re.resolveGVK(resource) + } + if gvk.Empty() { + return nil, fmt.Errorf("failed to resolve GVK for resource %s", resource.Name) + } return re.client.GetResource(ctx, gvk, namespace, name, transportTarget) } @@ - gvk := re.resolveGVK(resource) + gvk := renderedGVK + if gvk.Empty() { + gvk = re.resolveGVK(resource) + } + if gvk.Empty() { + return nil, fmt.Errorf("failed to resolve GVK for resource %s", resource.Name) + } list, err := re.client.DiscoverResources(ctx, gvk, discoveryConfig, transportTarget)Also applies to: 141-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/executor/resource_executor.go` around lines 82 - 95, The discovery step currently calls discoverResource() using the original resource.Manifest which can mismatch after templating; use the decoded rendered object GVK instead: after json.Unmarshal(renderedBytes, &obj.Object) (the block that sets result.Kind/Namespace/ResourceName) pass obj.GetAPIVersion()/obj.GetKind() (or the unstructured.Unstructured obj itself) into discoverResource()/the discovery call so discovery uses the rendered manifest's GroupVersionKind; repeat the same change for the other occurrence where discoverResource is invoked later (the block around the second render/lookup).
🧹 Nitpick comments (2)
internal/configloader/loader_test.go (2)
1332-1339: Use the nested maestromanifestWork.refshape here.
TestTransportConfigYAMLParsingmodels maestro manifests undertransport.maestro.manifestWork, but this loader test still feeds the file throughresources[].manifest.ref. That means these new raw-string assertions would stay green even if the actual maestro ref-loading path regressed. Please switch this fixture or add a companion case withtransport.maestro.manifestWork.ref.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/configloader/loader_test.go` around lines 1332 - 1339, The test is asserting raw-string loading via resources[].Manifest but the real maestro path is transport.maestro.manifestWork.ref; update the fixture used by TestTransportConfigYAMLParsing (or add a companion test case) so it places the manifest under transport.maestro.manifestWork.ref instead of resources[].manifest.ref, then adjust the assertions to load that nested shape (or keep the same raw-string assertions but verify they come from transport.maestro.manifestWork.ref) so regressions in the maestro ref-loading path are caught.
1460-1464: Cover the default validator path for string manifests.Every new happy-path string-manifest test calls
WithSkipSemanticValidation(), so none of them exercises the newstringbypass ininternal/configloader/validator.go. Keeping at least oneLoadConfigcase on the default path—TestLoadConfigManifestRefPlainYAMLis the easiest candidate—would prevent regressions from being masked by the test setup.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 1507-1511, 1625-1629
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/configloader/loader_test.go` around lines 1460 - 1464, Test suite bypasses the new string-manifest validation because most happy-path tests call WithSkipSemanticValidation(); add at least one LoadConfig invocation that does NOT pass WithSkipSemanticValidation() (e.g., modify TestLoadConfigManifestRefPlainYAML or add a new case) so the default validation path in internal/configloader/validator.go for "string" manifests is exercised; keep the callsite to LoadConfig and existing helper options (WithAdapterConfigPath, WithTaskConfigPath) but remove WithSkipSemanticValidation() for that case to ensure the string bypass logic is covered and prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build-integration-image.sh`:
- Around line 37-40: The current logic adds both HTTP_PROXY and HTTPS_PROXY
build-args together which can pass empty values and override inherited proxy
settings; update the BUILD_ARGS handling in scripts/build-integration-image.sh
so you only append --build-arg "HTTP_PROXY=..." when PROXY_HTTP is non-empty and
only append --build-arg "HTTPS_PROXY=..." when PROXY_HTTPS is non-empty (i.e.,
check PROXY_HTTP and PROXY_HTTPS separately before pushing to BUILD_ARGS),
preserving existing behavior of printing "Using proxy configuration" when at
least one proxy is set.
In `@test/Dockerfile.integration`:
- Around line 7-9: The Dockerfile currently leaves the image running as root
after the privileged RUN steps; add a USER 1001 instruction immediately after
the privileged operations (after the RUN dnf install -y tar && dnf clean all
block and before the CMD) to switch back to the non-root runtime user, and
ensure any directories referenced later (e.g., /tmp/envtest created on line 25)
are chowned or made writable for UID 1001 (adjust the RUN that creates those
dirs to set correct ownership/permissions) so the container can run as non-root.
---
Duplicate comments:
In `@internal/executor/resource_executor.go`:
- Around line 263-280: The rendered template output (from utils.RenderTemplate
called on manifestStr) can be empty even when the source manifestStr is
non-empty, which leads to yaml.Unmarshal leaving manifestData nil and
json.Marshal producing "null"; update the code after rendering to check that the
rendered string is not empty or only whitespace (e.g., trim and error if empty)
and also verify manifestData is non-nil after yaml.Unmarshal (return an error
like "rendered manifest is empty" if manifestData == nil) before calling
json.Marshal; modify the logic around rendered, manifestData, and json.Marshal
to return a clear error when the rendered document is empty.
- Around line 82-95: The discovery step currently calls discoverResource() using
the original resource.Manifest which can mismatch after templating; use the
decoded rendered object GVK instead: after json.Unmarshal(renderedBytes,
&obj.Object) (the block that sets result.Kind/Namespace/ResourceName) pass
obj.GetAPIVersion()/obj.GetKind() (or the unstructured.Unstructured obj itself)
into discoverResource()/the discovery call so discovery uses the rendered
manifest's GroupVersionKind; repeat the same change for the other occurrence
where discoverResource is invoked later (the block around the second
render/lookup).
---
Nitpick comments:
In `@internal/configloader/loader_test.go`:
- Around line 1332-1339: The test is asserting raw-string loading via
resources[].Manifest but the real maestro path is
transport.maestro.manifestWork.ref; update the fixture used by
TestTransportConfigYAMLParsing (or add a companion test case) so it places the
manifest under transport.maestro.manifestWork.ref instead of
resources[].manifest.ref, then adjust the assertions to load that nested shape
(or keep the same raw-string assertions but verify they come from
transport.maestro.manifestWork.ref) so regressions in the maestro ref-loading
path are caught.
- Around line 1460-1464: Test suite bypasses the new string-manifest validation
because most happy-path tests call WithSkipSemanticValidation(); add at least
one LoadConfig invocation that does NOT pass WithSkipSemanticValidation() (e.g.,
modify TestLoadConfigManifestRefPlainYAML or add a new case) so the default
validation path in internal/configloader/validator.go for "string" manifests is
exercised; keep the callsite to LoadConfig and existing helper options
(WithAdapterConfigPath, WithTaskConfigPath) but remove
WithSkipSemanticValidation() for that case to ensure the string bypass logic is
covered and prevent regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42beea71-0b11-42f2-a7de-bc2dfd15862e
📒 Files selected for processing (18)
internal/configloader/loader.gointernal/configloader/loader_test.gointernal/configloader/validator.gointernal/configloader/validator_test.gointernal/executor/executor_test.gointernal/executor/post_action_executor.gointernal/executor/resource_executor.gointernal/executor/resource_executor_test.gointernal/executor/utils.gointernal/executor/utils_test.gointernal/manifest/gvk.gointernal/manifest/gvk_test.gointernal/manifest/render_test.gopkg/utils/map_test.gopkg/utils/template_test.goscripts/build-integration-image.shscripts/container-runtime.shtest/Dockerfile.integration
💤 Files with no reviewable changes (2)
- internal/executor/executor_test.go
- internal/executor/utils_test.go
✅ Files skipped from review due to trivial changes (4)
- scripts/container-runtime.sh
- internal/executor/post_action_executor.go
- internal/configloader/validator_test.go
- internal/configloader/loader.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/configloader/validator.go
xueli181114
force-pushed
the
HYPERFLEET-864
branch
from
f9fe6cc to
b42f7ac
Compare
…manifest resources
Manifest ref files are now loaded as raw strings instead of being parsed as
YAML, allowing structural Go templates ({{ if }}, {{ range }}, {{ else }}) to
be used for conditional properties and list generation. Inline manifests support
the same templates via YAML multiline string syntax (manifest: |).
Templates are rendered at execution time when params are available, then the
rendered output is parsed as YAML. K8s manifest validation is skipped for string
manifests since validation cannot occur before template rendering. Template
variable validation is still performed at config load time.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
xueli181114
force-pushed
the
HYPERFLEET-864
branch
from
b42f7ac to
b07d3ba
Compare
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ref:files are now loaded as raw strings instead of being parsed as YAML, allowing structural Go templates ({{ if }},{{ range }},{{ else }}) to be used for conditional properties and list generationmanifest: |)Changes
loadRawFile()function;loadTaskConfigFileReferencesstores manifest refs as raw stringsrenderStringManifest()method handles string manifests (render template → parse YAML → marshal JSON)Test plan
{{ if }},{{ range }},{{ if/else }})Relates to: HYPERFLEET-864
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores