HYPERFLEET-864 - test: add Go template conditional coverage to cl-maestro e2e test#69
Open
xueli181114 wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
Open
HYPERFLEET-864 - test: add Go template conditional coverage to cl-maestro e2e test#69xueli181114 wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
xueli181114 wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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:
WalkthroughThis pull request externalizes an inline ManifestWork into a separate templated manifest file, adds CEL preconditions to capture Sequence Diagram(s)sequenceDiagram
participant TestClient as Test Client
participant Adapter as Adapter
participant Maestro as Maestro Transport
participant Managed as Managed Cluster (K8s API)
TestClient->>Adapter: create cluster with platform & labels
Adapter->>Adapter: evaluate CEL preconditions (platformType, environment, subnets)
Adapter->>Adapter: render manifest.ref template (ManifestWork with {{if}}/{{range}})
Adapter->>Maestro: submit ManifestWork reference (resourceBundle)
Maestro->>Managed: apply ManifestWork (Namespace + ConfigMap)
Managed-->>Maestro: resource creation status & feedback (.status/.data)
Maestro-->>Adapter: feedback rules/results (resourceVersion, data)
Adapter-->>TestClient: test assertions validate conditional label and rendered ConfigMap entries
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/adapter/manifest_go_template.go`:
- Around line 75-76: The test reads the generated ConfigMap once with
h.GetConfigMap which can race with Kubernetes propagation; replace the single
read with a retry/assert-until-success pattern (e.g., use a polling loop or
Gomega's Eventually) that repeatedly calls h.GetConfigMap for
"template-test-"+clusterID until it succeeds or a timeout elapses, then assert
Expect(err).NotTo(HaveOccurred()) and use the returned cm for subsequent
assertions; update the block around h.GetConfigMap/Expect to perform the
retry/backoff to eliminate flakiness.
🪄 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: de37bcba-1173-4486-8c10-ef7c1de57cc5
📒 Files selected for processing (5)
e2e/adapter/manifest_go_template.gotestdata/adapter-configs/cl-template/adapter-config.yamltestdata/adapter-configs/cl-template/adapter-task-config.yamltestdata/adapter-configs/cl-template/adapter-task-resource-configmap.yamltestdata/adapter-configs/cl-template/values.yaml
xueli181114
force-pushed
the
HYPERFLEET-864
branch
from
95ef3fc to
375f798
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testdata/adapter-configs/cl-template/adapter-task-resource-configmap.yaml`:
- Around line 4-20: The template references .clusterId, .clusterName, .testRunId
and .ci but the templateVars struct in pkg/client/payload.go only contains
Timestamp/TimestampMs/Random/UUID, so those fields render empty; fix by
extending templateVars to include ClusterID (string), ClusterName (string),
TestRunID (string or *string) and CI (bool or string) and ensure the code path
that builds/passes templateVars (the payload/template rendering callsite)
populates these fields from the cluster/test context before invoking
text/template execution; alternatively, if you prefer not to change payload.go,
refactor the YAML template to use only the existing
Timestamp/TimestampMs/Random/UUID fields and remove references to
.clusterId/.clusterName/.testRunId/.ci.
🪄 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: e6b65de3-540d-438e-b6c8-872dffaa353b
📒 Files selected for processing (10)
e2e/adapter/adapter_with_maestro.goe2e/adapter/manifest_go_template.gotestdata/adapter-configs/cl-maestro/adapter-config.yamltestdata/adapter-configs/cl-maestro/adapter-task-config.yamltestdata/adapter-configs/cl-maestro/adapter-task-resource-manifestwork.yamltestdata/adapter-configs/cl-maestro/values.yamltestdata/adapter-configs/cl-template/adapter-config.yamltestdata/adapter-configs/cl-template/adapter-task-config.yamltestdata/adapter-configs/cl-template/adapter-task-resource-configmap.yamltestdata/adapter-configs/cl-template/values.yaml
✅ Files skipped from review due to trivial changes (6)
- testdata/adapter-configs/cl-maestro/adapter-config.yaml
- testdata/adapter-configs/cl-template/adapter-config.yaml
- testdata/adapter-configs/cl-template/values.yaml
- testdata/adapter-configs/cl-maestro/values.yaml
- e2e/adapter/manifest_go_template.go
- testdata/adapter-configs/cl-template/adapter-task-config.yaml
testdata/adapter-configs/cl-template/adapter-task-resource-configmap.yaml
Outdated
Show resolved
Hide resolved
xueli181114
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test-design/testcases/adapter-with-maestro-transport.md (1)
69-72: Add one non-testcase if you want realelsecoverage.The new payload only drives
labels.environment: "test"and the expected result only checks"test", so this scenario proves the template containing{{ else }}parses, but not that theelsebranch renders. A second case with a non-testor missing label would close that gap.Also applies to: 110-112, 275-278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-design/testcases/adapter-with-maestro-transport.md` around lines 69 - 72, Add a second test case in the adapter-with-maestro-transport.md test matrix that drives a non-"test" environment (e.g., labels.environment: "prod" or omit labels.environment) so the Go template conditional using {{ if eq .environment "test" }} / {{ else }} exercises the else branch; update the expected result assertions to verify the alternate ConfigMap data produced by the else branch (or absence of the "test" value) and mirror this change for the other similar scenarios referenced (the places exercising {{ else }} behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test-design/testcases/adapter-with-maestro-transport.md`:
- Around line 95-96: Update the precondition text that describes how the Adapter
task config captures fields so it reflects the actual source: change the
statement that `environment` is captured from the cluster spec to say it is
captured from `labels.environment` (as implemented in
testdata/adapter-configs/cl-maestro/adapter-task-config.yaml); ensure the bullet
(currently "Adapter task config captures `platformType`, `environment`, and
`subnets` from cluster spec via CEL expressions") is edited to mention
`environment` comes from `labels.environment` while keeping `platformType` and
`subnets` as from the cluster spec.
In `@testdata/adapter-configs/cl-maestro/adapter-task-resource-manifestwork.yaml`:
- Around line 29-31: The checked-in YAML contains unrendered Go-template control
blocks (e.g., the "{{ if .platformType }}" / "hyperfleet.io/platform-type"
lines) which break YAML parsing; either move this asset to a template-aware
location or change its filename/extension (for example to .tmpl, .tpl, or
.yaml.tmpl) so tools treat it as a template, or update your CI/lint config to
exclude this file from YAML validation; ensure references to the template
markers ({{ if .platformType }}, {{ end }}) remain intact so rendering still
works.
---
Nitpick comments:
In `@test-design/testcases/adapter-with-maestro-transport.md`:
- Around line 69-72: Add a second test case in the
adapter-with-maestro-transport.md test matrix that drives a non-"test"
environment (e.g., labels.environment: "prod" or omit labels.environment) so the
Go template conditional using {{ if eq .environment "test" }} / {{ else }}
exercises the else branch; update the expected result assertions to verify the
alternate ConfigMap data produced by the else branch (or absence of the "test"
value) and mirror this change for the other similar scenarios referenced (the
places exercising {{ else }} behavior).
🪄 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: fb4a8e03-2143-4bee-8744-4847a7378761
📒 Files selected for processing (5)
e2e/adapter/adapter_with_maestro.gotest-design/testcases/adapter-with-maestro-transport.mdtestdata/adapter-configs/cl-maestro/adapter-task-config.yamltestdata/adapter-configs/cl-maestro/adapter-task-resource-manifestwork.yamltestdata/payloads/clusters/cluster-request.json
✅ Files skipped from review due to trivial changes (2)
- testdata/payloads/clusters/cluster-request.json
- e2e/adapter/adapter_with_maestro.go
🚧 Files skipped from review as they are similar to previous changes (1)
- testdata/adapter-configs/cl-maestro/adapter-task-config.yaml
testdata/adapter-configs/cl-maestro/adapter-task-resource-manifestwork.yaml
Show resolved
Hide resolved
xueli181114
force-pushed
the
HYPERFLEET-864
branch
from
8ad8204 to
6bb95f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testdata/adapter-configs/cl-maestro/adapter-task-resource-manifestwork.yaml`:
- Around line 79-83: The manifest template expects a .subnets array but the
render context only includes platformType/environment; update the data passed
into manifest.ref so it includes a "subnets" slice with each element having id,
name, cidr, and role fields (matching the template's {{ range $i, $subnet :=
.subnets }} usage). Locate the code that constructs the render data for
manifest.ref (the same place you added platformType and environment) and add the
subnets list there so the template can emit subnet_{{ $subnet.id }}_name,
subnet_{{ $subnet.id }}_cidr, and subnet_{{ $subnet.id }}_role for the e2e
assertions in adapter_with_maestro.go.
🪄 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: da74f1e9-fc44-4525-9591-c112c4a32bb2
📒 Files selected for processing (6)
e2e/adapter/adapter_with_maestro.gotest-design/testcases/adapter-with-maestro-transport.mdtestdata/adapter-configs/cl-maestro/adapter-task-config.yamltestdata/adapter-configs/cl-maestro/adapter-task-resource-manifestwork.yamltestdata/adapter-configs/cl-maestro/values.yamltestdata/payloads/clusters/cluster-request.json
✅ Files skipped from review due to trivial changes (3)
- testdata/adapter-configs/cl-maestro/values.yaml
- testdata/payloads/clusters/cluster-request.json
- test-design/testcases/adapter-with-maestro-transport.md
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/adapter/adapter_with_maestro.go
- testdata/adapter-configs/cl-maestro/adapter-task-config.yaml
testdata/adapter-configs/cl-maestro/adapter-task-resource-manifestwork.yaml
Show resolved
Hide resolved
Contributor
Author
|
Re: CodeRabbit comment on Not valid. The |
…cl-maestro e2e test
xueli181114
force-pushed
the
HYPERFLEET-864
branch
from
44a0e7b to
6b1d6d7
Compare
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
{{ if }},{{ else }},{{ end }})platformTypefromspec.platform.type,environmentfromlabels.environment)cl-templateadapter config with standalone Go template e2e test for K8s transportbase_url: CHANGE_MEin cl-maestro adapter-config.yamlTest plan
make buildpassesmake checkpasses (fmt, vet, lint, unit tests)hyperfleet.io/platform-type: gcplabelenvironment: testConfigMap dataDepends on: openshift-hyperfleet/hyperfleet-adapter#96
Relates to: HYPERFLEET-864
Summary by CodeRabbit
New Features
Tests